1172651 - browser_968565_insert_before_hidden_items.js is going to permafail on Win7 when Gecko 41 merges to Aurora

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Hit this on two different runs, both on Win7 opt.

https://treeherder.mozilla.org/logviewer.html#?job_id=8234976&repo=try

22:04:06 INFO - 1080 INFO Entering test
22:04:06 INFO - 1081 INFO TEST-PASS | browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js | Should be in the default state
22:04:06 INFO - 1082 INFO TEST-PASS | browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js | The hidden item should be before the home button
22:04:06 INFO - 1083 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js | The hidden item should still be before the home button - Got search-container, expected home-button
22:04:06 INFO - Stack trace:
22:04:06 INFO - chrome://mochikit/content/browser-test.js:test_is:925
22:04:06 INFO - chrome://mochitests/content/browser/browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js:null:78
22:04:06 INFO - self-hosted:InterpretGeneratorResume:716
22:04:06 INFO - self-hosted:next:674
22:04:06 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:887:23
22:04:06 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:766:7
22:04:06 INFO - this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:39
22:04:06 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
22:04:06 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:729:7
22:04:06 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:687:7
22:04:06 INFO - onNewTabLoaded@chrome://mochitests/content/browser/browser/components/customizableui/test/head.js:209:7
22:04:06 INFO - 1084 INFO TEST-PASS | browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js | The downloads button should now be before the hidden button
22:04:06 INFO - 1085 INFO Leaving test

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Sorry, hit on both Win7 opt and debug.

Comment 2

[:Gijs (he/him)]

I don't understand why this would suddenly start failing when going to Aurora, when AIUI all of what we're doing (pocket etc.) is already on aurora. This isn't the same test as the one in bug 1163231, either. Matt, thoughts/ideas?

Comment 3

[:Gijs (he/him)]

I've been looking at this but apparently even to run tests against an already-built executable I need a working build environment, and my MSVS in the VM is outdated, and arrgh. So going to take a bit longer still. Sorry.

Comment 4

[Matthew N. [:MattN]]

Dolske was looking at this test before. IIRC we didn't have this earlier because Pocket was disabled on Aurora. IIRC the problem is that DevEdition adds 2 extra toolbar buttons on the toolbar and adding Pocket to that changes what overflows in this test. Dolske would know better about how we avoided this on current Aurora.

Comment 5

[Justin Dolske [:Dolske]]

Bug 1163231 is what became of that, and it was fixed (on m-c too) by disabling the test that was somehow causing things the screw up (apparently the overflow we saw in the failure screenshots was just a symptom of that). So I don't know what would cause it to start failing anew.

Comment 6

[:Gijs (he/him)]

This isn't the same test as bug 1163231, it's not even the entire list in bug 1163231 comment #0.

Comment 7

[:Gijs (he/him)]

bug 1154140 (and some other ChromeUtils changes) messed with our dragging stuff, and now this test is actually dragging the *wrong item*. It's being called with the downloads-button, but it's the search bar that actually moves. I don't know why. There is no toolbar overflow at any point. Tooru/Neil, can you have a look?

Comment 8

[Tooru Fujisawa [:arai]]

Oops, it should be caused by ".parentNode"s in following line, those should be removed.

https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js#75
>  simulateItemDrag(downloadsButton.parentNode, homeButton.parentNode);

Because simulateItemDrag accesses to parentNode again, so the 1st argument will be toolbar, not button.

> function simulateItemDrag(aToDrag, aTarget) {
>   synthesizeDrop(aToDrag.parentNode, aTarget);
> }

Actually, the test expects nothing is moved by the drag (it's dragging to same place), and it's my mistake that it doesn't drag anything but toolbar itself (which doesn't move anything) on Nightly and it passed the test wrongly. on Aurora on windows 7, it moves search bar because it's placed at the center of the toolbar.

I'll try fixing it shortly.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Hidden button is placed before the dragged item and visible after drag-and-drop

and now I'm experiencing the hidden item is placed *before* the downloads button, and the *hidden* item is actually visible as empty box, after the drag-and-drop :/
even with manual drag-and-drop.

any idea?

Comment 10

[Tooru Fujisawa [:arai]]

sorry, the empty box was caused by my another broken patch.
the place seems to be still wrong tho.

Comment 11

[Tooru Fujisawa [:arai]]

I don't see any code which corresponds to following comment in browser_968565_insert_before_hidden_items.js.
> // When we drag an item onto a target that has a hidden element before it, we should
> // instead place the new item before the hidden elements.

_findVisiblePreviousSiblingNode seems to be similar one, but it handles when the target is hidden, not when the previous element is.
So, currently, if an element is dropped onto a hidden element, it will be placed before the first visible element before the hidden element. If an element is dropped onto a visible element, it's placed just before the visible element, regardless of the existence of previous hidden elements.

Which is correct the comment or the current behavior?

Comment 12

[:Gijs (he/him)]

(In reply to Tooru Fujisawa [:arai] from comment #11)
> I don't see any code which corresponds to following comment in
> browser_968565_insert_before_hidden_items.js.
> > // When we drag an item onto a target that has a hidden element before it, we should
> > // instead place the new item before the hidden elements.
> 
> _findVisiblePreviousSiblingNode seems to be similar one, but it handles when
> the target is hidden, not when the previous element is.
> So, currently, if an element is dropped onto a hidden element, it will be
> placed before the first visible element before the hidden element. If an
> element is dropped onto a visible element, it's placed just before the
> visible element, regardless of the existence of previous hidden elements.
> 
> Which is correct the comment or the current behavior?

The comment is correct, I don't understand why the test wasn't failing before your dnd changes.

The code here looks broken also because we do after/before magic, and I don't really understand why, in the following situation:


insertion point here ---> .
[button] [hidden] [hidden] [button]

it would be better to insert directly after the first button and not directly before the last button.

I imagine that you can probably remove the call to the _findVisiblePreviousSiblingNode function from the case where the parent isn't the toolbar node itself (but we're dragging/dropping in a toolbar area), assuming the toolbarpaletteitems are properly hidden (take up 0 width on the toolbar) and remove the second test.

Mike, does that sound right?

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Insert dropped item before hidden elements in toolbar customization.

Okay, removed _findVisiblePreviousSiblingNode from the case where item is dropped to the button, and added some code to follow the comment in the test, into _onDragDrop handler, so, it keeps the target node same while dragging, and handle the hidden elements while resolving before/after.  It searches the first element which has visible previous element.  It also skips the dragged element itself.

In the test, I removed the .parentNode from source node (downloadsButton). destination node stays same (homeButton.parentNode), because it actually receives the event, when I do it manually (sending events to homeButton itself opens the confirmation dialog for setting home page).

Before my DnD patch in bug 1154140, the test also didn't drag anything, because it passed the wrapper (downloadsButton.parentNode) to simulateItemDrag, and dropping with wrapper's id does nothing, then, the test expects nothing is moved, and passed wrongly.  So, to prevent same mistake again, might be better to use another toolbar button, and expect the button is actually moved? (maybe in different bug?)

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95f994f32ad

Comment 14

[:Gijs (he/him)]

Comment on attachment 8622949 [details] [diff] [review]
Insert dropped item before hidden elements in toolbar customization.

This is more complicated than it needs to be. Assuming Mike agrees with me, I think we should remove the second add_task in the test, and the this._findVisiblePreviousSiblingNode code.

As noted in my previous comment, I don't actually understand why inserting something before the hidden nodes is better when the hidden nodes aren't at the end of the area, so in the:

insertion point here ---> .
[button] [hidden] [hidden] [button]

case, why move to insert before the hidden item instead of directly before the button?

If you look at the original bug, this would solve the case where the search bar has been moved, and you're dragging the search bar, and the toolbar looks like:


[url field] [hidden button] [visible button]

and you insert before the visible button. On the other hand, it doesn't really help if you were moving the search bar and the situation looks like this:


[button] [hidden button] [url field]

because now we'll actively move the search bar away from the url field.

I think we should just not mess with the drop location here. It's sucky, but I don't see how our messing helps any. Still waiting on Mike to confirm my suspicion. Going to cancel the ni on Neil for now.

If we do need to mess with the insertion point, we should do so while dragging, not while dropping, so that the placeholders and so on show the right thing happening in all cases (even silly ones where add-on button styling overrides the default styles for [hidden="true"] !)

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8622949 [details] [diff] [review]
> Insert dropped item before hidden elements in toolbar customization.
> 
> This is more complicated than it needs to be. Assuming Mike agrees with me,
> I think we should remove the second add_task in the test, and the
> this._findVisiblePreviousSiblingNode code.

... the *call* that this patch does already remove, not all of it, to be clear.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Remove a test for unimplemented behavior from browser_968565_insert_before_hidden_items.js.

Thank you for the detailed explanation :)
Removed the 2nd test.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff43407f22b0

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8622949 [details] [diff] [review]
> Insert dropped item before hidden elements in toolbar customization.
> 
> This is more complicated than it needs to be. Assuming Mike agrees with me,
> I think we should remove the second add_task in the test, and the
> this._findVisiblePreviousSiblingNode code.
> 
> As noted in my previous comment, I don't actually understand why inserting
> something before the hidden nodes is better when the hidden nodes aren't at
> the end of the area, so in the:
> 
> insertion point here ---> .
> [button] [hidden] [hidden] [button]
> 
> case, why move to insert before the hidden item instead of directly before
> the button?
> 
> If you look at the original bug, this would solve the case where the search
> bar has been moved, and you're dragging the search bar, and the toolbar
> looks like:
> 
> 
> [url field] [hidden button] [visible button]
> 
> and you insert before the visible button. On the other hand, it doesn't
> really help if you were moving the search bar and the situation looks like
> this:
> 
> 
> [button] [hidden button] [url field]
> 
> because now we'll actively move the search bar away from the url field.
> 
> I think we should just not mess with the drop location here. It's sucky, but
> I don't see how our messing helps any. Still waiting on Mike to confirm my
> suspicion. Going to cancel the ni on Neil for now.
> 
> If we do need to mess with the insertion point, we should do so while
> dragging, not while dropping, so that the placeholders and so on show the
> right thing happening in all cases (even silly ones where add-on button
> styling overrides the default styles for [hidden="true"] !)

The only reason I can think of why we might have done this is for the URL bar / search bar resizer thing.

Like, suppose we have this:

insert search bar here ----.
[url bar] [hidden] [hidden] [button]

Then the result will be:

[url bar] [hidden] [hidden] [search bar] [button]

And the resizer grip between the url bar and search bar will not work, despite the two items appearing to be next to one another.

That's the only reason why I think we would have done this.

Comment 18

[Tooru Fujisawa [:arai]]

Comment on attachment 8623288 [details] [diff] [review]
Remove a test for unimplemented behavior from browser_968565_insert_before_hidden_items.js.

So, anyway, applying the insert-before-previous-hidden-elements or any other rule to *everything* won't be a good approach, and, (if it's really needed), we should handle those [url bar][search bar] and [search bar][url bar] cases specially, right?
If so, I'd like to leave it to another bug, and land this patch for now to avoid aurora perm-failure.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8623288 [details] [diff] [review]
Remove a test for unimplemented behavior from browser_968565_insert_before_hidden_items.js.

Review of attachment 8623288 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, considering this actually never did what it was intended to, but I've not seen new reports of this, let's remove the busted test and useless code, and we can fix the searchbar/urlbar case specifically if the issue comes back... also because as I noted in comment 14, there are cases where moving it as the comment indicates wouldn't help the urlbar/searchbar situation.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a57875030d60

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a57875030d60