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.

Oops, this wasn't supposed to be a meta bug.

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?

(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.

(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! :)

(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.

(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.

(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. ;)

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.

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

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.

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.

(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 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.

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.

* 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 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!

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.

Sorry I am not nearly as well-acquanited with that test as Kris, I have to defer to him...

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 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; ```

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. :-)

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

Backout by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e4c251c24cb0 Backed out 6 changesets for OSX permaleaks on a CLOSED TREE.

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...

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

Verified on Windows, Mac, and Ubuntu.