1280135 - The search indicator in Linux uses a low rez version on hidpi displays

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Eitan Isaacson [:eeejay]]

The very attractive search indicator icon is fuzzy in Linux!

There isn't a good reason for this since the images are not platform dependent, so it is just a linux css theme tweak.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1280135 - Make hidpi version of search-indicator icon work on Linux.

Review commit: https://reviewboard.mozilla.org/r/59262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59262/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8762771 [details]
Bug 1280135 - Make hidpi version of search-indicator icon work on Linux.

https://reviewboard.mozilla.org/r/59262/#review56310

Annnnd you can guess my feedback by now... can we unify this? Doesn't feel like it makes sense to duplicate this CSS 3 times over. Please request the next review for this particular patch from :florian.

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8762771 [details]
> Bug 1280135 - Make hidpi version of search-indicator icon work on Linux.
> 
> https://reviewboard.mozilla.org/r/59262/#review56310
> 
> Annnnd you can guess my feedback by now... can we unify this?

Theoretically, yes? There is no shared css for this piece. Maybe there should be becuase there is a lot of style in common across the 3 platforms. But I think it is beyond the scope of this bug.

Comment 4

[Eitan Isaacson [:eeejay]]

Comment on attachment 8762771 [details]
Bug 1280135 - Make hidpi version of search-indicator icon work on Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59262/diff/1-2/

Comment 5

[Florian Quèze [:florian]]

(In reply to Eitan Isaacson [:eeejay] from comment #3)

> > Annnnd you can guess my feedback by now... can we unify this?
> 
> Theoretically, yes? There is no shared css for this piece. Maybe there
> should be becuase there is a lot of style in common across the 3 platforms.
> But I think it is beyond the scope of this bug.

There was some attempt to unify in bug 1106569, but it never got finished because that bug ended up mixing refactoring/merging with an actual bugfix and it got confusing. Yes, this duplicated code should be unified, but let's do this change in its own bug.

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8762771 [details]
Bug 1280135 - Make hidpi version of search-indicator icon work on Linux.

https://reviewboard.mozilla.org/r/59262/#review57054

Looks good to me by code inspection (I don't have access to a hidpi Linux machine), just one comment that I think should be addressed.

::: browser/themes/linux/searchbar.css:89
(Diff revision 2)
>  
>  .searchbar-search-button:hover:active {
>    -moz-image-region: rect(0, 60px, 20px, 40px);
>  }
>  
> +@media (min-resolution: 1.1dppx) {

It looks to me like you also want to include in your patch this section of the Windows CSS file:
http://searchfox.org/mozilla-central/source/browser/themes/windows/searchbar.css#249

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 8762771 [details]
Bug 1280135 - Make hidpi version of search-indicator icon work on Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59262/diff/2-3/

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8762771 [details]
Bug 1280135 - Make hidpi version of search-indicator icon work on Linux.

https://reviewboard.mozilla.org/r/59262/#review59216

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/450e5d0b3df0
Make hidpi version of search-indicator icon work on Linux. r=florian

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/450e5d0b3df0