980155 - Initialization of overflowable toolbars breaks when adding them dynamically

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Turns out our code for initializing overflowable toolbars breaks when you try adding them dynamically - ie, after window load.

Add-ons will end up using this, so we should fix it. Also, it's getting in the way of testing.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

WiP patch coming up, but I can't for the life of me get it to actually work.

Comment 2

[:Gijs (he/him)]

This is a dupe, but it's past midnight here and I'm on a phone. Has to do with how we moved the overflow event handling into the binding for perf + consistency reasons.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

That and we're also just not calling init() at all - we listen for the delayed startup event without checking first if it's already completed.

Comment 4

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

Assigning based on comment #1.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

This *seems to* initialize everything properly, but the toolbar doesn't actually overflow. Debugging shows the overflow event handler never gets called.

Gijs, you alluded to some knowledge about what might be going on here.

Comment 6

[:Gijs (he/him)]

(In reply to Blair McBride [:Unfocused] from comment #5)
> Created attachment 8387556 [details] [diff] [review]
> WiP v1
> 
> This *seems to* initialize everything properly, but the toolbar doesn't
> actually overflow. Debugging shows the overflow event handler never gets
> called.
> 
> Gijs, you alluded to some knowledge about what might be going on here.

bug 929563, which predicts this problem... but I don't remember why.

Comment 7

[:Gijs (he/him)]

IRC context:

[2013-10-22 17:48:55] <Gijs> jaws: hrm. I just realized my patch isn't watertight in case people add an overflowable toolbar in an add-on, if the constructor runs before the area is registered. :(
[2013-10-22 17:49:10] <Gijs> jaws: in that case, no overflow handler would be added.
[2013-10-22 17:49:31] <Gijs> jaws: (as the isAreaOverflowable method would return false)
[2013-10-22 17:49:59] <Gijs> jaws: is it OK if I file a followup for that? Add-ons adding overflowable toolbars is pretty edge-casey anyway...


I'll look into this when I'm at the office and after that other bug...

Comment 9

[:Gijs (he/him)]

Attachment: Initialization of overflowable toolbars breaks when adding them dynamically

So it needs a customization target that's separate (which is assumed because you'd normally always want to have a chevron in the toolbar that shows the overflow, which implies you need a separate node) and that has flex 1, and then stuff seems to work? Not sure if you wanted to add more tests here.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8387738 [details] [diff] [review]
Initialization of overflowable toolbars breaks when adding them dynamically

Passing feedback flag back.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8387738 [details] [diff] [review]
Initialization of overflowable toolbars breaks when adding them dynamically

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

That did that trick (thankyou!).

New patch coming up, rebased so it applies cleanly. That way we can land this without bug 976792, and land 976792 on top - with that bug having full test coverage. Also added test coverage specifically for this bug, and cleaned up head.js a bit.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 13

[:Gijs (he/him)]

Comment on attachment 8388042 [details] [diff] [review]
Patch v1

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

This WFM, but we should undupe the bug I duped here because we haven't fixed the case where registerArea is called after the node is added (which would be the case for overlaid toolbars - this is why I moved the registerArea call in your head.js function). Unless we want to explicitly wontfix that, I guess? Otherwise, I suspect that adding a check for the overflowable="true" attribute (in addition to the "proper" CUI.isOverflowable check) in the binding will be enough to fix that bug, too, but I don't really want to scope-creep this bug.

::: browser/components/customizableui/test/browser_980155_add_overflow_toolbar.js
@@ +10,5 @@
> +add_task(function addOverflowingToolbar() {
> +  let originalWindowWidth = window.outerWidth;
> +
> +  let widgetIds = [];
> +  for (let i = 0; i < 100; i++) {

Unfortunately this kind of stuff (ie 100 widgets) is making the test cause warnings about how we're storing too much stuff in prefs. Maybe use the same trick I used in the other test to make do with like 5 nodes (and just give them a bigger min-width so they overflow) ?

@@ +25,5 @@
> +  isnot(toolbarNode.customizationTarget, null, "Toolbar should have customization target");
> +  isnot(toolbarNode.customizationTarget, toolbarNode, "Customization target should not be toolbar node");
> +
> +  let oldChildCount = toolbarNode.customizationTarget.childElementCount;
> +  let oldOverflowCount = toolbarNode.overflowable._list.childElementCount;

Mumble mumble internal properties mumble. Maybe rely on the test's createOverflowableToolbarWithPlacements internals and just doc.getElementById() the element you care about?

Comment 14

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

I addressed Gijs' feedback and pushed to fx-team.

https://hg.mozilla.org/integration/fx-team/rev/512f9b89f5cc

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/512f9b89f5cc

Comment 16

[:Gijs (he/him)]

Comment on attachment 8388042 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: overflowable toolbars don't behave deterministically; needed for bug 976792
Testing completed (on m-c, etc.): on m-c, has automated test
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f87901f237ee