Closed Bug 1403359 Opened 7 years ago Closed 7 years ago

Update search bar icon to photon icon

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: daleharvey, Assigned: daleharvey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(3 files, 1 obsolete file)

Bug 1403359 - Switch search bar to photon icon.
59 bytes, text/x-review-board-request
dao
: review+
ritu
: approval-mozilla-beta+
Details
Screen Shot 2017-09-27 at 16.29.01.png
109.73 KB, image/png
Details
search-indicator-badge-add.svg
538 bytes, image/svg+xml
Details
Assignee: nobody → dharvey
Status: NEW → ASSIGNED
Flags: qe-verify-
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.
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.
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)
> 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
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 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
Flags: needinfo?(shorlander)

    
    

    
      
  

    
    

    
      
  
No longer blocks: 1396893

    
    

    
      
  

    
    

    
      
  

    
    

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

    
    

    
      
  

    
    

    
  
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

    
    

    
  

      
      
      
      

        Attached image
          
        Bad rendering with opacity set (obsolete)
        — 
      

    


  

    
    

    
  
Have you tried putting a lower z-index for the search icon?

    
    

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

    
    

    
  
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)

    
    

    
      
  

    
    

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

    
    

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

    
    

    
      
  

    
    

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

    
    

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

    
    

    
  
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

    
    

    
  
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)

    
    

    
      
  

    
    

    
  
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
Closed: 7 years ago

          status-firefox58:
          --- → fixed
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: