Closed Bug 1352364 Opened 3 years ago Closed 3 years ago

Share toolbar button styling across platforms

Categories

(Firefox :: Theme, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.5 - May 15
Tracking Status
firefox55 --- fixed

People

(Reporter: dao, Assigned: johannh)

References

Details

(Whiteboard: [photon-visual][p1])

Attachments

(2 files, 1 obsolete file)

Currently, we have three different implementations for our custom toolbar button appearance. We should take the most mature one (the one for Windows) and share it across platforms. We can use CSS variables to account for platform differences.
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [photon] → [photon-visual]
Blocks: 1352358
Flags: qe-verify-
Whiteboard: [photon-visual] → [photon-visual][p1]
This patch should fix all major visual regressions that I noticed, so I put it up for review.

I'll do two try pushes now, one running tests and one for mozscreenshots. There might also be more unused image files, I want to see if the try push detects them.
Oh, I forgot: this patch is not split up because we talked about it and dao mentioned it might not be necessary for him. Let me know if you change your mind and I'll try to make it more granular.
Iteration: --- → 55.4 - May 1
Comment on attachment 8859042 [details]
Bug 1352364 - Share toolbar button styling code between platforms.

https://reviewboard.mozilla.org/r/131062/#review133806

Please share this code in a new file, e.g. toolbarbuttons.inc.css (rename the original toolbarbuttons.inc.css to toolbarbutton-icons.inc.css), and use hg copy with windows/browser.css to create that copy. This should (1) make it easier to review this and (2) preserve annotations.
Comment on attachment 8859042 [details]
Bug 1352364 - Share toolbar button styling code between platforms.

https://reviewboard.mozilla.org/r/131062/#review133808
Attachment #8859042 - Flags: review?(dao+bmo)
Can you also enable browser_backButtonFitts.js on Mac?
Comment on attachment 8859042 [details]
Bug 1352364 - Share toolbar button styling code between platforms.

https://reviewboard.mozilla.org/r/131062/#review134356

toolbarbuttons.inc.css isn't recognized as a copy of browser.css here. Might have to rename the toolbarbuttons.inc.css in a separate first patch, or find another name to avoid confusing Mercurial.

::: browser/themes/osx/compacttheme.css:9
(Diff revision 4)
>  
>  %include ../shared/compacttheme.inc.css
>  
>  :root {
>    --forwardbutton-width: 32px;
> +  --forwardbutton-height: 24px;

Is this still needed or can we use auto like on Windows and Linux?
Attachment #8859042 - Flags: review?(dao+bmo)
I added a new temporary commit in here to make reviewing the new toolbarbuttons.inc.css easier. I talked to Dao about it and the best thing might be to land renaming toolbarbuttons.inc.css in a separate bug. I'll remove the temp commit once that lands.
Comment on attachment 8859042 [details]
Bug 1352364 - Share toolbar button styling code between platforms.

waiting for a more readable patch based on bug 1358083
Attachment #8859042 - Flags: review?(dao+bmo)
Comment on attachment 8859042 [details]
Bug 1352364 - Share toolbar button styling code between platforms.

https://reviewboard.mozilla.org/r/131062/#review134356

> Is this still needed or can we use auto like on Windows and Linux?

Turns out I could get rid of these variables entirely. :)
Comment on attachment 8859042 [details]
Bug 1352364 - Share toolbar button styling code between platforms.

https://reviewboard.mozilla.org/r/131062/#review135246

::: browser/themes/shared/toolbarbuttons.inc.css
(Diff revision 7)
> -  background-clip: padding-box !important;
> -  border: 1px solid transparent;
> -  border-radius: 1px;
> -  transition-property: background-color, border-color, box-shadow;
> -  transition-duration: 150ms;
> -}

Why did you move this?

::: browser/themes/shared/toolbarbuttons.inc.css:314
(Diff revision 7)
>    border-top-left-radius: 0;
>    border-bottom-left-radius: 0;
>  }
>  
>  #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
> -  border-inline-end-style: none;
> +  border-inline-end: none !important;

Why this change?

::: toolkit/themes/osx/global/findBar.css
(Diff revision 7)
> -.findbar-find-previous,
> +.findbar-find-previous {
> -.findbar-highlight,
> -.findbar-case-sensitive,
> -.findbar-entire-word {
>    -moz-appearance: none;
> -  border-radius: 10000px;

This stuff looks like it will still be needed for findbars outside of browser.xul.

::: toolkit/themes/shared/jar.inc.mn:33
(Diff revision 7)
>    skin/classic/global/scale.css                            (../../shared/scale.css)
>    skin/classic/global/icons/calendar-arrows.svg            (../../shared/icons/calendar-arrows.svg)
> +  skin/classic/global/icons/chevron.png                    (../../shared/icons/chevron.png)
> +  skin/classic/global/icons/chevron-inverted.png           (../../shared/icons/chevron-inverted.png)
> +  skin/classic/global/icons/chevron@2x.png                 (../../shared/icons/chevron@2x.png)
> +  skin/classic/global/icons/chevron-inverted@2x.png        (../../shared/icons/chevron-inverted@2x.png)

Please undo this since chevron.png was designed for Mac.

::: toolkit/themes/shared/jar.inc.mn:35
(Diff revision 7)
> +  skin/classic/global/icons/chevron.png                    (../../shared/icons/chevron.png)
> +  skin/classic/global/icons/chevron-inverted.png           (../../shared/icons/chevron-inverted.png)
> +  skin/classic/global/icons/chevron@2x.png                 (../../shared/icons/chevron@2x.png)
> +  skin/classic/global/icons/chevron-inverted@2x.png        (../../shared/icons/chevron-inverted@2x.png)
>    skin/classic/global/icons/find-arrows.svg                (../../shared/icons/find-arrows.svg)
> +  skin/classic/global/icons/folder-item.png                (../../shared/icons/folder-item.png)

Ditto. This icon was designed for Windows.

I'd prefer if you undid all the bookmark related changes as they seem only tangentially related to this bug and make the patch unnecessarily complicated.

::: toolkit/themes/windows/global/toolbarbutton.css
(Diff revision 7)
>  }
>  
> -.toolbarbutton-icon[label]:not([label=""]),
> -.toolbarbutton-icon[type="menu"] {
> -  margin-inline-end: 5px;
> -}

Why did you remove this? Looks like this would still be useful, if not for our main toolbars then at least for others.
Attachment #8859042 - Flags: review?(dao+bmo) → review-
Comment on attachment 8859042 [details]
Bug 1352364 - Share toolbar button styling code between platforms.

https://reviewboard.mozilla.org/r/131062/#review135246

> Why did you move this?

Pretty sure that was on accident. I'll fix it.

> Why this change?

Hmm, the border would not properly disappear without this change, but it seems like later changes got rid of the conflict. I'll test on all platforms and revert this if possible.

> This stuff looks like it will still be needed for findbars outside of browser.xul.

But it conflicts with the shared findbar styling. What's the best way to solve this then? Reset these properties in browser.inc.css?

> Please undo this since chevron.png was designed for Mac.

Ok. :) I'll probably need to share this value in a CSS variable then.

> Ditto. This icon was designed for Windows.
> 
> I'd prefer if you undid all the bookmark related changes as they seem only tangentially related to this bug and make the patch unnecessarily complicated.

Oh, it was my understanding that bookmarks were part of the things we wanted to share (and they're technically toolbar buttons, no?). I don't mind reverting the change, but we might want to ask UX what the plans for the bookmarks toolbar are first. In case we do need to touch the bookmark buttons, I can also make either a follow-up bug or a second patch for these changes if you prefer that for reviewing.

In my personal opinion, not changing the bookmark toolbar at all would look weird. :)

> Why did you remove this? Looks like this would still be useful, if not for our main toolbars then at least for others.

IIRC it added margin to the back and forward button, but I just noticed it's actually shadowed by a rule in browser.css. Weird. I'll put it in again.
(In reply to Johann Hofmann [:johannh] from comment #21)
> > This stuff looks like it will still be needed for findbars outside of browser.xul.
> 
> But it conflicts with the shared findbar styling. What's the best way to
> solve this then? Reset these properties in browser.inc.css?

I'd keep it in osx/browser.css before including toolbarbuttons.css. Looks like this rule is currently taking care of this:

.findbar-button {
  background: none;
  box-shadow: none;
}

> > Please undo this since chevron.png was designed for Mac.
> 
> Ok. :) I'll probably need to share this value in a CSS variable then.

It's a bookmarks toolbar specific thing. You can just leave it in browser.css.

> > Ditto. This icon was designed for Windows.
> > 
> > I'd prefer if you undid all the bookmark related changes as they seem only tangentially related to this bug and make the patch unnecessarily complicated.
> 
> Oh, it was my understanding that bookmarks were part of the things we wanted
> to share (and they're technically toolbar buttons, no?).

I'd rather share this stuff in a file dedicated to the bookmarks toolbar if and when there's a need for that. It seems unnecessary for photon.

> I don't mind
> reverting the change, but we might want to ask UX what the plans for the
> bookmarks toolbar are first. In case we do need to touch the bookmark
> buttons, I can also make either a follow-up bug or a second patch for these
> changes if you prefer that for reviewing.
> 
> In my personal opinion, not changing the bookmark toolbar at all would look
> weird. :)

The styling would update automatically as far as it's using the same CSS variables.
There are some test failures from missing files, I'll look into it.
Comment on attachment 8859042 [details]
Bug 1352364 - Share toolbar button styling code between platforms.

https://reviewboard.mozilla.org/r/131062/#review136108

::: browser/themes/linux/browser.css
(Diff revision 8)
>    margin-inline-end: 4px;
>  }
>  
> -toolbarbutton.chevron {
> -  list-style-image: url("chrome://global/skin/toolbar/chevron.gif") !important;
> -}

As noted earlier, please move the chevron rules back to browser.css. This should solve the test failures.

::: browser/themes/shared/toolbarbuttons.inc.css:413
(Diff revision 8)
>  }
>  
>  .unified-nav-forward[_moz-menuactive]:-moz-locale-dir(ltr),
>  .unified-nav-back[_moz-menuactive]:-moz-locale-dir(rtl) {
>    list-style-image: url("chrome://browser/skin/menu-forward.png") !important;
>  }

Please remove the unified-nav-back und unified-nav-forward rules from toolbarbuttons.inc.css as well.
Attachment #8859042 - Flags: review?(dao+bmo) → review-
Comment on attachment 8859042 [details]
Bug 1352364 - Share toolbar button styling code between platforms.

https://reviewboard.mozilla.org/r/131062/#review136198

::: browser/themes/shared/toolbarbuttons.inc.css:41
(Diff revision 10)
>  }
>  
> -.bookmark-item[cutting] > .toolbarbutton-text,
> -.bookmark-item[cutting] > .menu-iconic-left > .menu-iconic-text {
> -  opacity: 0.7;
> +#BMB_bookmarksPopup[side="left"],
> +#BMB_bookmarksPopup[side="right"] {
> +  margin-top: -20px;
> +  margin-bottom: -20px;

Please move this back to (if I see this correctly) the bottom such that diff doesn't show this as a deletion/addition.
Attachment #8859042 - Flags: review?(dao+bmo) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfc0f3c355ff
Share toolbar button styling code between platforms. r=dao
Backed out for failing browser/components/search/test/browser_oneOffHeader.js on OSX 10.10 debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/60d376796d03405fca41b2971e6bf6c05e029c2d

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dfc0f3c355ff1f70e767fdf52dc755c8565ac05e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-searchStr=bc7+10.10
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=94107960&repo=autoland

10:18:30     INFO - Entering test bound test_text
10:18:30     INFO - Opening search panel
10:18:30     INFO - TEST-PASS | browser/components/search/test/browser_oneOffHeader.js | Header has the correct index selected with a search term. - 
10:18:30     INFO - TEST-PASS | browser/components/search/test/browser_oneOffHeader.js | Search header string is correct when a search term has been entered - 
10:18:30     INFO - Buffered messages finished
10:18:30     INFO - TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_oneOffHeader.js | Header has the correct index selected when a search engine has been selected - Got 1, expected 2
10:18:30     INFO - Stack trace:
10:18:30     INFO -     chrome://mochikit/content/browser-test.js:test_is:928
10:18:30     INFO -     chrome://mochitests/content/browser/browser/components/search/test/browser_oneOffHeader.js:test_text:114
10:18:30     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
10:18:30     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
10:18:30     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
10:18:30     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
10:18:30     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
10:18:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:324:15
10:18:30     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
10:18:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
10:18:30     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
10:18:30     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
10:18:30     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
10:18:30     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
10:18:30     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
10:18:30     INFO -     eventOccurred@chrome://mochitests/content/browser/browser/components/search/test/browser_oneOffHeader.js:49:7
10:18:30     INFO -     EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@resource://gre/modules/RemoteAddonsParent.jsm:670:5
10:18:30     INFO -     interposeProperty/desc.value@jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/omni.ja!/components/multiprocessShims.js:157:52
10:18:30     INFO -     synthesizeNativeMouseMove/<@chrome://mochitests/content/browser/browser/components/search/test/browser_oneOffHeader.js:52:5
10:18:30     INFO -     Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:390:5
10:18:30     INFO -     synthesizeNativeMouseMove@chrome://mochitests/content/browser/browser/components/search/test/browser_oneOffHeader.js:46:10
10:18:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:319:42
10:18:30     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
10:18:30     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
10:18:30     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
10:18:30     INFO -     TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
10:18:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
10:18:30     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
10:18:30     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
10:18:30     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
10:18:30     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
10:18:30     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
10:18:30     INFO -     eventOccurred@chrome://mochitests/content/browser/browser/components/search/test/browser_oneOffHeader.js:49:7
10:18:30     INFO -     EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@resource://gre/modules/RemoteAddonsParent.jsm:670:5
10:18:30     INFO -     interposeProperty/desc.value@jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/omni.ja!/components/multiprocessShims.js:157:52
10:18:30     INFO -     synthesizeNativeMouseMove/<@chrome://mochitests/content/browser/browser/components/search/test/browser_oneOffHeader.js:52:5
10:18:30     INFO -     Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:390:5
10:18:30     INFO -     synthesizeNativeMouseMove@chrome://mochitests/content/browser/browser/components/search/test/browser_oneOffHeader.js:46:10
10:18:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:319:42
10:18:30     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
10:18:30     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
10:18:30     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
10:18:30     INFO -     TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
10:18:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
10:18:30     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
10:18:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
10:18:30     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
10:18:30     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
10:18:30     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
10:18:30     INFO -     TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
10:18:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
10:18:30     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
10:18:30     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
10:18:30     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
10:18:30     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
10:18:30     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
10:18:30     INFO -     onSuccess@chrome://mochitests/content/browser/browser/components/search/test/head.js:76:13
10:18:30     INFO -     SRCH_SVC_addEngine/engine._installCallback@jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/omni.ja!/components/nsSearchService.js:4062:15
Flags: needinfo?(jhofmann)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/8e7b7e0fbbe9
Backed out changeset dfc0f3c355ff for failing browser/components/search/test/browser_oneOffHeader.js on OSX 10.10 debug. r=backout
To be honest I have no idea how my changes are failing that test on OSX debug e10s (only). I can't even reproduce it locally in Chaos Mode. I thought that failure was an intermittent.

From the screenshots it looks like it might just be a timing issue. Pretty strange. I'll push a try run with a timeout, that's the best I can come up with right now.
Flags: needinfo?(jhofmann)
After rebasing on the latest central the test is timeouting on my local machine (without this patch applied). :(
It seems unlikely that the changes here are at fault, so I think we should re-land this with the test disabled on Mac, and file a bug on that.
Iteration: 55.4 - May 1 → 55.5 - May 15
I agree, let's do that. I've looked into the test for a bit and it seems really brittle with timing. I'll make a patch.
try looks better now, I'll go ahead and land the disable with rs=dao based on comment 40.
https://hg.mozilla.org/mozilla-central/rev/d72b590f31e3
https://hg.mozilla.org/mozilla-central/rev/cf470e6222f9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 1340435
Blocks: 1341227
Depends on: 1362008
Depends on: 1362409
Depends on: 1362408
Depends on: 1363406
Blocks: 1363909
Depends on: 1364329
I know the new styling has been implemented for the bookmarks toolbar. Shouldn't it be also implemented for the find bar ?
Didn't we do that? What makes you think that the findbar button style is not shared?
(In reply to Johann Hofmann [:johannh] from comment #50)
> Didn't we do that? What makes you think that the findbar button style is not
> shared?

My bad. I assumed wrongly that it would be separated from the main patch like the bookmarks toolbar was.
Depends on: 1374474
You need to log in before you can comment on or make changes to this bug.