Update search bar icon to photon icon

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: daleharvey, Assigned: daleharvey)

Tracking

(Blocks 1 bug)

unspecified
Firefox 58
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Updated

2 years ago
Assignee: nobody → dharvey
Status: NEW → ASSIGNED
Flags: qe-verify-
Assignee

Comment 1

2 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)
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

2 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?
There's arrow-dropdown-12.svg but that's probably still too big for this.
Comment hidden (mozreview-request)
Assignee

Comment 6

2 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 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

2 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

2 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

2 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 12

2 years ago
duplicate
The inconsistency isn't totally fixed. The icon for "Find in Preferences" in about:preferences is different from the three other search icons I can see.

(This was originally reported in bug 1396893 which was closed as a duplicate of this bug.)

Comment 13

2 years ago
duplicate
(Sorry, the previous comment was meant for bug 1399642. I'll place it there. (Too many windows open.)
No longer blocks: 1396893
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 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

2 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

2 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

2 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

2 years ago
Posted image Bad rendering with opacity set (obsolete) —

Comment 24

2 years ago
Have you tried putting a lower z-index for the search icon?
Assignee

Comment 25

2 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

2 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

2 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

2 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

2 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

2 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?
(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

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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)
https://hg.mozilla.org/mozilla-central/rev/4b75e9c952bd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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?
Attachment #8914305 - Attachment is obsolete: true
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+
Depends on: 1407007
You need to log in before you can comment on or make changes to this bug.