Closed
Bug 1352364
Opened 8 years ago
Closed 8 years ago
Share toolbar button styling across platforms
Categories
(Firefox :: Theme, enhancement, P1)
Firefox
Theme
Tracking
()
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.
Reporter | ||
Updated•8 years ago
|
status-firefox55:
affected → ---
Reporter | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Updated•8 years ago
|
Priority: -- → P1
Reporter | ||
Updated•8 years ago
|
Whiteboard: [photon] → [photon-visual]
Updated•8 years ago
|
Flags: qe-verify-
Reporter | ||
Updated•8 years ago
|
Whiteboard: [photon-visual] → [photon-visual][p1]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 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•8 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.
Assignee | ||
Comment 5•8 years ago
|
||
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Iteration: --- → 55.4 - May 1
Reporter | ||
Comment 7•8 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•8 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•8 years ago
|
||
Can you also enable browser_backButtonFitts.js on Mac?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 years ago
|
||
Reporter | ||
Comment 12•8 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•8 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.
Reporter | ||
Comment 16•8 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•8 years ago
|
Attachment #8859967 -
Attachment is obsolete: true
Assignee | ||
Comment 18•8 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•8 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•8 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•8 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 24•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
There are some test failures from missing files, I'll look into it.
Reporter | ||
Comment 26•8 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 28•8 years ago
|
||
Assignee | ||
Comment 29•8 years ago
|
||
Yup, I missed those. Thanks! :)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 31•8 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•8 years ago
|
||
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfc0f3c355ff
Share toolbar button styling code between platforms. r=dao
Comment 34•8 years ago
|
||
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•8 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•8 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 37•8 years ago
|
||
Assignee | ||
Comment 38•8 years ago
|
||
After rebasing on the latest central the test is timeouting on my local machine (without this patch applied). :(
Assignee | ||
Comment 39•8 years ago
|
||
Reporter | ||
Comment 40•8 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.
Updated•8 years ago
|
Iteration: 55.4 - May 1 → 55.5 - May 15
Assignee | ||
Comment 41•8 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•8 years ago
|
||
try looks better now, I'll go ahead and land the disable with rs=dao based on comment 40.
Assignee | ||
Comment 45•8 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+
Assignee | ||
Comment 46•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d72b590f31e3
https://hg.mozilla.org/mozilla-central/rev/cf470e6222f9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1362305
Assignee | ||
Comment 48•7 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•7 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•7 years ago
|
||
Didn't we do that? What makes you think that the findbar button style is not shared?
Comment 51•7 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.
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•