Closed Bug 1153309 Opened 5 years ago Closed 5 years ago

Use a @2x menuPanel.png on Windows for basic hidpi support

Categories

(Firefox :: Theme, defect)

x86
Windows 8.1
defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
41.2 - Jun 8
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [windows10])

Attachments

(3 files, 3 obsolete files)

+++ 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
Iteration: 40.1 - 13 Apr → ---
Points: 5 → 3
No longer depends on: 1153243, 1147702
Yeah, it seems they were ignored. Thanks for linking there, this bug should be actionable now :)
(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: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
Points: 3 → 5
Flags: qe-verify+
Flags: firefox-backlog+
Whiteboard: [windows10]
Attached patch Patch (obsolete) — Splinter Review
Tested on Windows, Linux, and OSX.
Attachment #8609030 - Attachment is obsolete: true
Attachment #8609503 - Flags: review?(gijskruitbosch+bugs)
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+
Attached patch InterdiffSplinter Review
Attachment #8609503 - Attachment is obsolete: true
Attached patch Patch v1.1 (obsolete) — Splinter Review
Interdiff attached as a separate attachment.
Attachment #8610720 - Flags: review?(gijskruitbosch+bugs)
(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.
Attached patch Patch v1.2Splinter Review
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)
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.
Attachment #8610726 - Flags: review?(gijskruitbosch+bugs) → review+
Iteration: 41.1 - May 25 → 41.2 - Jun 8
https://hg.mozilla.org/mozilla-central/rev/daeb053ca8d4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Depends on: 1169314
QA Contact: cornel.ionce
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
Attached patch Patch for 40Splinter Review
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?
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+
Verified as fixed on Firefox 40 beta 2, build ID: 20150706172413.
Depends on: 1221174
You need to log in before you can comment on or make changes to this bug.