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)
Tracking
()
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)
No description provided.
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Blocks: 1363842
Assignee | ||
Updated•7 years ago
|
Whiteboard: [photon][triage] → [reserve-photon-visual][p3]
Assignee | ||
Updated•7 years ago
|
Flags: qe-verify+
Priority: -- → P3
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Component: Theme → Toolbars and Customization
Assignee | ||
Comment 2•7 years ago
|
||
This belongs in the Theme component.
Component: Toolbars and Customization → Theme
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
status-firefox55:
--- → affected
status-firefox56:
--- → affected
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
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
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
QA Contact: Virtual
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Priority: P3 → P1
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
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
Updated•7 years ago
|
Iteration: --- → 57.1 - Aug 15
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 5•7 years ago
|
||
(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
Comment 7•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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
Comment 10•7 years ago
|
||
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.
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
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)
Comment 13•7 years ago
|
||
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.
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Comment 14•7 years ago
|
||
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. :)
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Comment 16•7 years ago
|
||
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
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Blocks: 1368580
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Depends on: 1389755
Updated•7 years ago
|
Flags: qe-verify+
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Depends on: 1389921
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Blocks: 1384686
Comment 17•7 years ago
|
||
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
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
No longer depends on: 1389755
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Depends on: 1392978
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
No longer depends on: 1396325
You need to log in
before you can comment on or make changes to this bug.
Description
•