Closed Bug 1354094 Opened 3 years ago Closed 2 years ago

Update code to show/hide overflow panel to take into account whether the user has put permanent items into it

Categories

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

53 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.5 - May 15
Tracking Status
firefox55 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

Once we have a permanent/sticky part to the overflow panel, we'll need to ensure we always show the overflow button if there are items in that part of the overflow panel.
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Comment on attachment 8866411 [details]
Bug 1354094 - show the overflow button when there are items in the permanent overflow menu,

https://reviewboard.mozilla.org/r/138044/#review141196

With the comments below fixed, it'll be great - but I'd still like to confirm it all and try the patch locally as well.

::: browser/components/customizableui/CustomizableUI.jsm
(Diff revision 1)
>          child.setAttribute("overflowedItem", true);
>          child.setAttribute("cui-anchorid", this._chevron.id);
>          CustomizableUIInternal.notifyListeners("onWidgetOverflow", child, this._target);
>  
>          this._list.insertBefore(child, this._list.firstChild);
> -        if (!this._toolbar.hasAttribute("overflowing")) {

Please take a look at the hg blame, because I've got the feeling that this was put here intentionally.

::: browser/components/customizableui/CustomizableUI.jsm:4277
(Diff revision 1)
>      let win = this._target.ownerGlobal;
>      win.UpdateUrlbarSearchSplitterState();
>  
>      if (!this._collapsed.size) {
>        this._toolbar.removeAttribute("overflowing");
> -      CustomizableUI.removeListener(this);
> +      this._panel.setAttribute("nooverfloweditems", "true");

Same here, really. Of course, setting the attribute should stay.

::: browser/components/customizableui/CustomizableUI.jsm:4367
(Diff revision 1)
>        aNode.removeAttribute("overflowedItem");
>        CustomizableUIInternal.notifyListeners("onWidgetUnderflow", aNode, this._target);
>  
>        if (!this._collapsed.size) {
>          this._toolbar.removeAttribute("overflowing");
> -        CustomizableUI.removeListener(this);
> +        this._panel.setAttribute("nooverfloweditems", "true");

Same.

::: browser/components/customizableui/content/panelUI.inc.xul:378
(Diff revision 1)
>  <panel id="widget-overflow"
>         role="group"
>         type="arrow"
>         noautofocus="true"
>         position="bottomcenter topright"
> +       nooverfloweditems="true"

Can we use a positive-named attribute name and have the default of 'not present' be reflective of the initial state?

So I'd have opted for 'containsoverfloweditems' or 'hasoverfloweditems' or something. Perhaps even include the dynamic property of these items in there: 'hasdynamicoverfloweditems'...

::: browser/components/customizableui/content/panelUI.js:89
(Diff revision 1)
>    _initPhotonPanel() {
>      if (gPhotonStructure) {
>        this.overflowFixedList.hidden = false;
>        this.overflowFixedList.nextSibling.hidden = false;
>        CustomizableUI.registerMenuPanel(this.overflowFixedList, CustomizableUI.AREA_FIXED_OVERFLOW_PANEL);
> +      this.navbar.setAttribute("photon-structure", "true");

It doesn't seem you're using this attribute in this patch. Can you remove it?

::: browser/components/customizableui/content/panelUI.js:605
(Diff revision 1)
>    onPhotonChanged() {
>      this.reinit();
>    },
>  
> +  updateOverflowStatus() {
> +    let haveKids = this.overflowFixedList.hasChildNodes();

nit: s/haveKids/hasKids/

::: browser/components/customizableui/content/panelUI.js:607
(Diff revision 1)
>    },
>  
> +  updateOverflowStatus() {
> +    let haveKids = this.overflowFixedList.hasChildNodes();
> +    if (haveKids && !this.navbar.hasAttribute("nonemptyoverflow")) {
> +      this.navbar.setAttribute("nonemptyoverflow", "true");

nit: `true.toString() == "true"`, so I usually use the bool variant.
Attachment #8866411 - Flags: review?(mdeboer) → review-
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8866411 [details]
Bug 1354094 - show the overflow button when there are items in the permanent overflow menu,

https://reviewboard.mozilla.org/r/138044/#review141196

> Can we use a positive-named attribute name and have the default of 'not present' be reflective of the initial state?
> 
> So I'd have opted for 'containsoverfloweditems' or 'hasoverfloweditems' or something. Perhaps even include the dynamic property of these items in there: 'hasdynamicoverfloweditems'...

done, using 'hasdynamicitems' because we put it on the 'widget-overflow' panel so the CSS rule is clear like this.

> It doesn't seem you're using this attribute in this patch. Can you remove it?

It's used in the browser.css change. :-)
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Comment on attachment 8866411 [details]
Bug 1354094 - show the overflow button when there are items in the permanent overflow menu,

https://reviewboard.mozilla.org/r/138044/#review141328

Looking great, ship it!
Attachment #8866411 - Flags: review?(mdeboer) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0645deeb4caa
show the overflow button when there are items in the permanent overflow menu, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/0645deeb4caa
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
No longer depends on: 1364477
Depends on: 1366079
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1373972
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.