Share toolbar button styling across platforms

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: johannh)

Tracking

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

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years 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

2 years ago
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Priority: -- → P1
(Reporter)

Updated

2 years ago
Whiteboard: [photon] → [photon-visual]
(Reporter)

Updated

2 years ago
Blocks: 1352358
Flags: qe-verify-
(Reporter)

Updated

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

Comment 3

2 years 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

2 years 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.
Comment hidden (mozreview-request)
Iteration: --- → 55.4 - May 1
(Reporter)

Comment 7

2 years 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

2 years 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

2 years ago
Can you also enable browser_backButtonFitts.js on Mac?
Comment hidden (mozreview-request)
(Reporter)

Comment 12

2 years 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

2 years 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

2 years ago
Depends on: 1358083
(Reporter)

Comment 16

2 years 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

2 years ago
Attachment #8859967 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years 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

2 years 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

2 years 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

2 years 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 25

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

Comment 26

2 years 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 29

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

Comment 31

2 years 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

2 years 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

2 years 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

2 years 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 38

2 years ago
After rebasing on the latest central the test is timeouting on my local machine (without this patch applied). :(
(Reporter)

Comment 40

2 years 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.
Iteration: 55.4 - May 1 → 55.5 - May 15
(Assignee)

Comment 41

2 years 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

2 years ago
Depends on: 1361276
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

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

Comment 45

2 years 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+

Comment 47

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

Updated

2 years ago
Blocks: 1340435
(Reporter)

Updated

2 years ago
Blocks: 1341227
(Reporter)

Updated

2 years ago
Depends on: 1362008
(Assignee)

Updated

2 years ago
Depends on: 1362281
Depends on: 1362409
Depends on: 1362408
Depends on: 1362305
(Reporter)

Updated

2 years ago
Depends on: 1363406
(Reporter)

Updated

2 years ago
Blocks: 1363909
Depends on: 1364329
(Assignee)

Updated

2 years ago
Depends on: 1365002
(Assignee)

Comment 48

2 years 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.

Comment 49

2 years ago
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

2 years ago
Didn't we do that? What makes you think that the findbar button style is not shared?

Comment 51

2 years ago
(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
(Assignee)

Updated

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