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

[Mads Kiilerich]

On 07/15/2015 09:57 PM, Thomas De Schampheleire wrote:
> 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 didn't really see it myself. The 3 direct reports I saw was from 
non-admin users on windows, not using firefox. The javascript console 
showed the error of itemSelectEvent being null. I could not reproduce on 
linux with firefox or chrome.


> 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

Perhaps some issue with callback order?

One solution could be to move away from yui ;-)

> 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.

The users couldn't even create a comment form because the javascript 
crashed before it came that far.

> 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.

Yes - but from another perspective, we shouldn't remove the check before 
we have gotten to the bottom of it.

> 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.

Right. I think I have noticed that collision too. It is no big issue as 
long as people only create one comment at a time - potentially more of 
an issue when people can "commit" multiple issues at once.

/Mads