976792 - CustomizableUI's insertNodeInWindow should go through placements and try followup next nodes

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=951612#c31 for background.

Comment 1

[:Gijs (he/him)]

Blair, how is this going and/or can I help? :-)

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Main part of the patch is done, just gotta put time aside to expand the test coverage - because in fixing this, I discovered a few extra bugs in the old code. Bit surprised none of us noticed this sooner :\

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Fixing old bugs and writing additional tests sounds like a task for a followup non-Australis:P2 bug?

Comment 4

[:Gijs (he/him)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #3)
> Fixing old bugs and writing additional tests sounds like a task for a
> followup non-Australis:P2 bug?

I suspect 'old' isn't as 'old' as you think. :-)

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, no, I meant "old" as in the existing new-ish code we rely on that I had to re-write that code to fix this bug anyway. I wanted tests to properly cover the new code, covering the classes of issues I found in the code I rewrote.

Though, turns out even that involved yak shaving with finding more bugs. Trying to work around that now, and put it into followup bugs. Though I think we'll want a P2 out of that :\

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

So, update: Still banging my head against the desk on this. Tests are... not great, but good enough (bug 980155 got in the way plenty). But I'm fighting issues around overflow toolbars, which the old code handled blatantly wrong. Now I've fixed that I have a test failing - but that's good, because it was accidentally passing before, and it's actually indirectly testing something bug 980155 was preventing me from testing (overflowable toolbars and non-overflowable widgets).

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

Gijs, maybe you could share some brainpower. I'm stumped on finding an algorithm that works for finding the right place for a node in an overflowing toolbar, and widgets that disallow overflowing.

ie, placements looks something like:
  exists-1,exists-2,overflows-1,trying-to-insert-this,can't-overflow-1,overflows-2

So the actual DOM in the toolbar looks like:
  exists-1,exists-2,can't-overflow-1
And in the overflow list:
  overflows-1,overflows-2



And then there's the variation of having multiple widgets that don't allow overflowing, and and cases where we have widgets in various positions the placements but don't exist in the DOM.

Comment 8

[:Gijs (he/him)]

Attachment: CustomizableUI's insertNodeInWindow should go through placements and try followup next nodes

I think this is correct. Blair, can you verify? All the tests you added so far are passing. I'll look at the other patch you uploaded next.

Comment 9

[:Gijs (he/him)]

Bad bzexport... :-(

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8387640 [details] [diff] [review]
CustomizableUI's insertNodeInWindow should go through placements and try followup next nodes

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

r+ on your modifications (thankyou!), they work beautifully.

New patch coming up with more thorough test coverage.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8388043 [details] [diff] [review]
Patch v2

Oh, I should note: This applies *on top of* bug 980155 (previously it was the other way around). Which let me expand the test suite such that I'm finally happy with the test coverage.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8388043 [details] [diff] [review]
Patch v2

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

r=me, but same comment as on the other test... it'd be nice if we reduced the number of widgets these tests used, and made their width more deterministic (so we can guarantee that it works on all platforms).

Also... IME these resizing+overflow tests are funky on Linux, and I've disabled all the other ones. For aurora uplift at least, I think we should do the same here, although I'm somewhat hopeful that my fixes to the tests that open new windows (to wait longer for the new windows to have completely closed) will have helped.

Generally, the code should work the same cross-platform and where it doesn't, that's likely as not a core problem, so I think the loss of test coverage on one platform wouldn't be terrible.

::: browser/components/customizableui/test/browser_976792_insertNodeInWindow.js
@@ +53,5 @@
> +add_task(function() {
> +  let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +
> +  let widgetIds = [];
> +  for (let i = 0; i < 20; i++) {

Here, too, it would be swell if we could decrease the number of widgets we're using.

@@ +98,5 @@
> +add_task(function() {
> +  let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +
> +  let widgetIds = [];
> +  for (let i = 0; i < 20; i++) {

And here.

@@ +144,5 @@
> +add_task(function() {
> +  let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +
> +  let widgetIds = [];
> +  for (let i = 0; i < 20; i++) {

And here.

@@ +191,5 @@
> +add_task(function() {
> +  let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +
> +  let widgetIds = [];
> +  for (let i = 30; i >= 0; i--) {

And definitely here. :-)

@@ +199,5 @@
> +    CustomizableUI.createWidget(spec);
> +    CustomizableUI.addWidgetToArea(id, "nav-bar", 0);
> +  }
> +
> +  for (let i = 40; i < 50; i++) {

I think you get the idea now... :-)

Comment 14

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

I reduced the number of widgets in the test and pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/8f6765a92092

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8f6765a92092

Comment 16

[:Gijs (he/him)]

Comment on attachment 8388043 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: insertion of nodes is unpredictable when using the provided APIs
Testing completed (on m-c, etc.): on m-c, has automated test
Risk to taking this patch (and alternatives if risky): low due to automated test
String or IDL/UUID changes made by this patch: none

Comment 17

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

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