Closed
Bug 817366
Opened 8 years ago
Closed 8 years ago
Use more 2x images for HiDPI browser UI
Categories
(Firefox :: Theme, defect)
Tracking
()
RESOLVED
FIXED
Firefox 20
People
(Reporter: Dolske, Assigned: Dolske)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
46.92 KB,
patch
|
Details | Diff | 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)
Assignee | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
(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.
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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.
Comment 6•8 years ago
|
||
(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.
![]() |
||
Comment 7•8 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #5) Good catches. I should've read this patch more carefully.
Assignee | ||
Comment 8•8 years ago
|
||
(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.
Assignee | ||
Comment 9•8 years ago
|
||
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
![]() |
||
Comment 10•8 years ago
|
||
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;
Assignee | ||
Comment 11•8 years ago
|
||
(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 13•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/170cac918fb2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in
before you can comment on or make changes to this bug.
Description
•