Closed Bug 1369640 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Version:
55 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- disabled
firefox56 --- verified

People

(Reporter: hyacoub, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(2 files)

search icon.png
3.08 KB, image/png
Details
Bug 1369640 - Flip search icon for Windows and Linux
59 bytes, text/x-review-board-request
dao
: review+
dao
: feedback+
Details
Attached image 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.
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]
Mike, could you take at look for this? thanks
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 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-
Thanks for bringing this up. I've updated my patch and split svg to individual files for platforms. You can check that again.
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 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 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 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 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 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+
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0ea31832ab86 Flip search icon for Windows and Linux r=dao
https://hg.mozilla.org/mozilla-central/rev/0ea31832ab86
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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.

Attachment

General

Creator:
Created:
Updated:
Size: