Closed Bug 1354071 Opened 4 years ago Closed 4 years ago

Switch overflow panel to using a panelmultiview

Categories

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

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.4 - May 1
Tracking Status
firefox55 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(3 files)

+++ 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.
Blocks: 1354079
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon] → [photon-structure]
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1357405
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.
Iteration: --- → 55.4 - May 1
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?
Attachment #8859190 - Flags: review?(mdeboer) → review+
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 on attachment 8859616 [details]
Bug 1354071 - make sure context menu continues to work correctly,

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

LGTM!
Attachment #8859616 - Flags: review?(mdeboer) → review+
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.
Attachment #8859617 - Flags: review?(mdeboer) → review+
Pushed by gijskruitbosch@gmail.com:
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
Depends on: 1368255
Depends on: 1370986
You need to log in before you can comment on or make changes to this bug.