Closed
Bug 1403359
Opened 7 years ago
Closed 7 years ago
Update search bar icon to photon icon
Categories
(Firefox :: Theme, defect, P1)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 58
People
(Reporter: daleharvey, Assigned: daleharvey)
References
(Blocks 1 open bug)
Details
(Whiteboard: [reserve-photon-visual])
Attachments
(3 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
dao
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
109.73 KB,
image/png
|
Details | |
538 bytes,
image/svg+xml
|
Details |
Follow on from https://bugzilla.mozilla.org/show_bug.cgi?id=1399642
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dharvey
Updated•7 years ago
|
Status: NEW → ASSIGNED
Flags: qe-verify-
Assignee | ||
Comment 1•7 years ago
|
||
So http://searchfox.org/mozilla-central/source/browser/themes/shared/search/search-indicator.png is not consistent with the search icon used throughout the rest of UI (https://github.com/mozilla/gecko-dev/blob/inbound/browser/themes/shared/icons/search-glass.svg)
Especially if its for 57 we are probably going to want to replace the image as is, Stephen is this something we are gonna to get in for 57, anyone able to give me an icon?
Flags: needinfo?(shorlander)
Comment 2•7 years ago
|
||
We should probably use search-glass.svg here and overlay it with a dropdown icon for the hover state. Same for search-indicator-badge-add.png.
Assignee | ||
Comment 3•7 years ago
|
||
ok yeh I was wondering if we should do it that way, seems a little riskier for uplift but I can do a patch for it, is there all ready a drop down icon available to use?
Comment 4•7 years ago
|
||
There's arrow-dropdown-12.svg but that's probably still too big for this.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
It scales down reasonably (I used 6px to make it half), also the search icon itself is getting scaled down here so not sure if we want new assets for both of these or to use svg scaling in both, put up a version so can see what it looks like and really wasnt sure about the best implementation so figured I would get some feedback
Attachment #8912749 -
Flags: feedback?(dao+bmo)
Comment 7•7 years ago
|
||
Comment on attachment 8912749 [details]
Screen Shot 2017-09-27 at 16.29.01.png
(In reply to Dale Harvey (:daleharvey) from comment #6)
> Created attachment 8912749 [details]
> Screen Shot 2017-09-27 at 16.29.01.png
>
> It scales down reasonably (I used 6px to make it half),
Works for me.
> also the search icon itself is getting scaled down here
Why?
Attachment #8912749 -
Flags: feedback?(dao+bmo)
Assignee | ||
Comment 8•7 years ago
|
||
> Why?
I was matching it to the original search icon which was 14px, but since its being used in the url bar at 16px it makes sense keep that consistent in fact
Assignee | ||
Comment 9•7 years ago
|
||
Hey Stephen, so to update the needinfo, https://bug1403359.bmoattachments.org/attachment.cgi?id=8912749 is what it looks like using search-glass.svg and arrow-dropdown-12.svg, so for this bug I need at the least a standalone copy of the (+) from http://searchfox.org/mozilla-central/source/browser/themes/shared/search/search-indicator-badge-add.png at the correct size (and an arrow-dropdown if you dont want that scaled)
Cheers
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review189702
::: browser/themes/osx/searchbar.css:29
(Diff revision 1)
> }
>
> .search-go-container,
> .searchbar-search-button-container {
> -moz-box-align: center;
> + fill: #999999;
It would be better to use fill: currentColor; + different fill-opacities to match the urlbar icons
Comment 11•7 years ago
|
||
Flags: needinfo?(shorlander)
Comment hidden (duplicate) |
Comment hidden (duplicate) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review190166
::: browser/themes/shared/search/searchbar.inc.css:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
How about putting this stuff in urlbar-searchbar.inc.css instead of a separate new file?
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review190278
::: browser/components/search/content/search.xml:59
(Diff revision 4)
> <xul:hbox class="searchbar-search-button-container">
> <xul:image class="searchbar-search-button"
> anonid="searchbar-search-button"
> xbl:inherits="addengines"
> tooltiptext="&searchIcon.tooltip;"/>
> + <xul:image class="search-go-button-dropdown"/>
Can you rename this? It doesn't seem to have anything to do with the go button.
::: browser/components/search/content/search.xml:67
(Diff revision 4)
> <xul:hbox class="search-go-container">
> <xul:image class="search-go-button urlbar-icon" hidden="true"
> anonid="search-go-button"
> onclick="handleSearchCommand(event);"
> tooltiptext="&contentSearchSubmit.tooltip;"/>
> + <xul:image class="search-go-button-dropdown" hidden="true"/>
What's the point of this element?
::: toolkit/themes/shared/jar.inc.mn:46
(Diff revision 4)
> skin/classic/global/icons/spinner-arrow-down.svg (../../shared/icons/spinner-arrow-down.svg)
> skin/classic/global/icons/spinner-arrow-up.svg (../../shared/icons/spinner-arrow-up.svg)
> skin/classic/global/icons/arrow-dropdown-12.svg (../../shared/icons/arrow-dropdown-12.svg)
> skin/classic/global/icons/arrow-dropdown-16.svg (../../shared/icons/arrow-dropdown-16.svg)
> skin/classic/global/icons/warning.svg (../../shared/icons/warning.svg)
> + skin/classic/global/icons/search-indicator-badge-add.svg (../../shared/icons/search-indicator-badge-add.svg)
Please move this to this block:
http://searchfox.org/mozilla-central/rev/f2b181af9497af258d96213f2f0cfccc08740a86/browser/themes/shared/jar.inc.mn#180-183,185-191
Attachment #8912747 -
Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review190316
Looks pretty good overall, but some issues remain.
::: browser/themes/shared/urlbar-searchbar.inc.css:369
(Diff revision 5)
> +
> +.search-go-container,
> +.searchbar-search-button-container {
> + -moz-box-align: center;
> + fill: currentColor;
> + opacity: .4;
search-go-container already has fill and the right opacity set. The above makes it too transparent.
::: browser/themes/shared/urlbar-searchbar.inc.css:379
(Diff revision 5)
> + -moz-context-properties: fill;
> + margin-inline-start: 5px;
> +}
> +
> +.searchbar-search-button-overlay {
> + visibility: hidden;
Please use display: none; so that we don't needlessly load arrow-dropdown-12.svg on startup
::: browser/themes/shared/urlbar-searchbar.inc.css:380
(Diff revision 5)
> + margin-inline-start: 5px;
> +}
> +
> +.searchbar-search-button-overlay {
> + visibility: hidden;
> + pointer-events: none;
pointer-events: none; seems correct here, but in my testing the button still doesn't seem to respond to some clicks. Can you please double-check if you broke something here?
::: browser/themes/shared/urlbar-searchbar.inc.css:399
(Diff revision 5)
> + margin-top: -10px;
> + width: 11px;
> + height: 11px;
> +}
> +
> +.searchbar-search-button-container:hover .searchbar-search-button-overlay {
Please use the child selector
Attachment #8912747 -
Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
So I have found one weird issue with this, if I set .searchbar-search-button { opacity: .4; } then the magnifying glass renders strangely with the indicator-add (will attach a screenshot). I either need to set the fill-opacity on the magnifying glass which means the indicator or dropdown dont react to hover, or I need to set the opacity on the .searchbar-search-button-container, which means I need to change what clicks the drop down reacts to
I chose the former for now since it was the smaller change, but can change the hover + mouse clicks reacting to .searchbar-search-button-container if you want, that seems slightly cleaner imo
Assignee | ||
Comment 23•7 years ago
|
||
Comment 24•7 years ago
|
||
Have you tried putting a lower z-index for the search icon?
Assignee | ||
Comment 25•7 years ago
|
||
I did, the overlay comes later in the dom and has no opacity (nor inherits any, or has any in its svg fill), so it should effect the magnifying glass but it still seems to
Either way we wouldnt be able to set the opacity on the indicator seperately anyway as then the magnifying glass will see though which we dont want, so either need the indicator to not react, or to set opacity on the container (or to give the indicator context-fill and hard code the hover color shift)
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review190574
::: browser/themes/shared/urlbar-searchbar.inc.css:380
(Diff revision 6)
> +}
> +
> +.searchbar-search-button-overlay {
> + visibility: hidden;
> + pointer-events: none;
> + -moz-context-properties: fill;
You should probably set this only for the dropdown case, not the badge case.
Assignee | ||
Comment 27•7 years ago
|
||
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
I am going to move the hover and the onclick to the container for this instead, feels a lot cleaner
Attachment #8912747 -
Flags: review?(dao+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•7 years ago
|
||
I feel like this is cleaner and less brittle, only thing I am not so sure about is the
.searchbar-search-button:hover:not([addengines=true])
Are we supposed to avoid not() selectors? I could put an !important on the [addengines=true] version, or add an extra rule for
.searchbar-search-button[addengines=true]:hover
So its specificity doesnt get overridden, the not() seems cleanest if its not a performance issue
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review191346
::: browser/components/search/content/search.xml:58
(Diff revision 7)
> - <xul:hbox class="searchbar-search-button-container">
> + <xul:hbox class="searchbar-search-button"
> - <xul:image class="searchbar-search-button"
> - anonid="searchbar-search-button"
> + anonid="searchbar-search-button"
> - xbl:inherits="addengines"
> + xbl:inherits="addengines"
> - tooltiptext="&searchIcon.tooltip;"/>
> + tooltiptext="&searchIcon.tooltip;">
> + <xul:image class="searchbar-search-button-glass" />
nit: rename this to searchbar-search-icon
::: browser/components/search/content/search.xml:59
(Diff revision 7)
> - <xul:image class="searchbar-search-button"
> - anonid="searchbar-search-button"
> + anonid="searchbar-search-button"
> - xbl:inherits="addengines"
> + xbl:inherits="addengines"
> - tooltiptext="&searchIcon.tooltip;"/>
> + tooltiptext="&searchIcon.tooltip;">
> + <xul:image class="searchbar-search-button-glass" />
> + <xul:image class="searchbar-search-button-overlay"/>
nit: rename this to searchbar-search-icon-overlay
::: browser/components/uitour/UITour.jsm:188
(Diff revision 7)
> ["searchIcon", {
> query: (aDocument) => {
> let searchbar = aDocument.getElementById("searchbar");
> return aDocument.getAnonymousElementByAttribute(searchbar,
> "anonid",
> - "searchbar-search-button");
> + "searchbar-search-button-container");
This looks wrong
::: browser/themes/shared/urlbar-searchbar.inc.css:373
(Diff revision 7)
> +/* Search bar */
> +
> +.searchbar-search-button {
> + -moz-box-align: center;
> + fill: currentColor;
> + opacity: .4;
We need full opacity for the "add engine" badge
Attachment #8912747 -
Flags: review?(dao+bmo) → review-
Assignee | ||
Comment 31•7 years ago
|
||
> We need full opacity for the "add engine" badge
https://bug1403359.bmoattachments.org/attachment.cgi?id=8914305 is what happens if we only set the opacity on .searchbar-search-button-glass, I was able to work around that by setting fill-opacity on the magnifying glass however http://searchfox.org/mozilla-central/source/browser/themes/shared/search/search-indicator-badge-add.png does have seperate hover and active states for the add engine badge, are you saying we should remove that or give them specific context-fills?
Comment 32•7 years ago
|
||
(In reply to Dale Harvey (:daleharvey) from comment #31)
> http://searchfox.org/mozilla-central/source/browser/themes/shared/search/
> search-indicator-badge-add.png does have seperate hover and active states
> for the add engine badge,
I think we could get rid of this. In any case, a 40% default opacity is too low for the badge.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•7 years ago
|
||
Heh that was the behaviour on the previous PR I cancelled, but the code is still cleaner this way, now the overlay doesnt react to hover / active but the magnifying glass does
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review191362
::: browser/themes/shared/urlbar-searchbar.inc.css:404
(Diff revision 8)
> + width: 11px;
> + height: 11px;
> +}
> +
> +.searchbar-search-button:hover:not([addengines=true]) > .searchbar-search-icon-overlay {
> + list-style-image: url(chrome://global/skin/icons/arrow-dropdown-12.svg);
Need to set -moz-context-properties for this
Attachment #8912747 -
Flags: review?(dao+bmo) → review-
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review191364
::: browser/themes/shared/urlbar-searchbar.inc.css:377
(Diff revision 8)
> + fill: currentColor;
> +}
> +
> +.searchbar-search-icon {
> + list-style-image: url(chrome://browser/skin/search-glass.svg);
> + pointer-events: none;
Why are you setting pointer-events: none for this?
Comment hidden (mozreview-request) |
Comment 38•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review191794
::: browser/themes/shared/urlbar-searchbar.inc.css:389
(Diff revision 9)
> + visibility: hidden;
> + pointer-events: none;
> + margin-inline-start: -2px;
> + margin-inline-end: 2px;
> + width: 6px;
> + height: 6px;
It's pretty confusing that you let this element take up space even when it's invisible. Can you please:
1. Move width/height to where you're setting arrow-dropdown-12.svg
2. make sure that margin-inline-start + margin-inline-end == -1 * width so that the overlay never affects the spacing
Attachment #8912747 -
Flags: review?(dao+bmo)
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review191798
::: browser/themes/shared/urlbar-searchbar.inc.css:379
(Diff revision 9)
> +
> +.searchbar-search-icon {
> + list-style-image: url(chrome://browser/skin/search-glass.svg);
> + pointer-events: none;
> + -moz-context-properties: fill, fill-opacity;
> + margin-inline-start: 5px;
Oh, and let's make sure this start margin matches the location bar. Currently it looks like it's a bit smaller.
Assignee | ||
Comment 40•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review191364
> Why are you setting pointer-events: none for this?
Ths code that handles the mouse event uses the original target (http://searchfox.org/mozilla-central/source/browser/components/search/content/search.xml#552), it needs to be on the container so the mouse clicks correspond to the hover states so ignoring the icon element seemed cleanest
Comment hidden (mozreview-request) |
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review191848
::: browser/components/search/content/search.xml:58
(Diff revision 10)
> - <xul:hbox class="searchbar-search-button-container">
> + <xul:hbox class="searchbar-search-button"
> - <xul:image class="searchbar-search-button"
> - anonid="searchbar-search-button"
> + anonid="searchbar-search-button"
> - xbl:inherits="addengines"
> + xbl:inherits="addengines"
> - tooltiptext="&searchIcon.tooltip;"/>
> + tooltiptext="&searchIcon.tooltip;">
> + <xul:image class="searchbar-search-icon" />
nit: remove space before />
::: browser/themes/shared/jar.inc.mn:189
(Diff revision 10)
> - skin/classic/browser/search-indicator-badge-add.png (../shared/search/search-indicator-badge-add.png)
> - skin/classic/browser/search-indicator-badge-add@2x.png (../shared/search/search-indicator-badge-add@2x.png)
> skin/classic/browser/search-indicator-magnifying-glass.svg (../shared/search/search-indicator-magnifying-glass.svg)
> skin/classic/browser/search-arrow-go.svg (../shared/search/search-arrow-go.svg)
> skin/classic/browser/gear.svg (../shared/search/gear.svg)
> + skin/classic/browser/search-indicator-badge-add.svg (../shared/search/search-indicator-badge-add.svg)
nit: fix indent
::: browser/themes/shared/urlbar-searchbar.inc.css:378
(Diff revision 10)
> +}
> +
> +.searchbar-search-icon {
> + list-style-image: url(chrome://browser/skin/search-glass.svg);
> + pointer-events: none;
> + -moz-context-properties: fill, fill-opacity;
nit: move -moz-context-properties right after list-style-image
::: browser/themes/shared/urlbar-searchbar.inc.css:385
(Diff revision 10)
> + margin-inline-end: 6px;
> + fill-opacity: .4;
> +}
> +
> +.searchbar-search-icon-overlay {
> + visibility: hidden;
This is a no-op at this point, right?
::: browser/themes/shared/urlbar-searchbar.inc.css:402
(Diff revision 10)
> +}
> +
> +.searchbar-search-button:hover:not([addengines=true]) > .searchbar-search-icon-overlay {
> + -moz-context-properties: fill;
> + visibility: visible;
> + list-style-image: url(chrome://global/skin/icons/arrow-dropdown-12.svg);
nit: move -moz-context-properties right after list-style-image
::: browser/themes/shared/urlbar-searchbar.inc.css:415
(Diff revision 10)
> + fill-opacity: .6;
> +}
> +
> +.searchbar-search-button:hover:active > .searchbar-search-icon {
> + fill-opacity: .8;
> +}
nit: move these two rules up right after the .searchbar-search-icon rule
Attachment #8912747 -
Flags: review?(dao+bmo)
Comment hidden (mozreview-request) |
Comment 44•7 years ago
|
||
mozreview-review |
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
https://reviewboard.mozilla.org/r/184058/#review191884
Attachment #8912747 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Comment 45•7 years ago
|
||
Thanks for that, sorry for it being so many back and forwards, will pay more attention especially to the nits in future, pushed it to try to make sure and all green https://treeherder.mozilla.org/#/jobs?repo=try&revision=98ed93c0d3656311374e63d8931589ddf7c5029f
Comment 46•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
hg error in cmd: hg rebase -s ce3d50c6c1b4 -d abcd8dfc3a86: rebasing 424393:ce3d50c6c1b4 "Bug 1403359 - Switch search bar to photon icon. r=dao" (tip)
merging browser/themes/linux/searchbar.css
merging browser/themes/osx/searchbar.css
merging browser/themes/shared/jar.inc.mn
merging browser/themes/windows/searchbar.css
warning: conflicts while merging browser/themes/shared/jar.inc.mn! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment 48•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
hg error in cmd: hg rebase -s 826047463d91 -d 0d8b611c0dd2: rebasing 424498:826047463d91 "Bug 1403359 - Switch search bar to photon icon. r=dao" (tip)
merging browser/themes/linux/searchbar.css
merging browser/themes/osx/searchbar.css
merging browser/themes/shared/jar.inc.mn
merging browser/themes/windows/searchbar.css
warning: conflicts while merging browser/themes/shared/jar.inc.mn! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment 49•7 years ago
|
||
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b75e9c952bd
Switch search bar to photon icon. r=dao
Comment 50•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 51•7 years ago
|
||
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
Approval Request Comment
[Feature/Bug causing the regression]: photon polish
[User impact if declined]: inconsistent search glass icons in primary UI
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: not much
[Why is the change risky/not risky?]: this is a fairly isolated fix and we're more consistent now overall. if this needs a followup, that would likely be minor compared to the inconsistencies fixed here.
[String changes made/needed]: /
Attachment #8912747 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
Attachment #8914305 -
Attachment is obsolete: true
status-firefox57:
--- → affected
Comment on attachment 8912747 [details]
Bug 1403359 - Switch search bar to photon icon.
Photon polish, Beta57+
Attachment #8912747 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 53•7 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•