The search icon in the search input field for Windows and Linux should match the spec

VERIFIED FIXED in Firefox 56

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: hani.yacoub, Assigned: rickychien)

Tracking

(Blocks: 1 bug)

55 Branch
Firefox 56
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox53 unaffected, firefox54 unaffected, firefox55 disabled, firefox56 verified)

Details

(Whiteboard: [photon-preference])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8873740 [details]
search icon.png

[Affected versions]: 
Nightly 55.0a1

[Affected platforms]:
Platforms: Ubuntu 16.10

[Steps to reproduce]:
1. Launch Firefox, go to about:config and search for "browser.preferences.search" and set it value to true.
2. Go to "about:preferences".
3. Observe the search icon.

[Expected result]:
The search icon in the search input field for Windows and Linux should match the spec.

[Actual result]:
The search icon in the search input field for Windows and Linux doesn't match the spec.
(Reporter)

Updated

2 years ago
Blocks: 1357285
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [photon-preference]
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
QA Contact: hani.yacoub
Whiteboard: [photon-preference] → [photon-preference][triage]
Comment hidden (mozreview-request)
Mike, could you take at look for this? thanks
Comment hidden (mozreview-request)
Dao, this is a regression caused by bug 1360491 due to lacking Window + Linux styling support. Could you take a look? Thanks
Whiteboard: [photon-preference][triage] → [photon-preference]

Comment 5

2 years ago
mozreview-review
Comment on attachment 8873779 [details]
Bug 1369640 - Flip search icon for Windows and Linux

https://reviewboard.mozilla.org/r/145202/#review150186

::: toolkit/themes/shared/icons/search-textbox.svg:7
(Diff revision 2)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> -<svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12">
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="12" height="12" viewBox="0 0 12 12">
>    <style>
> -    path {
> +    use:not(:target) {

:not(:target) shouldn't be used anymore. See bug 1358998.
Attachment #8873779 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thanks for bringing this up. I've updated my patch and split svg to individual files for platforms. You can check that again.

Comment 10

2 years ago
mozreview-review
Comment on attachment 8873779 [details]
Bug 1369640 - Flip search icon for Windows and Linux

https://reviewboard.mozilla.org/r/145202/#review150636

::: toolkit/themes/linux/global/icons/search-textbox.svg:5
(Diff revision 5)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="12" height="12" viewBox="0 0 12 12">

nit: remove xmlns:xlink="http://www.w3.org/1999/xlink"

::: toolkit/themes/linux/global/icons/search-textbox.svg:9
(Diff revision 5)
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="12" height="12" viewBox="0 0 12 12">
> +  <style>
> +    path {
> +      fill: #939393;
> +      fill-rule: evenodd;

Please set fill="#939393" and fill-rule="evenodd" directly on the path element and get rid of <style>.

::: toolkit/themes/linux/global/icons/search-textbox.svg:12
(Diff revision 5)
> +    path {
> +      fill: #939393;
> +      fill-rule: evenodd;
> +    }
> +  </style>
> +  <path d="M11.354,10.646l-0.707.707L7.295,8A4.483,4.483,0,1,1,9,4.5,4.458,4.458,0,0,1,8,7.295ZM4.5,1A3.5,3.5,0,1,0,8,4.5,3.5,3.5,0,0,0,4.5,1Z" transform="scale(-1, 1) translate(-12, 0)"/>

Please set the mirrored path data directly instead of the transform.
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8873779 [details]
Bug 1369640 - Flip search icon for Windows and Linux

https://reviewboard.mozilla.org/r/145202/#review150636

> Please set the mirrored path data directly instead of the transform.

Dao, I've updated my patch. However, after investigating and tring some SVO tools, I cannot figure out how to optimze and remove transform in order to set the mirrored path data directly.

The tool which designer used will try to apply transform like this way, so I think it's hard for doing that.

Do you think it could cause obvious performance issue? Or do you have any nice tool / mechanism to optimize this?

Comment 13

2 years ago
mozreview-review
Comment on attachment 8873779 [details]
Bug 1369640 - Flip search icon for Windows and Linux

https://reviewboard.mozilla.org/r/145202/#review150658

::: toolkit/themes/linux/global/icons/search-textbox.svg:6
(Diff revision 6)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12">
> +  <path fill="#939393" fill-rule="evenodd" d="M11.354,10.646l-0.707.707L7.295,8A4.483,4.483,0,1,1,9,4.5,4.458,4.458,0,0,1,8,7.295ZM4.5,1A3.5,3.5,0,1,0,8,4.5,3.5,3.5,0,0,0,4.5,1Z" transform="scale(-1, 1) translate(-12, 0)"/>

Please set the mirrored path data directly in the d attribute instead of the transform.
(In reply to Ricky Chien [:rickychien] from comment #12)
> Comment on attachment 8873779 [details]
> Bug 1369640 - Flip search icon for Windows and Linux
> 
> https://reviewboard.mozilla.org/r/145202/#review150636
> 
> > Please set the mirrored path data directly instead of the transform.
> 
> Dao, I've updated my patch. However, after investigating and tring some SVO
> tools, I cannot figure out how to optimze and remove transform in order to
> set the mirrored path data directly.
> 
> The tool which designer used will try to apply transform like this way, so I
> think it's hard for doing that.
> 
> Do you think it could cause obvious performance issue?

It could potentially perform worse. jwatt is working on a faster SVG subset and it's not clear yet whether that will support transform (bug 1353771).

> Or do you have any
> nice tool / mechanism to optimize this?

Maybe SVGO can do this?
I tried https://github.com/svg/svgo with transformsWithOnePath and tried https://jakearchibald.github.io/svgomg as well. They cannot get rid of transform and former even break the SVG icon.

I'm tot very sure whether I checked the wrong options but feel free to try them yourself :)
Flags: needinfo?(dao+bmo)
Have you disabled the removeDimensions optimization? See https://github.com/ben-eb/gulp-svgmin/issues/23#issuecomment-247536043
Flags: needinfo?(dao+bmo)
I just tried that, the optimized icon is still broken.
Flags: needinfo?(dao+bmo)
After investigating SVGO [1], the only option can remove transform attribute is "convertPathData" [2]. Unfortunately, the options will break our SVG since not all svg optimization plugins are reliable and do not guarantee it won't break the path. Here is a good article refers to that problem [3].

Dao, I know doing SVG matrix transformation will be a high cost in painting. Do you think it's safe to land if it doesn't make a big impact on overall performance?

[1] https://github.com/svg/svgo
[2] https://github.com/svg/svgo#what-it-can-do
[3] https://hacks.mozilla.org/2015/03/optimising-svg-images/
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8873779 [details]
Bug 1369640 - Flip search icon for Windows and Linux

https://reviewboard.mozilla.org/r/145202/#review150716

::: toolkit/themes/osx/global/jar.mn:105
(Diff revision 7)
>    skin/classic/global/icons/error-16.png                             (icons/error-16.png)
>    skin/classic/global/icons/error-64.png                             (icons/error-64.png)
>    skin/classic/global/icons/question-16.png                          (icons/question-16.png)
>    skin/classic/global/icons/question-32.png                          (icons/question-32.png)
>    skin/classic/global/icons/question-64.png                          (icons/question-64.png)
> +  skin/classic/global/icons/search-textbox.svg                       (icons/search-textbox.svg)

This should probably be in toolkit/themes/shared/non-mac.jar.inc.mn instead.
Attachment #8873779 - Flags: review?(dao+bmo) → review+
Thanks for the review!

I'm not very clear. Could you tell me what's your instruction / what you want me to do in your last comment?


Should I copy following message from toolkit/themes/osx/global/jar.mn

skin/classic/global/icons/search-textbox.svg                       (icons/search-textbox.svg)

to toolkit/themes/shared/non-mac.jar.inc.mn [1] and become

skin/classic/global/icons/search-textbox.svg                       (../../osx/global/icons/search-textbox.svg)


All paths in [1] come from windows/, so it would be weird if we copy it to non-mac.jar.inc.mn.

On the other hand, I'm still not clear what's purpose of non-mac.jar...


[1] https://searchfox.org/mozilla-central/source/toolkit/themes/shared/non-mac.jar.inc.mn
Flags: needinfo?(dao+bmo)
non-mac.jar.inc.mn basically is for files shared between the Windows and Linux themes. As far as I can tell, search-textbox.svg is the same in both themes.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Comment on attachment 8873779 [details]
Bug 1369640 - Flip search icon for Windows and Linux

Nice trick with using non-mac.jar.inc.mn!

Dao, could you confirm my patch to make sure I'm following your expectation?

thanks!
Attachment #8873779 - Flags: feedback?(dao+bmo)
Comment on attachment 8873779 [details]
Bug 1369640 - Flip search icon for Windows and Linux

I had commented in the wrong file from your earlier patch. The search-textbox.svg entry should be in toolkit/themes/osx/global/jar.mn and non-mac.jar.inc.mn. toolkit/themes/shared/icons/search-textbox.svg should move to toolkit/themes/osx/global/icons/search-textbox.svg.
Attachment #8873779 - Flags: feedback?(dao+bmo) → feedback-
Comment hidden (mozreview-request)
Comment on attachment 8873779 [details]
Bug 1369640 - Flip search icon for Windows and Linux

Thanks for clarifying. Patch has been updated to match your requirement on comment 25.

I've verified the search icon is displayed properly on Mac and Windows.

Dao, could you kindly review my patch again? Thanks
Attachment #8873779 - Flags: feedback?(dao+bmo)
Attachment #8873779 - Flags: feedback?(dao+bmo) → feedback+

Comment 28

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ea31832ab86
Flip search icon for Windows and Linux r=dao

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0ea31832ab86
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Search is disabled in 55.
status-firefox55: affected → wontfix
(Reporter)

Comment 31

2 years ago
Build ID: 20170614030206
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Verified as fixed on Firefox Nightly 56.0a1 on Windows 10 x 64 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox55: wontfix → affected
status-firefox56: fixed → verified
browser.preferences.search is off in 55
status-firefox55: affected → disabled
You need to log in before you can comment on or make changes to this bug.