1168585 - The search dropdown and icons within the Search panel are blurry/pixelated on Windows HiDPI

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Comment 1

[Matthew N. [:MattN]]

Likely a dupe of bug 1163559 and not Windows-specific.

Comment 2

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

Not a dupe because it covers other icons in the search UI outside of the search engine icons.

Comment 3

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

Attachment: Patch

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8611363 [details] [diff] [review]
Patch

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

::: browser/themes/windows/searchbar.css
@@ +7,5 @@
>  }
>  
>  .searchbar-dropmarker-image {
>    --searchbar-dropmarker-url: url("chrome://browser/skin/searchbar-dropdown-arrow.png");
> +  --searchbar-dropmarker-2x-url: url("chrome://browser/skin/[e-mail]");

Dumb idea, I'm sure, but... couldn't we do:

@media (min-resolution: 1.1dppx) {
  .searchbar-dropmarker-image {
    --searchbar-dropmarker-url: url("chrome://browser/skin/[e-mail]");
  }
}

And ditto for the other images? Seems neater. :-)

@@ +137,5 @@
> +  }
> +
> +  .search-go-button {
> +    list-style-image: url("chrome://browser/skin/[e-mail]");
> +    width: 14px;

We already margin-specify these to death; can we specify the width in the same place, please?

@@ +161,5 @@
> +
> +  searchbar[oneoffui] .search-go-button {
> +    list-style-image: url("chrome://browser/skin/[e-mail]");
> +    -moz-image-region: rect(0, 84px, 28px, 56px);
> +    width: 14px;

No need to specify width: a second time here.

@@ +280,5 @@
>  .addengine-badge {
>    width: 16px;
>    height: 16px;
>    margin: -7px -9px 7px 9px;
> +  list-style-image: url("chrome://browser/skin/0.png");

This seems wrong.

Comment 5

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

Attachment: Patch v1.1

I removed the search-indicator-add-engine graphic from the tree since it's unused by any theme.

(In reply to :Gijs Kruitbosch from comment #4)
> Dumb idea, I'm sure, but... couldn't we do:
> 
> @media (min-resolution: 1.1dppx) {
>   .searchbar-dropmarker-image {
>     --searchbar-dropmarker-url:
> url("chrome://browser/skin/[e-mail]");
>   }
> }

Not a dumb idea. I agree that we should do something to clean that up, but this affects devedition which also uses these variables with SVG icons. We should handle that in a follow-up due to the coordination with the devtools people.

> @@ +161,5 @@
> > +
> > +  searchbar[oneoffui] .search-go-button {
> > +    list-style-image: url("chrome://browser/skin/[e-mail]");
> > +    -moz-image-region: rect(0, 84px, 28px, 56px);
> > +    width: 14px;
> 
> No need to specify width: a second time here.

width needs to be specified again for the oneoffsearch UI because it uses 14px in oneoffsearch and 16px in the old UI.

> @@ +280,5 @@
> >  .addengine-badge {
> >    width: 16px;
> >    height: 16px;
> >    margin: -7px -9px 7px 9px;
> > +  list-style-image: url("chrome://browser/skin/0.png");
> 
> This seems wrong.

Ouch, don't know how that got in there. Reverted.

Comment 6

[:Gijs (out for now; he/him)]

Comment on attachment 8611404 [details] [diff] [review]
Patch v1.1

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

r+ with widths moved and the one question answered.

::: browser/themes/windows/searchbar.css
@@ +111,5 @@
>  
>  searchbar[oneoffui] .search-go-button {
>    list-style-image: url("chrome://browser/skin/reload-stop-go.png");
>    -moz-image-region: rect(0, 42px, 14px, 28px);
> +  width: 14px;

This is interesting because the width was in there twice in the previous patch, both as 14px. (for .search-go-button and searchbar[oneoffui] .search-go-button).

It looks like now you no longer change the search-go-button in the non-oneoffui case? Is that intentional?

@@ +133,5 @@
> +  }
> +
> +  .searchbar-dropmarker-image {
> +    list-style-image: var(--searchbar-dropmarker-2x-url);
> +    width: 7px;

I'm sorry, I should have been more specific. I kinda meant all of these width: specifications. I suppose I'm potentially *too* paranoid, but I would feel happier if we specified the width everywhere, including on lodpi.

Comment 7

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

Attachment: Patch v1.2

(In reply to :Gijs Kruitbosch from comment #6)
> ::: browser/themes/windows/searchbar.css
> @@ +111,5 @@
> >  
> >  searchbar[oneoffui] .search-go-button {
> >    list-style-image: url("chrome://browser/skin/reload-stop-go.png");
> >    -moz-image-region: rect(0, 42px, 14px, 28px);
> > +  width: 14px;
> 
> This is interesting because the width was in there twice in the previous
> patch, both as 14px. (for .search-go-button and searchbar[oneoffui]
> .search-go-button).
> 
> It looks like now you no longer change the search-go-button in the
> non-oneoffui case? Is that intentional?

Yeah, that was an intentional change in this latest patch. I realized that we were missing the HiDPI icon for the non-oneoffui, and as the oneoffui is the future I'm not concerned about us not having that icon.
 
> @@ +133,5 @@
> > +  }
> > +
> > +  .searchbar-dropmarker-image {
> > +    list-style-image: var(--searchbar-dropmarker-2x-url);
> > +    width: 7px;
> 
> I'm sorry, I should have been more specific. I kinda meant all of these
> width: specifications. I suppose I'm potentially *too* paranoid, but I would
> feel happier if we specified the width everywhere, including on lodpi.

I've moved the rest in my latest patch.

Comment 8

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

Attachment: Patch v1.2 (qref'd)

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cb827b44f564

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb827b44f564

Comment 11

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

Attachment: Patch for 40

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

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8627205 [details] [diff] [review]
Patch for 40

Visual fix in support of Windows 10. On m-c for more than a month. Beta+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1ff083905682