On 07/15/2015 09:57 PM, Thomas De Schampheleire wrote:
On Tue, Jul 14, 2015 at 3:28 PM, Mads Kiilerich <[email protected]> wrote:
On 07/14/2015 01:36 PM, Thomas De Schampheleire wrote:
On Tue, Jul 14, 2015 at 12:57 PM, Mads Kiilerich <[email protected]>
wrote:
On 06/30/2015 10:43 PM, Thomas De Schampheleire wrote:
# HG changeset patch
# User Thomas De Schampheleire <[email protected]>
# 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
_______________________________________________
kallithea-general mailing list
[email protected]
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
