Closed Bug 1377184 Opened 7 years ago Closed 7 years ago

Use new Photon hover effect also on "Show more bookmarks" and extensions buttons icons in Bookmarks Toolbar

Categories

(Firefox :: Theme, defect, P1)

55 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- disabled
firefox56 --- disabled
firefox57 --- verified

People

(Reporter: Virtual, Assigned: dao)

References

Details

(Keywords: nightly-community, ux-consistency, Whiteboard: [reserve-photon-visual][p3])

Attachments

(3 files)

Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Whiteboard: [photon][triage] → [reserve-photon-visual][p3]
Flags: qe-verify+
Priority: -- → P3
This belongs in the Theme component.
Component: Toolbars and Customization → Theme
Summary: Use new hover effect also on extensions buttons icons in Bookmarks Toolbar → Use new Photon hover effect also on extensions buttons icons in Bookmarks Toolbar
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Priority: P3 → P1
Summary: Use new Photon hover effect also on extensions buttons icons in Bookmarks Toolbar → Use new Photon hover effect also on "Show more bookmarks" and extensions buttons icons in Bookmarks Toolbar
Iteration: --- → 57.1 - Aug 15
Blocks: 1388676
Blocks: 1363023
Comment on attachment 8895278 [details] Bug 1377184 - Consistently use the custom toolbar button styling in all browser toolbars. https://reviewboard.mozilla.org/r/166462/#review171714 ::: browser/themes/shared/toolbarbuttons.inc.css:91 (Diff revision 1) > .findbar-button { > -moz-appearance: none; > padding: 0; > } > > -#nav-bar .toolbarbutton-1 { > +toolbar .toolbarbutton-1 { I wonder why we need the toolbar selector at all here, are there toolbarbutton-1 elements that are not in a toolbar?
Attachment #8895278 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #4) > > -#nav-bar .toolbarbutton-1 { > > +toolbar .toolbarbutton-1 { > > I wonder why we need the toolbar selector at all here, are there > toolbarbutton-1 elements that are not in a toolbar? In the overflow panel and the customization palette.
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c4376773f9cd Consistently use the custom toolbar button styling in all browser toolbars. r=johannh
Backed out for failing browser-chrome's browser_startup_images.js: chevron.svg should have been shown: https://hg.mozilla.org/integration/autoland/rev/aa5e52bbd13b6c60d1f478c6ea9504e721b5b6c6 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c4376773f9cd977c8c450f71ae33e937ed4fd026&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122037618&repo=autoland [task 2017-08-09T17:11:50.494825Z] 17:11:50 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_images.js | Loaded image chrome://browser/skin/chevron.svg should have been shown. - [task 2017-08-09T17:11:50.497645Z] 17:11:50 INFO - Stack trace: [task 2017-08-09T17:11:50.502813Z] 17:11:50 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/browser_startup_images.js:null:126
Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/500a01cf896d Consistently use the custom toolbar button styling in all browser toolbars. r=johannh
If this bug changed a ">>" button for Bookmarks toolbar to be consistent with other buttons and toolbars, then it was probably not a best idea. The button makes a Bookmarks toolbar much higher/bigger in all density options and then consequently the bar is shrinking and expanding depending on this button. https://s27.postimg.org/4q68ze8bn/bookmarks.png So with the new button styling the whole Bookmarks bar height is way beyond photon specs. I think this needs more clarification, because obviously there is no room for both ways.
I'm not seeing any differences in "Show more bookmarks" button sizes at all Densities (Compact, Normal and Touch) in Bookmark Toolbar. "Show more bookmarks" button size is always 14 x 24 px, at least in Mozilla Firefox Nightly 57.0a1 (2017-08-10). Maybe you are thinking about other ">>" button, which is "More tools" button in URL/Address/Location Bar.
Flags: needinfo?(Eddwardiq)
Yes, because patch is not in Nightly 57.0a1 (2017-08-10) yet. I tried a build from autoland branch. Well, the thing is that the ">>" "More tools" button in URL bar and ">>" "Show more bookmarks" button in Bookmarks bar are now exactly the same and it causes issues with Bookmarks bar height. I see only two options... leave it as it was before for Bookmarks bar or implement different "photon" button for this bar.
Flags: needinfo?(Eddwardiq)
It is not even that easy since on the Bookmarks toolbar you can also put icons from Customization panel. In the latest Nightly, the buttons are shrinking to the size of Bookmarks bar. And here, the Bookmarks bar is expanding to the size of buttons. So, in this case a different "photon" button will not help. I think Bookmarks bar just needs to be locked to the intended size, so every added button will shrink accordingly as it was before this patch, but I'm not sure if it is not against the idea of this bug.
All buttons/items/etc. should use the same new Photon hover effect, which is this bug about, not buttons sizes. But, if there will be some regression, like the size of "Show more bookmarks" will get massively huge compared to before, that should be fixed too. So your second option will probably be the best choice for all. I will monitor this upcoming issue, please do the same. :)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Thank you very much \o/. I'm confirming that bug is fixed, starting in Mozilla Firefox 57.0a1 (2017-08-11), so I'm marking this bug as VERIFIED.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1389966
when this landed we see a performance improvement in the tab animation test: == Change summary for alert #8694 (as of August 09 2017 18:52 UTC) == Improvements: 3% tart summary windows7-32 opt e10s 6.96 -> 6.78 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8694
Depends on: 1391017
Depends on: 1392687
Depends on: 1396325
Depends on: 1401823
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: