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

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
4 months ago
a month ago

People

(Reporter: Virtual, Assigned: dao)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {nightly-community, ux-consistency})

55 Branch
Firefox 57
x86_64
Windows 7
nightly-community, ux-consistency
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 disabled, firefox56 disabled, firefox57 verified)

Details

(Whiteboard: [reserve-photon-visual][p3])

MozReview Requests

()

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

Attachments

(3 attachments)

Created attachment 8882252 [details]
old hover effect.png
Created attachment 8882253 [details]
new hover effect.png
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Blocks: 1363842
(Assignee)

Updated

4 months ago
Whiteboard: [photon][triage] → [reserve-photon-visual][p3]
(Assignee)

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P3
Component: Theme → Toolbars and Customization
(Assignee)

Comment 2

4 months ago
This belongs in the Theme component.
Component: Toolbars and Customization → Theme
status-firefox55: --- → affected
status-firefox56: --- → affected
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
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
Version: Trunk → 55 Branch
QA Contact: Virtual
status-firefox55: affected → disabled
status-firefox56: affected → disabled
status-firefox57: --- → affected
(Assignee)

Updated

3 months ago
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

Updated

3 months ago
Iteration: --- → 57.1 - Aug 15
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Blocks: 1388676
(Assignee)

Updated

3 months ago
Blocks: 1363023

Comment 4

3 months 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

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

Comment 6

3 months ago
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)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Flags: needinfo?(dao+bmo)

Comment 9

3 months ago
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

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

3 months 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

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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/500a01cf896d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: affected → fixed
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
status-firefox57: fixed → verified
Blocks: 1368580
Depends on: 1389755

Updated

2 months ago
Flags: qe-verify+
Depends on: 1389921
Blocks: 1384686

Updated

2 months ago
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
No longer depends on: 1389755
Depends on: 1391017
Depends on: 1392978
Depends on: 1392687

Updated

2 months ago
Depends on: 1396325
No longer depends on: 1396325

Updated

a month ago
Depends on: 1401823
You need to log in before you can comment on or make changes to this bug.