Closed
Bug 1153309
Opened 9 years ago
Closed 9 years ago
Use a @2x menuPanel.png on Windows for basic hidpi support
Categories
(Firefox :: Theme, defect)
Tracking
()
People
(Reporter: jaws, Assigned: jaws)
References
(Blocks 1 open bug)
Details
(Whiteboard: [windows10])
Attachments
(3 files, 3 obsolete files)
4.39 KB,
patch
|
Details | Diff | Splinter Review | |
529.07 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
529.24 KB,
patch
|
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1147702 +++ We will probably need: /browser/themes/windows/menuPanel@2x.png /browser/themes/windows/menuPanel-customize@2x.png /browser/themes/windows/menuPanel-exit@2x.png /browser/themes/windows/menuPanel-help@2x.png /browser/themes/windows/menuPanel-small@2x.png /browser/themes/windows/syncProgress-menuPanel@2x.png
Assignee | ||
Updated•9 years ago
|
Comment 1•9 years ago
|
||
All of those assets were part of the @2x assets patch in bug 1147702 https://bug1147702.bugzilla.mozilla.org/attachment.cgi?id=8584500
Assignee | ||
Comment 2•9 years ago
|
||
Yeah, it seems they were ignored. Thanks for linking there, this bug should be actionable now :)
Comment 3•9 years ago
|
||
(In reply to (behind on reviews) Jared Wein [:jaws] (please needinfo? me) from comment #2) > Yeah, it seems they were ignored. They were out of that bug's scope.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Iteration: --- → 41.1 - May 25
Points: 3 → 5
Flags: qe-verify+
Flags: firefox-backlog+
Whiteboard: [windows10]
Assignee | ||
Updated•9 years ago
|
Blocks: windows-10
Assignee | ||
Comment 5•9 years ago
|
||
Tested on Windows, Linux, and OSX.
Attachment #8609030 -
Attachment is obsolete: true
Attachment #8609503 -
Flags: review?(gijskruitbosch+bugs)
Comment 6•9 years ago
|
||
Comment on attachment 8609503 [details] [diff] [review] Patch Review of attachment 8609503 [details] [diff] [review]: ----------------------------------------------------------------- Almost there. If possible, please provide an interdiff for the next iteration of this patch. It's almost there, and I wouldn't want to be going through it with a fine comb again. :-) TBH... where we are copying the windows files to linux, can we just make the jar.mn thing source the files from the windows directory? Having identical files in the tree several times is tedious and is bound to lead to them getting out of sync at some point. (I realize this is not new with your patch, I'd just rather change it. If you agree, please do that for the new files and we can file a followup to update the rest of them?) ::: browser/themes/osx/browser.css @@ +1071,5 @@ > #PanelUI-fxa-status > .toolbarbutton-icon, > #PanelUI-quit > .toolbarbutton-icon, > #PanelUI-customize > .toolbarbutton-icon, > #PanelUI-help > .toolbarbutton-icon { > width: 16px; This can be removed now because it exists in the shared file, though that rule misses update-status (see below). ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +465,5 @@ > + .panelUI-grid .toolbarbutton-1 > .toolbarbutton-icon, > + .panelUI-grid .toolbarbutton-1 > .toolbarbutton-badge-container, > + .customization-palette .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon, > + .customization-palette .toolbarbutton-1 > .toolbarbutton-icon, > + .customization-palette .toolbarbutton-1 > .toolbarbutton-badge-container, Shouldn't this be the icon inside the badge-container? That's certainly what we do for the toolbar one, and in the rule above. @@ +469,5 @@ > + .customization-palette .toolbarbutton-1 > .toolbarbutton-badge-container, > + .panelUI-grid #bookmarks-toolbar-placeholder > .toolbarbutton-icon, > + .customization-palette #bookmarks-toolbar-placeholder > .toolbarbutton-icon, > + .panel-customization-placeholder-child > .toolbarbutton-icon { > + width: 32px; Why this rule? We already set a min-width for these in the rules above here. Should that just become a width, or both? I'm worried that this being in a media query will create lodpi/hidpi-only bugs. I can see that if I remove this rule, things look wonky on Windows hidpi. What I don't understand is how we never needed this on OSX (at least, I can't find an existing identical rule there). @@ +1452,5 @@ > + > + #PanelUI-fxa-status > .toolbarbutton-icon, > + #PanelUI-customize > .toolbarbutton-icon, > + #PanelUI-help > .toolbarbutton-icon, > + #PanelUI-quit > .toolbarbutton-icon { This misses update-status (for the nightly+aurora only update footer item). ::: browser/themes/shared/menupanel.inc.css @@ +154,5 @@ > + toolbarpaletteitem[place="palette"] > #sidebar-button { > + -moz-image-region: rect(0, 864px, 32px, 832px); > + } > + > + #sidebar-button[cui-areatype="menu-panel"][panel-multiview-anchor=true] { Thanks for catching this. Was there a separate bug filed about this?
Attachment #8609503 -
Flags: review?(gijskruitbosch+bugs) → feedback+
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8609503 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Interdiff attached as a separate attachment.
Attachment #8610720 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #6) > TBH... where we are copying the windows files to linux, can we just make the > jar.mn thing source the files from the windows directory? Discussed over IRC and we'll file a follow-up bug to make this change for all of the icons instead of doing it piece-meal in this bug. > ::: browser/themes/osx/browser.css > @@ +1071,5 @@ > > #PanelUI-fxa-status > .toolbarbutton-icon, > > #PanelUI-quit > .toolbarbutton-icon, > > #PanelUI-customize > .toolbarbutton-icon, > > #PanelUI-help > .toolbarbutton-icon { > > width: 16px; > > This can be removed now because it exists in the shared file, though that > rule misses update-status (see below). Removed. > ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css > @@ +465,5 @@ > > + .panelUI-grid .toolbarbutton-1 > .toolbarbutton-icon, > > + .panelUI-grid .toolbarbutton-1 > .toolbarbutton-badge-container, > > + .customization-palette .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon, > > + .customization-palette .toolbarbutton-1 > .toolbarbutton-icon, > > + .customization-palette .toolbarbutton-1 > .toolbarbutton-badge-container, > > Shouldn't this be the icon inside the badge-container? That's certainly what > we do for the toolbar one, and in the rule above. Yes, thanks for catching that. > @@ +469,5 @@ > > + .customization-palette .toolbarbutton-1 > .toolbarbutton-badge-container, > > + .panelUI-grid #bookmarks-toolbar-placeholder > .toolbarbutton-icon, > > + .customization-palette #bookmarks-toolbar-placeholder > .toolbarbutton-icon, > > + .panel-customization-placeholder-child > .toolbarbutton-icon { > > + width: 32px; > > Why this rule? We already set a min-width for these in the rules above here. > Should that just become a width, or both? I'm worried that this being in a > media query will create lodpi/hidpi-only bugs. > > I can see that if I remove this rule, things look wonky on Windows hidpi. > What I don't understand is how we never needed this on OSX (at least, I > can't find an existing identical rule there). I moved the width:32px; and height:32px; to the above rule and removed the specialized 1.1dppx version of it since it is harmless on the 1dppx version. I am not sure why this wasn't caught before or what makes Windows react differently than OSX. > @@ +1452,5 @@ > > + > > + #PanelUI-fxa-status > .toolbarbutton-icon, > > + #PanelUI-customize > .toolbarbutton-icon, > > + #PanelUI-help > .toolbarbutton-icon, > > + #PanelUI-quit > .toolbarbutton-icon { > > This misses update-status (for the nightly+aurora only update footer item). Fixed. > ::: browser/themes/shared/menupanel.inc.css > @@ +154,5 @@ > > + toolbarpaletteitem[place="palette"] > #sidebar-button { > > + -moz-image-region: rect(0, 864px, 32px, 832px); > > + } > > + > > + #sidebar-button[cui-areatype="menu-panel"][panel-multiview-anchor=true] { > > Thanks for catching this. Was there a separate bug filed about this? I didn't know of a separate bug filed for this. I just saw this in my testing.
Assignee | ||
Comment 10•9 years ago
|
||
Noticed one more change that was made only for OSX but should exist for Windows and Linux now that they have HiDPI icons.
I moved,
> toolbarpaletteitem:-moz-any([place="palette"], [place="panel"]) > toolbaritem[sdkstylewidget="true"] > .toolbarbutton-1 > .toolbarbutton-icon
> width: 32px;
> height: 32px;
> }
from osx/customizableui/panelUIOverlay.css to shared/customizableui/panelUIOverlay.inc.css
Attachment #8610720 -
Attachment is obsolete: true
Attachment #8610720 -
Flags: review?(gijskruitbosch+bugs)
Attachment #8610726 -
Flags: review?(gijskruitbosch+bugs)
Comment 11•9 years ago
|
||
FYI: If you want to check any CSS refactorings for obvious regressions affecting static appearance (not hover/interactive aspects), you can now use mozscreenshots more easily via try server by importing this patch[1] and changing the environment variable in the try syntax to match the screenshot set names that you want. You can then use fetch_screenshots.py to download the screenshots and comparison.py to check for any differences between sets of images. There is already an AppMenu configuration that you can use. I just got this working on Try this weekend so haven't had a chance to blog or send this to the mailing lists yet.
Updated•9 years ago
|
Attachment #8610726 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Updated•9 years ago
|
Iteration: 41.1 - May 25 → 41.2 - Jun 8
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/daeb053ca8d4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•9 years ago
|
QA Contact: cornel.ionce
Comment 14•9 years ago
|
||
While testing this I've also ran into bug 1153306, which is already assigned. Confirming the fix on Windows 7 64-bit, Windows 8.1 32-bit and Windows 10 64-bit using latest Nightly, build ID: 20150608030201.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 15•9 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: Windows 10 HiDPI [User impact if declined]: HiDPI icons on Windows are blurry [Describe test coverage new/current, TreeHerder]: on mozilla-central for much of 41-nightly [Risks and why]: none expected [String/UUID change made/needed]: none https://hg.mozilla.org/try/pushloghtml?changeset=71084a9edf1e
Attachment #8627202 -
Flags: approval-mozilla-beta?
Updated•9 years ago
|
status-firefox40:
--- → affected
Comment 16•9 years ago
|
||
Comment on attachment 8627202 [details] [diff] [review] Patch for 40 Verified visual fix in support of Windows 10. Beta+
Attachment #8627202 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 18•9 years ago
|
||
Verified as fixed on Firefox 40 beta 2, build ID: 20150706172413.
You need to log in
before you can comment on or make changes to this bug.
Description
•