817366 - Use more 2x images for HiDPI browser UI

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

This is a followup to bug 781327, which landed HiDPI support for primary browser UI. I'm fixing a bunch more images here, which are most of the ones I still notice frequently. (There are of course still more obscure once that need fixing too.)

The images in this patch are simply pixel-doubled versions of the existing icons, (plus a few pixels set to a bright color to help double check that I've using the @2x image and it's scaled properly). Steven will need to provide the actual @2x artwork before this can land.

Comment 1

[Justin Dolske [:Dolske]]

Steven: these are the images I need actual @2x version for:

  browser/themes/pinstripe/[e-mail]
  toolkit/themes/pinstripe/global/arrow/[e-mail]
  toolkit/themes/pinstripe/global/arrow/[e-mail]
  toolkit/themes/pinstripe/global/arrow/[e-mail]
  toolkit/themes/pinstripe/global/icons/[e-mail]
  toolkit/themes/pinstripe/global/icons/[e-mail]
  toolkit/themes/pinstripe/global/icons/[e-mail]
  toolkit/themes/pinstripe/global/media/[e-mail]
  toolkit/themes/pinstripe/global/media/[e-mail]
  toolkit/themes/pinstripe/global/media/[e-mail]
  toolkit/themes/pinstripe/global/media/[e-mail]
  toolkit/themes/pinstripe/global/media/[e-mail]
  toolkit/themes/pinstripe/global/media/[e-mail]
  toolkit/themes/pinstripe/global/media/[e-mail]
  toolkit/themes/pinstripe/global/media/[e-mail]
  toolkit/themes/pinstripe/global/media/[e-mail]
  toolkit/themes/pinstripe/mozapps/passwordmgr/[e-mail]

Comment 2

[Frank Yan (:fryn)]

Comment on attachment 687489 [details] [diff] [review]
Patch v.1

Review of attachment 687489 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed and actual images provided.

::: browser/themes/pinstripe/browser.css
@@ +4004,5 @@
> +@media (min-resolution: 2dppx) {
> +  panel[type="arrow"][popupid="click-to-play-plugins"] > .panel-arrowcontainer > .panel-arrowbox > .panel-arrow[side="top"],
> +  panel[type="arrow"][popupid="click-to-play-plugins"] > .panel-arrowcontainer > .panel-arrowbox > .panel-arrow[side="bottom"] {
> +    /* this isn't working, I still get non-light hidpi image. :( */
> +    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;

If this is referring to the panel arrow of the "Click to play" arrow panel, this seems to work for me. Did you rebuild the required directories?

Could you move this rule after the non-HiDPI rule and remove the !important?

::: toolkit/themes/pinstripe/global/findBar.css
@@ +161,5 @@
>  .findbar-container > checkbox > .checkbox-label-box > .checkbox-icon {
>    -moz-padding-start: 1px;
>    padding-bottom: 1px;
> +  width: 17px;
> +  height: 17px;

Could you put this inside an @media block?
It's been an optimization at best and a convention at worst for us not to lock dimensions when we shouldn't need to, so when I've had to lock the dimensions for this hacky HiDPI technique, I put these rules inside @media blocks, so they are low-risk (only affecting HiDPI users), and it's easy to spot which rules to remove if we ever switch to a less hacky implementation.

Comment 3

[Justin Dolske [:Dolske]]

(In reply to Frank Yan (:fryn) from comment #2)

> > +    /* this isn't working, I still get non-light hidpi image. :( */
> > +    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;
> 
> If this is referring to the panel arrow of the "Click to play" arrow panel,
> this seems to work for me. Did you rebuild the required directories?

Oops, outdated comment. Something was wonky in my build and it wasn't showing up, then after building elsewhere for this patch it suddenly was ok.

Comment 4

[Stephen Horlander [:shorlander] (If you're waiting on a response please ping me on Slack or Riot)]

Attachment: More HiDPI Icons

Attached patch has all the of the requested icons, but also includes an updated key-16.png with the current notification icon style.

>   browser/themes/pinstripe/[e-mail]

Added one for this for now even though it needs to be replaced.

Comment 5

[Dão Gottwald [::dao]]

Comment on attachment 687489 [details] [diff] [review]
Patch v.1

>+@media (min-resolution: 2dppx) {
>+  panel[type="arrow"][popupid="click-to-play-plugins"] > .panel-arrowcontainer > .panel-arrowbox > .panel-arrow[side="top"],
>+  panel[type="arrow"][popupid="click-to-play-plugins"] > .panel-arrowcontainer > .panel-arrowbox > .panel-arrow[side="bottom"] {
>+    /* this isn't working, I still get non-light hidpi image. :( */
>+    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;
>+ }
>+}

Please get this sorted out before landing it. (last resort: leave it out and file a followup bug for getting this hidpi image landed properly)

>+@media (min-resolution: 2dppx) {
>+  .playButton {
>+    background: url(chrome://global/skin/media/[e-mail]) no-repeat center;

This can just be background-image, no-repeat and center were specified earlier.

>+@media (min-resolution: 2dppx) {
>+  .panel-arrow[side="top"],
>+  .panel-arrow[side="bottom"] {
>+    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;
>+    width: 39px;
>+    height: 16px;
>+  }
>+
>+  .panel-arrow[side="left"],
>+  .panel-arrow[side="right"] {
>+    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;
>+    width: 16px;
>+    height: 39px;
>+  }
>+}

What's !important needed for here? Can you just move this block after the rules that you're overriding?

>--- a/toolkit/themes/pinstripe/global/resizer.css
>+++ b/toolkit/themes/pinstripe/global/resizer.css
>@@ -7,25 +7,39 @@
> resizer {
>   -moz-appearance: resizer;
>   background: url("chrome://global/skin/icons/resizer.png") no-repeat;
>   background-size: 100% 100%;
>   cursor: se-resize;
>   width: 15px;
>   height: 15px;
> }
>+@media (min-resolution: 2dppx) {
>+  resizer {
>+    background: url("chrome://global/skin/icons/[e-mail]") no-repeat;
>+    background-size: 100% 100%;
>+  }

background-image again, the rest is redundant.

Comment 6

[Dão Gottwald [::dao]]

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 687489 [details] [diff] [review]
> Patch v.1
> 
> >+@media (min-resolution: 2dppx) {
> >+  panel[type="arrow"][popupid="click-to-play-plugins"] > .panel-arrowcontainer > .panel-arrowbox > .panel-arrow[side="top"],
> >+  panel[type="arrow"][popupid="click-to-play-plugins"] > .panel-arrowcontainer > .panel-arrowbox > .panel-arrow[side="bottom"] {
> >+    /* this isn't working, I still get non-light hidpi image. :( */
> >+    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;
> >+ }
> >+}
> 
> Please get this sorted out before landing it. (last resort: leave it out and
> file a followup bug for getting this hidpi image landed properly)

I just saw comment 3.

Comment 7

[Frank Yan (:fryn)]

(In reply to Dão Gottwald [:dao] from comment #5)

Good catches. I should've read this patch more carefully.

Comment 8

[Justin Dolske [:Dolske]]

(In reply to Frank Yan (:fryn) from comment #2)
> ::: browser/themes/pinstripe/browser.css
...
> > +    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;
> 
> Could you move this rule after the non-HiDPI rule and remove the !important?

I moved it, but for some reason the !important is still needed. O_o I flipped back and forth a few times, with |make -C browser| in between, just to be sure. Without the !important I just get the non-light hidpi version. Guess that's what my comment was referring to!

/(In reply to Dão Gottwald [:dao] from comment #5)

> >+@media (min-resolution: 2dppx) {
> >+  .playButton {
> >+    background: url(chrome://global/skin/media/[e-mail]) no-repeat center;
> 
> This can just be background-image, no-repeat and center were specified
> earlier.

Fixed all the occurrences of that.


> >+    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;
...
> 
> What's !important needed for here? Can you just move this block after the
> rules that you're overriding?

Done. I think this was the first part of the patch I worked on, and I forgot to fix it after I noticed how we were avoiding !important elsewhere.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Merged with steven's updated icons.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff14bf18c9c

Comment 10

[Frank Yan (:fryn)]

Just to add this to the record so people not on IRC won't be confused:

(In reply to Justin Dolske [:Dolske] from comment #8)
> (In reply to Frank Yan (:fryn) from comment #2)
> > > +    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;
> > 
> > Could you move this rule after the non-HiDPI rule and remove the !important?
> 
> I moved it, but for some reason the !important is still needed. O_o I
> flipped back and forth a few times, with |make -C browser| in between, just
> to be sure. Without the !important I just get the non-light hidpi version.
> Guess that's what my comment was referring to!

I think this is because you were trying to remove the !important for the light rule before you removed the !important for the generic non-light rule that you had below.

> > >+    list-style-image: url("chrome://global/skin/arrow/[e-mail]") !important;

Comment 11

[Justin Dolske [:Dolske]]

(In reply to Frank Yan (:fryn) from comment #10)

> I think this is because you were trying to remove the !important for the
> light rule before you removed the !important for the generic non-light rule
> that you had below.

Indeed! That was exactly the problem. Good catch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/170cac918fb2

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3ff14bf18c9c

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/170cac918fb2