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

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Preferences
P1
normal
VERIFIED FIXED
5 months ago
4 months 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])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

5 months 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

5 months ago
Blocks: 1357285
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [photon-preference]
(Assignee)

Updated

5 months ago
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)
(Assignee)

Comment 2

5 months ago
Mike, could you take at look for this? thanks
Comment hidden (mozreview-request)
(Assignee)

Comment 4

5 months ago
Dao, this is a regression caused by bug 1360491 due to lacking Window + Linux styling support. Could you take a look? Thanks

Updated

4 months ago
Whiteboard: [photon-preference][triage] → [photon-preference]

Comment 5

4 months 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)
(Assignee)

Comment 9

4 months ago
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

4 months 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

4 months 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

4 months 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?
(Assignee)

Comment 15

4 months ago
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)
(Assignee)

Comment 17

4 months ago
I just tried that, the optimized icon is still broken.
(Assignee)

Updated

4 months ago
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 18

4 months ago
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/
(Assignee)

Updated

4 months ago
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)

Comment 20

4 months 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+
(Assignee)

Comment 21

4 months ago
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)
(Assignee)

Comment 24

4 months ago
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)
(Assignee)

Comment 27

4 months ago
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)

Updated

4 months ago
Attachment #8873779 - Flags: feedback?(dao+bmo) → feedback+

Comment 28

4 months 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

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

Comment 31

4 months 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.