Closed Bug 1385702 Opened 7 years ago Closed 7 years ago

Update close icon style for photon

Categories

(Toolkit :: Themes, enhancement, P1)

Product:
Toolkit
Component:
Themes
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla57
Iteration:
57.1 - Aug 15
Tracking Flags:
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)

Bug 1385702 - Update and clean up close icon styling for photon.
59 bytes, text/x-review-board-request
dao
: review+
Details 
      No description provided.
Blocks: photon-visual
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]
QA Contact: brindusa.tot
Depends on: 1349555
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)
Blocks: 1387028
Flags: needinfo?(ntim.bugs)
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 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 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 on attachment 8891741 [details]
[mq]: foo

https://reviewboard.mozilla.org/r/162808/#review169854

Thanks a lot!
Attachment #8891741 - Flags: review?(dao+bmo) → review+
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)
Attachment #8891741 - Attachment is obsolete: true
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+
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)
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
https://hg.mozilla.org/mozilla-central/rev/04cf5e9b8f72
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Iteration: --- → 57.1 - Aug 15
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
Depends on: 1387496
QA Contact: brindusa.tot → ovidiu.boca
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.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
Depends on: 1400863
Depends on: 1403543
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: