Add permanent/'sticky' area to overflow panel (behind a pref)

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Toolbars and Customization
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(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

(1 attachment)

(Assignee)

Description

a year ago
It should be possible to move items in/out of the overflow panel permanently. To do this, we need to (conditionally, based on the pref) add an area to CUI similar to the current panel area.

We'll need to add a separator between this area and the items that appear in the menu dynamically.

We will need to be careful to ensure the move to toolbar and/or remove context menus continue to work (though updating them to go to/from this area is bug 1354078).


This bug is blocked on making the panel use a panelmultiview in order for subviews to work correctly.
(Assignee)

Updated

a year ago
Blocks: 1354094
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon] → [photon-structure]
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
Note that this patch doesn't fix bug 1354078 (the context menus), bug 1354082 (showing the panel in customize mode) or bug 1354094 (showing the button if permanent items are in the panel).

Something else to bear in mind: there's basically 3 panels in play at the moment (the overflow panel, the old hamburger panel, and the new hamburger panel), and PanelUI.js and CustomizableUI relate to all 3, making for a somewhat confusing mix. In particular, at the moment items in the permanent part of the overflow menu get attributes for being in a panel, leading to different colour icons from the toolbar icons. I haven't updated this because it would affect the old hamburger panel while we're still shipping that. It should be straightforward to update all customizable buttons to always use the same images, whether in the toolbar or in the overflow panel, once 57 hits.

More generally, the 2 customizable area types of toolbar vs. panel map somewhat awkwardly onto the 3 different places we really have (toolbar, overflow, hamburger). This was already somewhat the case before but is now temporarily worse. I propose we don't worry about this too much (unless it's obvious there are serious difficulties) until we get closer to actually removing the old-style panel, mostly because, given CSS and add-on dependencies, a lot of the more radical solutions (e.g. blanket restyling/reusing the "panel" area for styling the overflow area, adjusting the attributes of items as they get added/removed from the overflow widget) aren't currently possible/plausible.
(Assignee)

Updated

a year ago
Attachment #8861182 - Flags: review?(mdeboer)
(Assignee)

Comment 3

a year ago
Eh, I swear this passed tests when I last ran them, but it definitely doesn't right now, so let's fix that before asking for review...
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
I missed the DragPositionManager.jsm dependency, which didn't like the additional area. Fixed now.

I also just realized we should probably not register the additional area in CustomizableUI unless the photon pref is flipped... I'll fix that, should be a small change.
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8861182 [details]
Bug 1354079 - add sticky / permanent part to overflow panel,

https://reviewboard.mozilla.org/r/133148/#review137642

Looks good! I haven't been able to try and actually move anything into it, of course, but this is bringing that quite a bit closer :)

::: browser/components/customizableui/CustomizableUI.jsm:237
(Diff revision 3)
>        type: CustomizableUI.TYPE_MENU_PANEL,
>        defaultPlacements: panelPlacements
>      }, true);
>      PanelWideWidgetTracker.init();
>  
> +    if (Services.prefs.getBoolPref("browser.photon.structure.enabled", false)) {

The pref is in firefox.js, so this default value is not needed. Doesn't hurt, but also doesn't serve a purpose.

::: browser/components/customizableui/CustomizableUI.jsm:2838
(Diff revision 3)
>     */
>    AREA_ADDONBAR: "addon-bar",
>    /**
> +   * Constant reference to the ID of the non-dymanic (fixed) list in the overflow panel.
> +   */
> +  AREA_FIXED_OVERFLOW_PANEL: "widget-overflow-fixed-list",

nit: how about 'widget-overflow-fixed-set' to stay in customization terms?

::: browser/components/customizableui/content/panelUI.js:42
(Diff revision 3)
>        panel: gPhotonStructure ? "appMenu-popup" : "PanelUI-popup",
>        notificationPanel: "PanelUI-notification-popup",
>        scroller: "PanelUI-contents-scroller",
> -      footer: "PanelUI-footer"
> +      footer: "PanelUI-footer",
> +
> +      overflowContents: gPhotonStructure ? "widget-overflow-fixed-list" : "",

nit: I'd mirror the internal property name a bit more with the ID you provide for the element.
Attachment #8861182 - Flags: review?(mdeboer) → review+
Iteration: 55.4 - May 1 → 55.5 - May 15
Comment hidden (mozreview-request)

Comment 9

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ecb2351ed40f
add sticky / permanent part to overflow panel, r=mikedeboer

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ecb2351ed40f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.