Share toolbar button styling across platforms

ASSIGNED
Assigned to

Status

()

Firefox
Theme
P1
normal
ASSIGNED
27 days ago
16 hours ago

People

(Reporter: dao, Assigned: johannh)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

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

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

27 days ago
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.
(Reporter)

Updated

27 days ago
status-firefox55: affected → ---
(Reporter)

Updated

27 days ago
Status: NEW → ASSIGNED
(Reporter)

Updated

27 days ago
Priority: -- → P1
(Reporter)

Updated

15 days ago
Whiteboard: [photon] → [photon-visual]
(Reporter)

Updated

15 days ago
Blocks: 1352358

Updated

14 days ago
Flags: qe-verify-
(Reporter)

Updated

11 days ago
Whiteboard: [photon-visual] → [photon-visual][p1]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32a21545f28d201c1fd95aebdf78bd00aa9a8508
Comment hidden (mozreview-request)
Iteration: --- → 55.4 - May 1
(Reporter)

Comment 7

8 days ago
mozreview-review
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.
(Reporter)

Comment 8

8 days ago
mozreview-review
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)
(Reporter)

Comment 9

8 days ago
Can you also enable browser_backButtonFitts.js on Mac?
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1c00a838d9a9844748d4a8af801a4d2684b7995
(Reporter)

Comment 12

7 days ago
mozreview-review
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Depends on: 1358083
(Reporter)

Comment 16

6 days ago
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 hidden (mozreview-request)
Attachment #8859967 - Attachment is obsolete: true
(Assignee)

Comment 18

6 days ago
mozreview-review-reply
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 hidden (mozreview-request)
(Reporter)

Comment 20

6 days ago
mozreview-review
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-
(Assignee)

Comment 21

5 days ago
mozreview-review-reply
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.
(Reporter)

Comment 22

5 days ago
(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.
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6db6652dd583f332125b0f4a6f5589af8123f6e
There are some test failures from missing files, I'll look into it.
(Reporter)

Comment 26

2 days ago
mozreview-review
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 hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=487e0ab2c1a1f5a61f00458dd31ff6d7873567b7
Yup, I missed those. Thanks! :)
Comment hidden (mozreview-request)
(Reporter)

Comment 31

a day ago
mozreview-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+
Comment hidden (mozreview-request)

Comment 33

a day ago
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)

Comment 35

a day ago
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)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22c2b79772bc183f085ad286f78abc276e8bdfbc
After rebasing on the latest central the test is timeouting on my local machine (without this patch applied). :(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a040735b79b367b4794b8d2daf31117fc4a3db46
You need to log in before you can comment on or make changes to this bug.