914138 - Australis: CustomizableUI API calls will produce unpredictable/wrong results if either/both of the nodes they touch are in overflowable toolbar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

STR:

1. Open Firefox.
2. Open a second window.
3. Make the first window small enough to make most of the items end up in the overflowable toolbar
4. Open customize mode in the second (ie, the big) window.
5. Add an item to the navbar

ER:
Item ends up in the same place in both windows; between items in the overflow panel if necessary

AR:
Item ends up in different places in both windows. If you were to do anything to this area later on, the difference with the expected placements will probably confuse other things still more.

Related to bug 884402 as well, I'd imagine.

Comment 1

[:Gijs (he/him)]

Attachment: 914138-inserting-overflowable-toolbar

So this is what I have right now. Asking for feedback only because:

(a) the test isn't landable as-is (needs at least a try run to verify it passes on non-mac platforms, on try machines with whatever resolutions they have, etc., but ideally, need to come up with a less fragile way of testing this than the hardcoded widths - suggestions welcome!), and

(b) my gut feeling about the code is that it's too complex and fragile. Not sure how to do better, though - the basic concept of the overflow toolbar as-is is easy in theory, but in practice, with content nodes that can arbitrarily change size, flex, and no reliable overflow events[0] this is already not the most predictable code. Would love to have some suggestions as to how to improve this patch, but asking for feedback as I've stared at this for too many days already.

[0] sometimes they fire with identical clientWidth and scrollWidth? I don't understand this. They then don't fire again, sometimes leaving things in an overflown state without further notification, meaning stuff disappears under the chevron but doesn't get included in the overflow menu - already a problem without this patch.

Comment 2

[:Gijs (he/him)]

Oh, also: I've architected this with bug 884402 comment 3 in the back of my mind. That should be workable with this patch, although some of the parentNode checks would need to become ancestor checks.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 808760 [details] [diff] [review]
914138-inserting-overflowable-toolbar

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

I hear new patches should be on their way...

Comment 4

[:Gijs (he/him)]

Attachment: refactor some code, append to the overflowable toolbar in the right spot if necessary,

This mostly just refactors the add/move code to share code, and tweaks the overflowable toolbar to have a toolbox and customizationTarget reference, taking care of the requirements of some of our code that containing nodes have those. This passes all the existing tests for me locally, independent of other changes.

Comment 5

[:Gijs (he/him)]

Attachment: test scaffolding fix, minor overflow checking issue,

This is a minor test framework change to check state after teardown, not between test running and teardown, so that we can use the teardown to clean up, even if test code fails for some reason. It also adds a supporting method for the test to be able to use Task.jsm style 'sleep', and fixes our overflow event handler to only care about horizontal overflow.

Comment 6

[:Gijs (he/him)]

Attachment: add the toolbar as a listener, and do bookkeeping for inserted/removed nodes,

This makes the toolbar listen for CUI events. This works, but it's pretty hairy. I've tried to liberally comment the code. Please check my assumptions carefully.

Comment 7

[:Gijs (he/him)]

Attachment: add the toolbar as a listener, and do bookkeeping for inserted/removed nodes,

Actually, this is nicer (adding/removing the listener when we start/stop overflowing, to reduce overhead

Comment 8

[:Gijs (he/him)]

Attachment: add the toolbar as a listener, and do bookkeeping for inserted/removed nodes,

I am dumb, and missed a brace. This works (tested and everything...)

Comment 9

[:Gijs (he/him)]

Attachment: add a test for the overflow panel insertion/additions,

This is the test... it's basically the bit I'm the least happy about. :-|

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 815560 [details] [diff] [review]
refactor some code, append to the overflowable toolbar in the right spot if necessary,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +735,5 @@
>      aNode.removeAttribute("customizableui-areatype");
>      aNode.removeAttribute("customizableui-anchorid");
>    },
>  
> +  insertNode: function(aWidgetId, aNextNodeId, aAreaNode, aArea, isNew) {

It'd be nice if this function (and findNextNode) didn't take so many arguments, but I don't have a better recommendation.

@@ +2502,5 @@
> +    }
> +
> +    let nextNode = this._list.querySelector(idToSelector(aNextNodeId));
> +    // If this is the first item, we can actually just append the node to the toolbar.
> +    // If it overflows, we'll get an event and should be fine.

Please change this to:
// If this is the first overflowed item, we can actually just append the node
// to the end of the toolbar. If it results in an overflow event, we'll move
// the new node to the overflow target.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 815597 [details] [diff] [review]
add the toolbar as a listener, and do bookkeeping for inserted/removed nodes,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2522,5 @@
> +        updatedMinSize = 1;
> +      }
> +      let nextItem = aNode.nextSibling;
> +      while (nextItem) {
> +        this._collapsed.set(nextItem.id, updatedMinSize);

Isn't this already set within this._onOverflow?

@@ +2545,5 @@
> +        // NB: we're guaranteed that it has a previousSibling, because if it didn't,
> +        // we would have added it to the toolbar instead. See getOverflowedNextNode.
> +        let prevId = aNode.previousSibling.id;
> +        let minSize = this._collapsed.get(prevId);
> +        this._collapsed.set(aNode.id, minSize);

Why not just use the already implemented _lazyResizeHandler?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 815605 [details] [diff] [review]
add a test for the overflow panel insertion/additions,

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

::: browser/components/customizableui/test/browser_914138_widget_API_overflowable_toolbar.js
@@ +61,5 @@
> +      // This is pretty weird. We're going to try to move only the home button into the overlay:
> +      let downloadsBtn = document.getElementById(kDownloadsBtn);
> +      // Guarantee overflow of too much stuff:
> +      window.resizeTo(700, window.outerHeight);
> +      let w = 700;

w is unused.

@@ +71,5 @@
> +      }
> +    },
> +    run: function() {
> +      CustomizableUI.removeWidgetFromArea("downloads-button");
> +      is(document.getElementById("home-button").parentNode, navbar.customizationTarget, "Home button should move back.");

Please add a check that makes sure that the downloadsBtn and homeBtn are overflowed before we start to increase the window size.

@@ +137,5 @@
> +      yield waitForCondition(() => navbar.querySelector("#" + kSearchBox));
> +      CustomizableUI.moveWidgetWithinArea(kStarBtn);
> +      window.resizeTo(670, window.outerHeight);
> +      yield waitForCondition(() => navbar.querySelector("#" + kDownloadsBtn));
> +      CustomizableUI.moveWidgetWithinArea(kHomeBtn);

Should probably add some assertions here that the buttons did move to the end of the area.

Comment 13

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #11)
> Comment on attachment 815597 [details] [diff] [review]
> add the toolbar as a listener, and do bookkeeping for inserted/removed nodes,
> 
> Review of attachment 815597 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +2522,5 @@
> > +        updatedMinSize = 1;
> > +      }
> > +      let nextItem = aNode.nextSibling;
> > +      while (nextItem) {
> > +        this._collapsed.set(nextItem.id, updatedMinSize);
> 
> Isn't this already set within this._onOverflow?

That only happens when an item is overflowed via the events. Because we're "manually" inserting/removing nodes here, this implies adjusting the metadata. Otherwise we'd need to move everything in and out manually.

> 
> @@ +2545,5 @@
> > +        // NB: we're guaranteed that it has a previousSibling, because if it didn't,
> > +        // we would have added it to the toolbar instead. See getOverflowedNextNode.
> > +        let prevId = aNode.previousSibling.id;
> > +        let minSize = this._collapsed.get(prevId);
> > +        this._collapsed.set(aNode.id, minSize);
> 
> Why not just use the already implemented _lazyResizeHandler?

I'm confused. This collection of patches will insert nodes directly into the overflow panel if that's where their position puts them. They might not be the first node - in fact, it should be possible for them to be the last node. How would calling _lazyResizeHandler help? We actually don't know at what point this node could be moved back to the toolbar, either, because we don't know at what width it would 'naturally' have gone into the overflow panel.

Rather than all this mucking about, yes, we could temporarily move all the items back into the toolbar, then insert something, and then call ._onOverflow, or something, but it'd be highly visible, which I find a little yucky.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 815597 [details] [diff] [review]
add the toolbar as a listener, and do bookkeeping for inserted/removed nodes,

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

Yeah, the UI would jump around quite a bit if we waited for events.

Comment 15

[:Gijs (he/him)]

Attachment: refactor some code, append to the overflowable toolbar in the right spot if necessary,

I've refactored a bit more so the number of arguments is smaller (and so is the amount of duplicated code). I've also fixed a bug where items weren't inserted at the very end of the overflow list, which was uncovered by your requests for additional checks in the test. :-)

Comment 16

[:Gijs (he/him)]

Attachment: add the toolbar as a listener, and do bookkeeping for inserted/removed nodes

This patch bitrotted because of my changes to the other one, carrying over review.

Comment 17

[:Gijs (he/him)]

Attachment: add a test for the overflow panel insertion/additions,

Test w/ nits fixed and additional assertions, carrying over review

Comment 18

[:Gijs (he/him)]

Landed:

https://hg.mozilla.org/projects/ux/rev/6b916ebaefde
https://hg.mozilla.org/projects/ux/rev/f16b1a539767
https://hg.mozilla.org/projects/ux/rev/9b7e65b82aed
https://hg.mozilla.org/projects/ux/rev/33e7c00f5e24

and

https://hg.mozilla.org/projects/ux/rev/462cb39be982

because apparently my brain has been turned to mush.


The restriction as regards the test run was discussed with Jared. Followup bug to make the test more robust: bug 927064.

Comment 19

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/33e7c00f5e24
https://hg.mozilla.org/mozilla-central/rev/9b7e65b82aed
https://hg.mozilla.org/mozilla-central/rev/f16b1a539767
https://hg.mozilla.org/mozilla-central/rev/6b916ebaefde
https://hg.mozilla.org/mozilla-central/rev/462cb39be982