Switch overflow panel to using a panelmultiview

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Toolbars and Customization
P1
normal
RESOLVED FIXED
2 months ago
2 days ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

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

53 Branch
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 months ago
+++ 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.
(Assignee)

Updated

2 months ago
Blocks: 1354079

Updated

2 months ago
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon] → [photon-structure]
(Assignee)

Updated

2 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

Updated

2 months ago
Priority: P2 → P1
(Assignee)

Updated

a month ago
Depends on: 1357405
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a month ago
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.

Updated

a month ago
Iteration: --- → 55.4 - May 1

Comment 3

a month ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a month ago
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

a month ago
mozreview-review
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 8

a month ago
mozreview-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+
Comment hidden (mozreview-request)

Comment 10

a month ago
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
https://hg.mozilla.org/mozilla-central/rev/e3a384a8f7d7
https://hg.mozilla.org/mozilla-central/rev/900f2feda12a
https://hg.mozilla.org/mozilla-central/rev/139362080b76
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

2 days ago
Depends on: 1368255
You need to log in before you can comment on or make changes to this bug.