1354071 - Switch overflow panel to using a panelmultiview

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1352692 +++

The overflow panel should probably use a panelmultiview to allow buttons that have subviews to work the way they currently do in the main ("hamburger") panel.

This could potentially be implemented without any additional prefs, though we'll need to be careful about styling.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1354071 - use a panelmultiview for the overflow panel,

Review commit: https://reviewboard.mozilla.org/r/131242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131242/

Comment 2

[:Gijs (he/him)]

The patch here requires the fix from bug 1357405 in order for the context menu to continue to work properly, and for the automated CUI tests to pass.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8859190 [details]
Bug 1354071 - use a panelmultiview for the overflow panel,

https://reviewboard.mozilla.org/r/131242/#review134256

LGTM. Looking forward to seeing a test as well. Or is the current coverage enough and we add tests for the overflow panel in the upcoming bugs?

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1354071 - make sure context menu continues to work correctly,

Review commit: https://reviewboard.mozilla.org/r/131624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131624/

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1354071 - test that using subviews in the overflow panel works, and that non-subview panels also work,

Review commit: https://reviewboard.mozilla.org/r/131626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131626/

Comment 6

[:Gijs (he/him)]

Oh, one more thing... I didn't implement anchor styling yet. So no blue blob and fadeout and arrow styling yet. I don't know if I should worry about this given bug 1353360. If we land this change for 55 we should either fix this to look 'right' for the status quo, or we should ensure we also fix 1353360 on 55.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8859616 [details]
Bug 1354071 - make sure context menu continues to work correctly,

https://reviewboard.mozilla.org/r/131624/#review134402

LGTM!

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8859617 [details]
Bug 1354071 - test that using subviews in the overflow panel works, and that non-subview panels also work,

https://reviewboard.mozilla.org/r/131626/#review134412

Also looking good here - thanks for the test!

r=me with comments addressed.

::: browser/components/customizableui/test/browser_overflow_use_subviews.js:3
(Diff revision 1)
> +"use strict";
> +
> +let overflowPanel = document.getElementById("widget-overflow");

nit: `const kOverflowPanel` or `const OVERFLOW_PANEL`

::: browser/components/customizableui/test/browser_overflow_use_subviews.js:5
(Diff revision 1)
> +"use strict";
> +
> +let overflowPanel = document.getElementById("widget-overflow");
> +
> +const isOSX = (Services.appinfo.OS === "Darwin");

Unused.

::: browser/components/customizableui/test/browser_overflow_use_subviews.js:7
(Diff revision 1)
> +
> +let overflowPanel = document.getElementById("widget-overflow");
> +
> +const isOSX = (Services.appinfo.OS === "Darwin");
> +
> +let originalWindowWidth;

nit: `var gOriginalWindowWidth`

::: browser/components/customizableui/test/browser_overflow_use_subviews.js:19
(Diff revision 1)
> +/**
> + * This checks that subview-compatible items show up as subviews rather than
> + * re-anchored panels. If we ever remove the character encoding widget, please
> + * replace this test with another subview - don't remove it.
> + */
> +add_task(function* check_character_encoding_subview_in_overflow() {

nit: please make these `async function` and s/yield/await/ inside them.

::: browser/components/customizableui/test/browser_overflow_use_subviews.js:44
(Diff revision 1)
> +  button.click();
> +  yield subviewShownPromise;
> +  is(characterEncodingView.closest("panel"), overflowPanel, "Should be inside the panel");
> +  overflowPanel.hidePopup();
> +  yield Promise.resolve(); // wait for popup to hide fully.
> +  

nit: superfluous whitespace.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8859617 [details]
Bug 1354071 - test that using subviews in the overflow panel works, and that non-subview panels also work,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131626/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3a384a8f7d7
use a panelmultiview for the overflow panel, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/900f2feda12a
make sure context menu continues to work correctly, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/139362080b76
test that using subviews in the overflow panel works, and that non-subview panels also work, r=mikedeboer

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/e3a384a8f7d7
https://hg.mozilla.org/mozilla-central/rev/900f2feda12a
https://hg.mozilla.org/mozilla-central/rev/139362080b76