Share toolbar button styling across platforms

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Theme
P1
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: dao, Assigned: johannh)

Tracking

(Depends on: 2 bugs)

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

Firefox Tracking Flags

(firefox55 fixed)

Details

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

MozReview Requests

()

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

5 months ago
status-firefox55: affected → ---
(Reporter)

Updated

5 months ago
Status: NEW → ASSIGNED
(Reporter)

Updated

5 months ago
Priority: -- → P1
(Reporter)

Updated

4 months ago
Whiteboard: [photon] → [photon-visual]
(Reporter)

Updated

4 months ago
Blocks: 1352358

Updated

4 months ago
Flags: qe-verify-
(Reporter)

Updated

4 months ago
Whiteboard: [photon-visual] → [photon-visual][p1]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

4 months ago
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.
(Assignee)

Comment 4

4 months ago
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.
(Assignee)

Comment 5

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32a21545f28d201c1fd95aebdf78bd00aa9a8508
Comment hidden (mozreview-request)

Updated

4 months ago
Iteration: --- → 55.4 - May 1
(Reporter)

Comment 7

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

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

4 months ago
Can you also enable browser_backButtonFitts.js on Mac?
Comment hidden (mozreview-request)
(Assignee)

Comment 11

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1c00a838d9a9844748d4a8af801a4d2684b7995
(Reporter)

Comment 12

4 months 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)
(Assignee)

Comment 15

4 months ago
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.
(Assignee)

Updated

4 months ago
Depends on: 1358083
(Reporter)

Comment 16

4 months 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)
(Assignee)

Updated

4 months ago
Attachment #8859967 - Attachment is obsolete: true
(Assignee)

Comment 18

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

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

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

4 months 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)
(Assignee)

Comment 24

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6db6652dd583f332125b0f4a6f5589af8123f6e
(Assignee)

Comment 25

4 months ago
There are some test failures from missing files, I'll look into it.
(Reporter)

Comment 26

4 months 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)
(Assignee)

Comment 28

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=487e0ab2c1a1f5a61f00458dd31ff6d7873567b7
(Assignee)

Comment 29

4 months ago
Yup, I missed those. Thanks! :)
Comment hidden (mozreview-request)
(Reporter)

Comment 31

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

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

4 months 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
(Assignee)

Comment 36

4 months ago
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)
(Assignee)

Comment 37

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22c2b79772bc183f085ad286f78abc276e8bdfbc
(Assignee)

Comment 38

4 months ago
After rebasing on the latest central the test is timeouting on my local machine (without this patch applied). :(
(Assignee)

Comment 39

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a040735b79b367b4794b8d2daf31117fc4a3db46
(Reporter)

Comment 40

4 months ago
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.

Updated

4 months ago
Iteration: 55.4 - May 1 → 55.5 - May 15
(Assignee)

Comment 41

4 months ago
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.
(Assignee)

Updated

4 months ago
Depends on: 1361276
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

4 months ago
try looks better now, I'll go ahead and land the disable with rs=dao based on comment 40.
(Assignee)

Comment 45

4 months ago
mozreview-review
Comment on attachment 8863627 [details]
Bug 1352364 - Disable browser_oneOffHeader.js on OSX. rs=dao

https://reviewboard.mozilla.org/r/135418/#review138424
Attachment #8863627 - Flags: review+
(Assignee)

Comment 46

4 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d72b590f31e35d634a326f8add941642b740e2a7
Bug 1352364 - Share toolbar button styling code between platforms. r=dao

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf470e6222f971b5cb4551ff627b49152a326c22
Bug 1352364 - Disable browser_oneOffHeader.js on OSX. rs=dao

Comment 47

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d72b590f31e3
https://hg.mozilla.org/mozilla-central/rev/cf470e6222f9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Reporter)

Updated

4 months ago
Blocks: 1340435
(Reporter)

Updated

4 months ago
Blocks: 1341227
(Reporter)

Updated

4 months ago
Depends on: 1362008
(Assignee)

Updated

4 months ago
Depends on: 1362281
Depends on: 1362409
Depends on: 1362408
Depends on: 1362305
(Reporter)

Updated

3 months ago
Depends on: 1363406
(Reporter)

Updated

3 months ago
Blocks: 1363909
Depends on: 1364329
(Assignee)

Updated

3 months ago
Depends on: 1365002
(Assignee)

Comment 48

3 months ago
Better late than never, mozscreenshots results:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=5eaf2d70eded608e64f459e6b255b4725ed95efe&newProject=mozilla-central&newRev=a748acbebbde373a88868dc02910fb2bc5e6a023

Shows a couple of minor differences in spacing but looks fine on all platforms that I checked.
I know the new styling has been implemented for the bookmarks toolbar. Shouldn't it be also implemented for the find bar ?
(Assignee)

Comment 50

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

Updated

2 months ago
Depends on: 1374474
(Assignee)

Updated

2 months ago
No longer depends on: 1374474
You need to log in before you can comment on or make changes to this bug.