1169716 - The Places icons are blurry/pixelated on HiDPI Windows

Status: RESOLVED INCOMPLETE

(Firefox :: Theme, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Hey Stephen, can you attach HiDPI versions of the bookmarks toolbar, bookmarks menu, and unsorted bookmarks graphics? I've got the ones for the folders, livemarks, and queries.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8613030 [details] [diff] [review]
WIP

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

Gijs, are you OK with landing this now and then doing a separate bug for the other icons when we get them? This patch is finished for the icons that we were provided with.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8613030 [details] [diff] [review]
WIP

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

I'm fine with doing a good part of it before we have the icons for the rest of it, but this patch has some issues.

I am suspicious about not having to size any of these icons. Did you test this patch? Do you know why sizing the icons is not necessary?

::: browser/themes/windows/browser.css
@@ +578,5 @@
>  
> +@media (min-resolution: 1.1dppx) {
> +  .bookmark-item[container] {
> +    list-style-image: url("chrome://global/skin/icons/[e-mail]");
> +    -moz-image-region: rect(0, 64px, 32px, 32px);

This region is going to apply to all the livemark/query files in use, isn't it? But those are only 32x32 pixels. So I'm confused how this worked in your testing. Did you test the various different bookmark types ?

@@ +586,5 @@
> +    -moz-image-region: rect(32px, 64px, 64px, 32px);
> +  }
> +
> +  .bookmark-item[container][livemark] {
> +    list-style-image: url("chrome://browser/skin/[e-mail]");

In other words, this

@@ +590,5 @@
> +    list-style-image: url("chrome://browser/skin/[e-mail]");
> +  }
> +
> +  .bookmark-item[container][query] {
> +    list-style-image: url("chrome://browser/skin/places/[e-mail]");

and this should both define a -moz-image-region to override the one defined for the plain container

::: browser/themes/windows/jar.mn
@@ +574,5 @@
>  
>  % override chrome://browser/skin/Info.png                             chrome://browser/skin/Info-XP.png                                 os=WINNT osversion<6
>  % override chrome://browser/skin/identity.png                         chrome://browser/skin/identity-XP.png                             os=WINNT osversion<6
>  % override chrome://browser/skin/livemark-folder.png                  chrome://browser/skin/livemark-folder-XP.png                      os=WINNT osversion<6
> +% override chrome://browser/skin/[e-mail]               chrome://browser/skin/[e-mail]                   os=WINNT osversion<6

You've not actually added this file to jar.mn.

::: browser/themes/windows/places/places.css
@@ +91,5 @@
> +    -moz-image-region: rect(32px, 64px, 64px, 32px);
> +  }
> +
> +  treechildren::-moz-tree-image(title, container, livemark) {
> +    list-style-image: url("chrome://browser/skin/[e-mail]");

This too has a moz-region problem, I think.

@@ +182,5 @@
> +    -moz-image-region: rect(0, 64px, 32px, 32px);
> +  }
> +
> +  treechildren::-moz-tree-image(title, query, hostContainer, open) {
> +    list-style-image: url("chrome://global/skin/icons/[e-mail]");

Whereas I don't think this needs the identical list-style-image. Hm, except that it seems that the non-hidpi version has an image specified. Can you check if that's really necessary? From how some of the other rules are meant to be overriding each other, I think you can remove it there and then leave this one out, too.

(Please make that change cross-platform if I'm right.)

@@ +186,5 @@
> +    list-style-image: url("chrome://global/skin/icons/[e-mail]");
> +    -moz-image-region: rect(32px, 64px, 64px, 32px);
> +  }
> +
> +  treechildren::-moz-tree-image(title, query, folder) {

This rule has the same contents as the 2nd one in the media query. Please just comma-separate the selectors there.

@@ +192,5 @@
> +    -moz-image-region: rect(0, 64px, 32px, 32px);
> +  }
> +
> +  treechildren::-moz-tree-image(title, query, folder, open) {
> +    -moz-image-region: rect(32px, 64px, 64px, 32px);

And this one would have the same one as the hostContainer version if it didn't respecify the list-style-image.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Closing old needinfo request...

Comment 6

[Marco Bonardo [:mak]]

doesn't apply to Photon.