[PATCH 08 of 19] autocomplete: clean up handlers for itemSelectEvent

[Thomas De Schampheleire]

On Tue, Jul 14, 2015 at 3:28 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 07/14/2015 01:36 PM, Thomas De Schampheleire wrote:
>>
>> On Tue, Jul 14, 2015 at 12:57 PM, Mads Kiilerich <mads at kiilerich.com>
>> wrote:
>>>
>>> On 06/30/2015 10:43 PM, Thomas De Schampheleire wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>>>> # Date 1435349220 -7200
>>>> #      Fri Jun 26 22:07:00 2015 +0200
>>>> # Node ID 98519504e88827b408447efcb9ea01ca270a7b0b
>>>> # Parent  623155b700e275d039d1a9065f272dc6fa4fb569
>>>> autocomplete: clean up handlers for itemSelectEvent
>>>>
>>>> The handlers that execute when an autocompleted item is selected are
>>>> really
>>>> specific and cannot be factored out. Nevertheless, some cleanup:
>>>>
>>>> - rationalize indentation
>>>> - remove checks on the existence of xAC.itemSelectEvent: there seems to
>>>> be
>>>>     no reason at all to check this, the YUI documentation also does not
>>>>     mention it.
>>>
>>>
>>> Grumpy users tell me that itemSelectEvent can be null. I cannot reproduce
>>> it
>>> ... but a blind fix restoring the check made them happy.
>>>
>>> Have you tried testing this with a wider audience?
>>
>> Hmm, was this with the entire patch series applied?
>
>
> Yes.
>
>> I have seen such a problem when the members autocomplete was still
>> handling two different autocomplete instances, but after splitting
>> that it was fixed...
>>
>> On which page do they see it?
>
>
> On pull requests, trying to click the 'add comment' bubble.
>
>> And are you sure that they don't suffer from browser caching?
>
>
> Quite. I bump the version number every time I deploy, and the version number
> goes in the base.js url. When I restored the fix it immediately started
> working for them again.
>

You said you could not reproduce this. Did you see the problem in live action?
Which browser was used? Do you have more detailed logs, Javascript
console logs, ... ?

I looked through the code and again through the YUI documentation. The
documentation nowhere mentions the need to check that value, all
examples do without it. If I look at the code, the itemSelectEvent is
in some way initialized to null at
https://github.com/yui/yui2/blob/master/src/autocomplete/js/AutoComplete.js#L1203.
When a new autocomplete object is created, the itemSelectEvent object
is populated:
https://github.com/yui/yui2/blob/master/src/autocomplete/js/AutoComplete.js#L206

If this is not done, then I would think that either we never reach
that line due to an earlier return (but looking at the different cases
I can't imagine that they can occur, and moreover it would mean that
mentions autocomplete in these boxes would not work (did your users
verify that?)), or the itemSelectEvent is not correctly filled because
CustomEvent fails.
So either case, it would be an error and something will be wrong, I'd
expect some output in the JavaScript console. Adding back the check
seems like a workaround, I'd rather get to the bottom of this.

I tried several things in my local instance but also could not make it fail.
I did find it odd that the input elements and containers for the
inline comment forms have IDs that are based on the line number alone,
meaning that you will have two containers/input elements with the same
ID if you add comments to line 5 of two different files in the same
pull request, but I could not use that to trigger a failure. It seems
to just work here.

/Thomas