913972 - Australis: overflowable toolbar reorders content, changes currentSet value

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

STR:

0. Enable a social provider
1. Make the window small enough to overflow all the overflowable content into the overflow panel.
2. Make the window big enough to get it all out.

ER:
The order of items is the same

AR:
The order of items is not the same

Note: after 1 and before 2, the currentSet value is also not the same anymore, even though the placements array hasn't changed. This also shouldn't happen.

Comment 1

[Mike de Boer [:mikedeboer]]

I found this problem (or very similar) while working on the overflow panel/ button in bug 885086

There is a patch that fixes it in that bug.

Comment 2

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

Should this bug be duplicated to that bug then?

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] from comment #2)
> Should this bug be duplicated to that bug then?

No, the patch should be moved to this bug. I can take it. Gijs, you ok with that?

Comment 4

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> (In reply to Jared Wein [:jaws] from comment #2)
> > Should this bug be duplicated to that bug then?
> 
> No, the patch should be moved to this bug. I can take it. Gijs, you ok with
> that?

Your patch doesn't address the currentSet or inDefaultState issue, though. It also stores node references to keep track of the order - nodes which might go away in the meantime. So it seems pretty fragile.

I think the first thing we should do is address the currentSet and inDefaultState issues. I have a local patch + test for that, but unfortunately I'm running into various versions of bug 913780 when checking it against all the other tests in our suite.

Once that situation is sorted out, we can then use the currentSet/placements to reinsert items in the correct order. This is more robust than your solution, IMO. I think the work in bug 885086 (to actually restyle the overflow panel) can be done separately.

Comment 5

[:Gijs (he/him)]

Attachment: part 0: don't move skipintoolbarset stuff so much,

As discussed on IRC, let's not mess with these during a reset/buildArea

Comment 6

[:Gijs (he/him)]

Attachment: Part 1: Use placements for currentSet so it doesn't reorder because of overflow

So, I'm using currentSet rather than duplicating that code and/or making the inDefaultState getter learn about overflowable toolbars and so on. This meant that I did need to teach the currentSet code about customize mode (ie, wrappers), but that technically needed to happen anyway so that add-ons relying on it wouldn't break if their code happened to run while the user was in customize mode.

Comment 7

[:Gijs (he/him)]

Attachment: Add some supporting assertions,

These all pass if I only run the tests in customizableui right now, and I think it would be helpful to know if any of the tests finish while violating any of these invariants.

Comment 8

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

Comment on attachment 801682 [details] [diff] [review]
part 0: don't move skipintoolbarset stuff so much,

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

Looks right to me.

Comment 9

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

Comment on attachment 801694 [details] [diff] [review]
Part 1: Use placements for currentSet so it doesn't reorder because of overflow

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

::: browser/components/customizableui/content/toolbar.xml
@@ +162,5 @@
>                }
>              }
>            }
> +          let orderedPlacements = CustomizableUI.getWidgetIdsInArea(this.id);
> +          return orderedPlacements.filter((x) => currentWidgets.has(x)).join(',');

How do we know that this will be the correct order? getWidgetIdsInArea returns gPlacements, which is a Map. And currentWidgets is a Set, so I don't think preserving order is a requirement for those containers.

::: browser/components/customizableui/test/browser_913972_currentset_overflow.js
@@ +4,5 @@
> +
> +let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +let gTests = [
> +  {
> +    desc: "Resize to a small window, resize back, shouldn't affect currentSet",

Maybe add another test that enters customization mode and sees that currentset hasn't changed?

Comment 10

[:Gijs (he/him)]

Attachment: Part 1: Use placements for currentSet so it doesn't reorder because of overflow

Added an extra test. We discussed the ordering thing over IRC, that should be fine (it's a map of strings to arrays, the arrays (which is what getWidgetIdsInArea will fetch you from that map) are in order).

Comment 11

[:Gijs (he/him)]

Attachment: Part 3: use placements to reinsert items into toolbar,

This fixes the ordering of reinserted items using the placements array, and tests that, too.

Comment 12

[:Gijs (he/him)]

Landed in the hope that these will eradicate our OSX bc orange.

https://hg.mozilla.org/projects/ux/rev/bfb5c27c566b
https://hg.mozilla.org/projects/ux/rev/2d5db6991535
https://hg.mozilla.org/projects/ux/rev/55e70c56cea0
https://hg.mozilla.org/projects/ux/rev/6cea80519bc4

Comment 13

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/6cea80519bc4
https://hg.mozilla.org/mozilla-central/rev/55e70c56cea0
https://hg.mozilla.org/mozilla-central/rev/2d5db6991535
https://hg.mozilla.org/mozilla-central/rev/bfb5c27c566b