Closed Bug 913972 Opened 7 years ago Closed 6 years ago

Australis: overflowable toolbar reorders content, changes currentSet value

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M9][Australis:P2])

Attachments

(4 files, 1 obsolete file)

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.
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.
Should this bug be duplicated to that bug then?
Flags: needinfo?(mdeboer)
(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?
Flags: needinfo?(mdeboer)
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
As discussed on IRC, let's not mess with these during a reset/buildArea
Attachment #801682 - Flags: review?(mconley)
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.
Attachment #801694 - Flags: review?(jaws)
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.
Attachment #801697 - Flags: review?(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.
Attachment #801682 - Flags: review?(mconley) → review+
Attachment #801697 - Flags: review?(mconley) → review+
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?
Attachment #801694 - Flags: review?(jaws) → feedback+
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).
Attachment #801826 - Flags: review?(jaws)
Attachment #801694 - Attachment is obsolete: true
This fixes the ordering of reinserted items using the placements array, and tests that, too.
Attachment #801829 - Flags: review?(jaws)
Attachment #801826 - Flags: review?(jaws) → review+
Attachment #801829 - Flags: review?(jaws) → review+
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
Whiteboard: [Australis:M9][Australis:P2] → [Australis:M9][Australis:P2][fixed-in-ux]
Depends on: 914562
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P2][fixed-in-ux] → [Australis:M9][Australis:P2]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.