Update overflow panel styling

VERIFIED FIXED in Firefox 56
(NeedInfo from)

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
5 months ago
a day ago

People

(Reporter: Gijs, Assigned: Gijs, NeedInfo)

Tracking

(Blocks: 2 bugs)

53 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

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

Attachments

(8 attachments)

(Assignee)

Description

5 months ago
Created attachment 8855283 [details]
Menus spec

We should update the styling of items in the overflow panel. Styling should match that of other vertically-laid out panels in terms of size, font size, and margins/paddings. We should try as much as possible to share this styling across these panels and to reduce duplication that may have crept in as a result of Australis.

Updated

4 months ago
Flags: qe-verify-
Summary: Update overflow panel styling → [meta] Update overflow panel styling
(Assignee)

Comment 1

4 months ago
Oops, this wasn't supposed to be a meta bug.
Keywords: meta
Summary: [meta] Update overflow panel styling → Update overflow panel styling
(Assignee)

Updated

4 months ago
Flags: qe-verify-

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]

Comment 2

3 months ago
Created attachment 8867524 [details]
screenshot

Is the wrong look of the icons in the overflow panel part of this bug (please see the attached screenshot) or should I file a new bug?
(Assignee)

Comment 3

3 months ago
(In reply to Sören Hentzschel from comment #2)
> Created attachment 8867524 [details]
> screenshot
> 
> Is the wrong look of the icons in the overflow panel part of this bug
> (please see the attached screenshot) or should I file a new bug?

There are 3 items in that screenshot and they're all add-ons, so I'm not convinced this is even a Firefox bug specifically - the screenshots item, which is also an add-on, does look correct, so it seems more likely to be an issue with test pilot and the "check bookmarks" add-on (whatever that is).

But yes, there's way too much spacing here right now, the hover styling is wrong, etc. etc., and we'll likely need to make other changes in this bug, so I would hold off filing a separate bug either way.

Comment 4

3 months ago
(In reply to :Gijs from comment #3)
> There are 3 items in that screenshot and they're all add-ons, so I'm not
> convinced this is even a Firefox bug specifically - the screenshots item,
> which is also an add-on, does look correct, so it seems more likely to be an
> issue with test pilot and the "check bookmarks" add-on (whatever that is).

It's a new add-on from me (but not yet reviewed):
https://addons.mozilla.org/en-US/firefox/addon/bookmarks-organizer/ ;)

But it has to be a Firefox bug because that's the way how WebExtensions add icons to the toolbar:

"browser_action": {
  "browser_style": false,
  "default_icon": {
    "16": "images/icon-16.png",
    "32": "images/icon-32.png",
    "64": "images/icon-64.png"
  },
  "default_title": "__MSG_button_check_bookmarks__"
},

As there is nothing what WebExtensions can do differently this has to be fixed on the Firefox side. :P

> But yes, there's way too much spacing here right now, the hover styling is
> wrong, etc. etc., and we'll likely need to make other changes in this bug,
> so I would hold off filing a separate bug either way.

Okay! :)
(Assignee)

Comment 5

3 months ago
(In reply to Sören Hentzschel from comment #4)
> "browser_action": {
>   "browser_style": false,
>   "default_icon": {
>     "16": "images/icon-16.png",
>     "32": "images/icon-32.png",
>     "64": "images/icon-64.png"
>   },
>   "default_title": "__MSG_button_check_bookmarks__"
> },
> 
> As there is nothing what WebExtensions can do differently this has to be
> fixed on the Firefox side. :P

Well, the screenshots add-on is a webextension too. Can you work out why that one is working correctly? I would expect either both to be broken or both to be working... That might help narrow down what is going on here.

Also, just to be clear... is this happening in the normal overflow panel as well, even with the photon structure pref off? Because in that case, this might be a regression, and it might deserve a separate bug that tracks 55. We aren't necessarily going to fix this bug for 55.
Flags: needinfo?(cadeyrn)

Comment 6

3 months ago
(In reply to :Gijs from comment #5) 
> Well, the screenshots add-on is a webextension too. Can you work out why
> that one is working correctly? I would expect either both to be broken or
> both to be working... That might help narrow down what is going on here.

My add-on Bookmarks Organizer und Test Pilot (both with broken icon) use PNG icons. Snooze Tabs uses a SVG icon and looks good. I converted the icon of my add-on to a SVG file and with the SVG file instead of the PNG file it looks good, too.

> Also, just to be clear... is this happening in the normal overflow panel as
> well, even with the photon structure pref off?

No, it only happens with the photon structure enabled.
Flags: needinfo?(cadeyrn)

Comment 7

3 months ago
(In reply to Sören Hentzschel from comment #6)
> My add-on Bookmarks Organizer und Test Pilot (both with broken icon) use PNG
> icons. Snooze Tabs uses a SVG icon and looks good.
> 
> No, it only happens with the photon structure enabled.

Sorry, I meant Page Shot and not Snooze Tabs. I have to many Firefox instances open. ;)
(Assignee)

Updated

3 months ago
Depends on: 1367370
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1368854
(Assignee)

Updated

2 months ago
See Also: → bug 1373961
(Assignee)

Updated

2 months ago
Depends on: 1373969
Depends on: 1374636
(Assignee)

Updated

2 months ago
No longer depends on: 1374636
(Assignee)

Updated

2 months ago
Blocks: 1377913
(Assignee)

Comment 9

2 months ago
In order to do this, we need to switch the overflow panel to a <photonpanelmultiview>

The photon view uses an offscreen container to determine the size of subviews when inserting them. This (and maybe other things) breaks webextensions tests. I have a WIP patch to fix this, but it looks like this might be a long slog. Taking so we don't have multiple folks getting lost in these particular weeds.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.2 - Jul 10
Priority: P2 → P1
(Assignee)

Updated

a month ago
Duplicate of this bug: 1377913
(Assignee)

Updated

a month ago
No longer blocks: 1377913
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a month ago
I can't reproduce the browser_ext_browserAction_popup.js orange locally. Another trypush to see if this got fixed by the other panelmultiview fixes or something: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47b4a1d936277d9c7591f69c3741e2679807d801

Updated

a month ago
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24

Comment 16

a month ago
mozreview-review
Comment on attachment 8883943 [details]
Bug 1354086 - switch overflow panel to using a photonpanelmultiview, allowing webextension views to specify their own size,

https://reviewboard.mozilla.org/r/154916/#review161190

::: browser/components/customizableui/PanelMultiView.jsm:513
(Diff revision 1)
>            Cu.reportError(e);
>            cancel = true;
>          }
>        }
>  
> +      this._viewShowing = null;

Nice one! Subtle change, big difference.
Attachment #8883943 - Flags: review?(mdeboer) → review+

Comment 17

a month ago
mozreview-review
Comment on attachment 8883944 [details]
Bug 1354086 - always use toolbar icons in overflow panel,

https://reviewboard.mozilla.org/r/154918/#review161194

r=me with the left-overs removed.

::: browser/themes/shared/toolbarbutton-icons.inc.css:50
(Diff revision 1)
>  }
>  %endif
>  
> -#home-button[cui-areatype="toolbar"] {
> +%ifdef MOZ_PHOTON_THEME
> +%else
> +%endif

Erm, what does this do? I think you left this here accidentally.
Attachment #8883944 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 18

a month ago
(In reply to :Gijs from comment #15)
> I can't reproduce the browser_ext_browserAction_popup.js orange locally.
> Another trypush to see if this got fixed by the other panelmultiview fixes
> or something:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=47b4a1d936277d9c7591f69c3741e2679807d801

FWIW, this now looks like it just got fixed by the other panelmultiview changes, but the popup_resize thing looks like it's still broken. I'll have to look at what's causing that... seems like something is off-by-one somewhere.

Comment 19

a month ago
mozreview-review
Comment on attachment 8883945 [details]
Bug 1354086 - update styling of normal toolbar buttons in the overflow panel,

https://reviewboard.mozilla.org/r/154920/#review161196

Descendant selectors are unfortunate, but necessary for buttons. Oh well.
Attachment #8883945 - Flags: review?(mdeboer) → review+

Comment 20

a month ago
mozreview-review
Comment on attachment 8883946 [details]
Bug 1354086 - update styling of combined buttons in the overflow panel,

https://reviewboard.mozilla.org/r/154922/#review161198

This also looks very sane to me, but please wait with landing these patches until I visually verified your changes.
Attachment #8883946 - Flags: review?(mdeboer) → review+
* What I'm seeing is that the font looks awfully small, compared to the Hamburger panel menu and the Library panel. I think that applying the toolbarbutton rule containing `font: menu;` will do the trick.
* When you drag the combined controls (zoom and edit) into the overflow menu in cust. mode, you see them overflowing horizontally. They look excellent in the overflow panel.

I think these two issues need to be fixed prior to landing.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

a month ago
mozreview-review
Comment on attachment 8885333 [details]
Bug 1354086 - fix fonts in overflow menu, combined buttons overflowing in customize mode,

https://reviewboard.mozilla.org/r/156192/#review161344

LGTM! Nice work!
Attachment #8885333 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 28

a month ago
I would like to land this, but I can't reproduce the issues that are showing up on try locally, because locally (testing on my 15" retina mbp) I get other issues with the webextension tests in question.

Specifically, if I try to run:

./mach mochitest --tag remote-webextensions browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js

the test hangs immediately on opening, even without applying these patches. It seems from some local debugging that the awaitBrowserLoaded in awaitExtensionPanel called from openPanel in the test never resolves.

Andrew / Kris, how do I move forward here? Without being able to reproduce these issues, it's going to be very hard to work out how to fix them. This is the second time this test is basically holding up patches that shouldn't materially affect it, and I would love to make them more reliable if that is possible.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
Sorry I am not nearly as well-acquanited with that test as Kris, I have to defer to him...
Flags: needinfo?(aswan)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

a month ago
Blake pointed out that running things locally breaks because of sandboxing. Passing --setpref 'security.sandbox.content.level=2' to the mochitest command makes things work. I reproduced the issue and included a small fix in the test, which makes things work locally. Pushing to try again in a bit.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 57

a month ago
mozreview-review
Comment on attachment 8887941 [details]
Bug 1354086 - fix leaks caused by transitionend listeners that hang around,

https://reviewboard.mozilla.org/r/158836/#review164204

Thanks for keepin' on pushin'! I hope this'll make your patches here landable!

::: browser/components/customizableui/PanelMultiView.jsm:941
(Diff revision 1)
>          this.node.removeAttribute("panelopen");
>          this.showMainView();
>          if (this.panelViews) {
> +          if (this._transitionEndListener) {
> +            this._viewContainer.removeEventListener("transitionend", this._transitionEndListener);
> +            this._transitionEndListener = null;

AND set `this._transitioning = false;`. You can also do this in its own block:

```js
if (this._transitionEndListener) {
  this._viewContainer.removeEventListener("transitionend", this._transitionEndListener);
  this._transitionEndListener = null;
}
if (this._transitioning)
  this._transitioning = false;
```
Attachment #8887941 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 58

a month ago
mozreview-review
Comment on attachment 8887941 [details]
Bug 1354086 - fix leaks caused by transitionend listeners that hang around,

https://reviewboard.mozilla.org/r/158836/#review164214

::: browser/components/customizableui/PanelMultiView.jsm:941
(Diff revision 1)
>          this.node.removeAttribute("panelopen");
>          this.showMainView();
>          if (this.panelViews) {
> +          if (this._transitionEndListener) {
> +            this._viewContainer.removeEventListener("transitionend", this._transitionEndListener);
> +            this._transitionEndListener = null;

This already happens a few lines earlier. :-)

Comment 59

a month ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b074842173dc
switch overflow panel to using a photonpanelmultiview, allowing webextension views to specify their own size, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/979385d4c246
always use toolbar icons in overflow panel, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/d627b7b2dea5
update styling of normal toolbar buttons in the overflow panel, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/87fc17c32c51
update styling of combined buttons in the overflow panel, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/a370f8134644
fix fonts in overflow menu, combined buttons overflowing in customize mode, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/f90ff9792bbd
fix leaks caused by transitionend listeners that hang around, r=mikedeboer

Comment 60

a month ago
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e4c251c24cb0
Backed out 6 changesets for OSX permaleaks on a CLOSED TREE.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 67

a month ago
OK, this time, reran the entire leaky test dir rather than, after fixing 1 leak, assuming there weren't any others, and that came back clean on my local osx machine. Still confused why the leaks are not happening on any of the other platforms... anyway, second time lucky, hopefully...

Comment 68

a month ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5ca4811f7836
switch overflow panel to using a photonpanelmultiview, allowing webextension views to specify their own size, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/4a279604f7ba
always use toolbar icons in overflow panel, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/ae0195a7359a
update styling of normal toolbar buttons in the overflow panel, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/48e18b6a3c7c
update styling of combined buttons in the overflow panel, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/eadb66a18add
fix fonts in overflow menu, combined buttons overflowing in customize mode, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/0e770947be4b
fix leaks caused by transitionend listeners that hang around, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/5ca4811f7836
https://hg.mozilla.org/mozilla-central/rev/4a279604f7ba
https://hg.mozilla.org/mozilla-central/rev/ae0195a7359a
https://hg.mozilla.org/mozilla-central/rev/48e18b6a3c7c
https://hg.mozilla.org/mozilla-central/rev/eadb66a18add
https://hg.mozilla.org/mozilla-central/rev/0e770947be4b
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(Assignee)

Updated

29 days ago
Depends on: 1382918

Updated

28 days ago
Depends on: 1383076
Blocks: 1387512
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.