Closed Bug 817366 Opened 7 years ago Closed 7 years ago

Use more 2x images for HiDPI browser UI

Categories

(Firefox :: Theme, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: Dolske, Assigned: Dolske)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v.1 (obsolete) — Splinter Review
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.
Attachment #687489 - Flags: review?(fryn)
Steven: these are the images I need actual @2x version for:

  browser/themes/pinstripe/webRTC-shareDevice-16@2x.png
  toolkit/themes/pinstripe/global/arrow/panelarrow-horizontal@2x.png
  toolkit/themes/pinstripe/global/arrow/panelarrow-light-vertical@2x.png
  toolkit/themes/pinstripe/global/arrow/panelarrow-vertical@2x.png
  toolkit/themes/pinstripe/global/icons/checkbox@2x.png
  toolkit/themes/pinstripe/global/icons/resizer-rtl@2x.png
  toolkit/themes/pinstripe/global/icons/resizer@2x.png
  toolkit/themes/pinstripe/global/media/fullscreenButton@2x.png
  toolkit/themes/pinstripe/global/media/muteButton@2x.png
  toolkit/themes/pinstripe/global/media/noAudio@2x.png
  toolkit/themes/pinstripe/global/media/pauseButton@2x.png
  toolkit/themes/pinstripe/global/media/playButton@2x.png
  toolkit/themes/pinstripe/global/media/scrubberThumb@2x.png
  toolkit/themes/pinstripe/global/media/scrubberThumbWide@2x.png
  toolkit/themes/pinstripe/global/media/unmuteButton@2x.png
  toolkit/themes/pinstripe/global/media/volumeThumb@2x.png
  toolkit/themes/pinstripe/mozapps/passwordmgr/key-16@2x.png
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/panelarrow-light-vertical@2x.png") !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.
Attachment #687489 - Flags: review?(fryn) → review+
(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/panelarrow-light-vertical@2x.png") !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.
Attached patch More HiDPI Icons (obsolete) — Splinter Review
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/webRTC-shareDevice-16@2x.png

Added one for this for now even though it needs to be replaced.
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/panelarrow-light-vertical@2x.png") !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/pauseButton@2x.png) 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/panelarrow-vertical@2x.png") !important;
>+    width: 39px;
>+    height: 16px;
>+  }
>+
>+  .panel-arrow[side="left"],
>+  .panel-arrow[side="right"] {
>+    list-style-image: url("chrome://global/skin/arrow/panelarrow-horizontal@2x.png") !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/resizer@2x.png") no-repeat;
>+    background-size: 100% 100%;
>+  }

background-image again, the rest is redundant.
(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/panelarrow-light-vertical@2x.png") !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.
(In reply to Dão Gottwald [:dao] from comment #5)

Good catches. I should've read this patch more carefully.
(In reply to Frank Yan (:fryn) from comment #2)
> ::: browser/themes/pinstripe/browser.css
...
> > +    list-style-image: url("chrome://global/skin/arrow/panelarrow-light-vertical@2x.png") !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/pauseButton@2x.png) 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/panelarrow-vertical@2x.png") !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.
Attached patch Patch v.2Splinter Review
Merged with steven's updated icons.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff14bf18c9c
Attachment #687489 - Attachment is obsolete: true
Attachment #688300 - Attachment is obsolete: true
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/panelarrow-light-vertical@2x.png") !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/panelarrow-vertical@2x.png") !important;
(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
https://hg.mozilla.org/mozilla-central/rev/170cac918fb2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Blocks: 819750
Blocks: 795665
No longer blocks: 795665
You need to log in before you can comment on or make changes to this bug.