Closed Bug 1354086 Opened 7 years ago Closed 7 years ago

Update overflow panel styling

Categories

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

Product:
Firefox
Component:
Toolbars and Customization
Version:
53 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Iteration:
56.3 - Jul 24
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(8 files)

Menus spec
1.60 MB, image/png
Details
screenshot
122.39 KB, image/png
Details
Bug 1354086 - switch overflow panel to using a photonpanelmultiview, allowing webextension views to specify their own size,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1354086 - always use toolbar icons in overflow panel,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1354086 - update styling of normal toolbar buttons in the overflow panel,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1354086 - update styling of combined buttons in the overflow panel,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1354086 - fix fonts in overflow menu, combined buttons overflowing in customize mode,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1354086 - fix leaks caused by transitionend listeners that hang around,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Attached image 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.
Flags: qe-verify-
Summary: Update overflow panel styling → [meta] Update overflow panel styling
Oops, this wasn't supposed to be a meta bug.
Keywords: meta
Summary: [meta] Update overflow panel styling → Update overflow panel styling
Flags: qe-verify-
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Attached image 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?
(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.
Flags: needinfo?(cadeyrn)
(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)
(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. ;)
Depends on: 1367370
See Also: → 1373961
Depends on: 1373969
No longer depends on: 1374636
Blocks: 1377913
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
No longer blocks: 1377913
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
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
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 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+
(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.
Attachment #8883945 - Flags: review?(mdeboer) → 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 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+
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)
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;
```
Attachment #8887941 - Flags: review?(mdeboer) → 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. :-)
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
Depends on: 1382918
Blocks: 1387512
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
Flags: qe-verify+
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: