Closed
Bug 1354079
Opened 8 years ago
Closed 8 years ago
Add permanent/'sticky' area to overflow panel (behind a pref)
Categories
(Firefox :: Toolbars and Customization, enhancement, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: Gijs, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-structure])
Attachments
(1 file)
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.
Updated•8 years ago
|
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon] → [photon-structure]
Updated•8 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years 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•8 years ago
|
Attachment #8861182 -
Flags: review?(mdeboer)
Assignee | ||
Comment 3•8 years 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•8 years 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•8 years 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+
Updated•8 years ago
|
Iteration: 55.4 - May 1 → 55.5 - May 15
Comment hidden (mozreview-request) |
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ecb2351ed40f
add sticky / permanent part to overflow panel, r=mikedeboer
Comment 10•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years 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.
Description
•