Closed
Bug 1385702
Opened 7 years ago
Closed 7 years ago
Update close icon style for photon
Categories
(Toolkit :: Themes, enhancement, P1)
Toolkit
Themes
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: ntim, Assigned: ntim)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [photon-visual][p1])
Attachments
(1 file, 1 obsolete file)
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Blocks: photon-visual
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
FYI, this will have to wait until 56 is off mozilla-central (tomorrow I think). Not sure whether we should land this before or after bug 1349555; updating either patch after the other is probably no big deal.
Blocks: photon-tabs
Flags: qe-verify+
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Whiteboard: [photon-visual][p1]
Updated•7 years ago
|
QA Contact: brindusa.tot
Comment 8•7 years ago
|
||
ntim, could you please rebase your patch? There are only some minor conflicts in browser/themes/windows/browser.css and browser/themes/shared/sidebar.inc.css.
Flags: needinfo?(ntim.bugs)
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8891741 [details] [mq]: foo https://reviewboard.mozilla.org/r/162808/#review169710
Attachment #8891741 -
Flags: review?(dao+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(ntim.bugs)
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8891741 [details] [mq]: foo https://reviewboard.mozilla.org/r/162808/#review169762 ::: toolkit/themes/shared/close-icon.inc.css:19 (Diff revision 7) > +.close-icon:hover { > + --close-icon-background-color: rgba(12,12,13,0.1); > +} > + > +.close-icon:hover:active { > + --close-icon-background-color: rgba(12,12,13,0.2); I think this should probably part of the SVG, using context-fill and context-fill-opacity. As written your background color isn't visible on dark backgrounds. ::: toolkit/themes/shared/icons/close.svg:5 (Diff revision 7) > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> > + <path fill="context-fill" d="M9.061 8l3.47-3.47a.75.75 0 0 0-1.061-1.06L8 6.939 4.53 3.47a.75.75 0 1 0-1.06 1.06L6.939 8 3.47 11.47a.75.75 0 1 0 1.06 1.06L8 9.061l3.47 3.47a.75.75 0 0 0 1.06-1.061z"></path> nit: <path/> rather than <path></path>
Attachment #8891741 -
Flags: review?(dao+bmo)
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8891741 [details] [mq]: foo https://reviewboard.mozilla.org/r/162808/#review169774 ::: browser/extensions/onboarding/content/onboarding.css:76 (Diff revision 8) > position: absolute; > top: 15px; > offset-inline-end: 15px; > cursor: pointer; > width: 16px; > height: 16px; These numbers need tweaking ::: browser/extensions/onboarding/content/onboarding.css:79 (Diff revision 8) > cursor: pointer; > width: 16px; > height: 16px; > padding: 12px; > border: none; > background: var(--onboarding-overlay-dialog-background-color); I think this can go away? ::: browser/extensions/onboarding/content/onboarding.css:83 (Diff revision 8) > border: none; > background: var(--onboarding-overlay-dialog-background-color); > } > > .onboarding-close-btn::before { > - content: url(chrome://browser/skin/sidebar/close.svg); > + content: url(chrome://global/skin/icons/close.svg); Need to set -moz-context-properties, fill, and fill-opacity. ::: toolkit/themes/shared/close-icon.inc.css:6 (Diff revision 8) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +.close-icon { > + --close-icon-background-color: transparent; nit: remove this :)
Attachment #8891741 -
Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8891741 [details] [mq]: foo https://reviewboard.mozilla.org/r/162808/#review169850 ::: browser/extensions/onboarding/content/onboarding.css:71 (Diff revision 9) > #onboarding-tour-sync-page[data-login-state=logged-out] .show-on-logged-in { > display: none; > } > > .onboarding-close-btn { > + background: url(chrome://global/skin/icons/close.svg); Pretty sure we need to keep using ::before for high-contrast themes.
Attachment #8891741 -
Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8891741 [details] [mq]: foo https://reviewboard.mozilla.org/r/162808/#review169854 Thanks a lot!
Attachment #8891741 -
Flags: review?(dao+bmo) → review+
Comment 18•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 3e01e40718dd -d fa94c0594016: rebasing 411392:3e01e40718dd "Bug 1385702 - Update and clean up close icon styling for photon. r=dao" (tip) merging browser/themes/osx/compacttheme.css warning: conflicts while merging browser/themes/osx/compacttheme.css! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8891741 -
Attachment is obsolete: true
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8893469 [details] Bug 1385702 - Update and clean up close icon styling for photon. https://reviewboard.mozilla.org/r/164548/#review169864
Attachment #8893469 -
Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 8496455dbefa -d fa94c0594016: rebasing 411392:8496455dbefa "Bug 1385702 - Update and clean up close icon styling for photon. r=dao" (tip) merging browser/themes/osx/compacttheme.css warning: conflicts while merging browser/themes/osx/compacttheme.css! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/04cf5e9b8f72 Update and clean up close icon styling for photon. r=dao
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/04cf5e9b8f72
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Iteration: --- → 57.1 - Aug 15
Comment 27•7 years ago
|
||
I see some AWSY improvements from this landing: == Change summary for alert #8532 (as of August 03 2017 17:41 UTC) == Improvements: 3% Images summary linux64-stylo opt stylo 7,652,586.31 -> 7,442,150.60 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8532
Comment 28•7 years ago
|
||
Sorry, this took a bit while we were fixing Windows, but here are the screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=36ad88e6b7b248c2f2ae59b80477e5474dd653dc&newProject=mozilla-central&newRev=5742919ec43f834cc061a96d87c767af1a1f7f75
Updated•7 years ago
|
QA Contact: brindusa.tot → ovidiu.boca
Comment 29•7 years ago
|
||
I verified this issue on Mac OS X 10.12, Mac OS X 10.10, Windows 10, Windows 7 and Ubuntu 16.04 with FF Nightly 57.0a1(2017-08-16) and I can confirm that the close button respects the spec from comment 28. I will mark this as verified fixed.
You need to log in
before you can comment on or make changes to this bug.
Description
•