Closed Bug 1360491 Opened 7 years ago Closed 7 years ago

Add the search icon in the search input field

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: evanxd, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Bug 1360491 - Add the search icon in the search input field
59 bytes, text/x-review-board-request
dao
: review+
Details 
Add the search icon in the search input field.

The design spec: https://mozilla.invisionapp.com/share/ZDAGPK3AF#/screens/218928209
Hi Tina,

Could you provide the icon? Thanks.
Flags: needinfo?(thsieh)
It's in the spec :) Just click on the "Assets" and you'll see them.
https://mozilla.invisionapp.com/share/ZDAGPK3AF#/screens/219340982
Flags: needinfo?(thsieh)
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: qe-verify+
Hey Helen,

Are we going to use the same search icon for category and search input field? I notice there is only one icon living in assets folder but it seems like the one in category search is different with the one in input field. If they are different, we can reuse the existing search icon in Firefox search bar, what do you think?
Flags: needinfo?(hhuang)
QA Contact: hani.yacoub
The search icon for category is different with the one on the search field. Agree with reuse the existing search icon in Firefox search bar. Thanks!
Flags: needinfo?(hhuang)
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review138900

::: browser/components/preferences/in-content/preferences.xul:175
(Diff revision 2)
> -        <textbox type="search" id="searchInput" placeholder="&searchInput.label;" hidden="true"/>
> +        <textbox type="search" id="searchInput" placeholder="&searchInput.label;" hidden="true">
> +          <image
> +            class="search-button"
> +            anonid="search-button"/>
> +        </textbox>

I don't think you need to do this.

The XUL textbox binding should already include a search icon.

The XUL binding already includes a search icon by default: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/textbox.xml#323

I think it's just a matter of making it display on all platforms at the right place in:

toolkit/themes/*/global/textbox.css
Priority: -- → P1
ntim,

Thanks for your suggestion. IIRC, there is a pre-defined search icon for textbox[type="search"], and existing search icon appears at the right side of input field if running on Windows and Linux (the icon doesn't appear on macOS). I'm afraid there could be so many textbox with [type="search"] will be affected if we modify the current textbox.xml.

Mike, what do you think if we can just keep a customized search textbox for preferences pane? Or do you think is it safe to modify all textbox[type="search"] elements?
Flags: needinfo?(mconley)
I'm guessing the photon redesign also aims for consistency (across UI elements and platforms). So modifying the textbox binding should be fine.
(In reply to Ricky Chien [:rickychien] from comment #8)
> ntim,
> Mike, what do you think if we can just keep a customized search textbox for
> preferences pane? Or do you think is it safe to modify all
> textbox[type="search"] elements?

I suggest trying to use the current binding without modifying the mark-up like this, unless there's no other option.

I guess my questions are:

Where are the other places that the textbox[type="search"] variation is used? Is UX okay with us making them all uniform? That'll inform the way forward here.

Note that even if UX wants to special case this search input, we can probably continue to use the image node in the current binding, and use CSS to make it visible and put it in the right place.
Flags: needinfo?(mconley)
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review139328

I think we should double-check with UX if we want to actually make all textbox[search="true"] nodes similar, and if so, just change the common binding / styling.
Attachment #8864041 - Flags: review?(mconley) → review-
Target Milestone: --- → Firefox 55
Helen,

Even though we can reuse existing search icon in Firefox, I think we still need the visual spec for more details especially for icon color and margin / padding.
Flags: needinfo?(hhuang)
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review143186

I'm also going to needinfo? dao on this to see if he has any concerns, since this is going to affect theme for search all over the place.

::: toolkit/content/textbox.css:23
(Diff revision 4)
> +.textbox-search-icon {
> +  list-style-image: url(chrome://global/skin/icons/search-textbox.svg);
> +  margin-right: 5px;
> +  filter: opacity(60%);
> +}

Is it not necessary to flip this icon in RTL?

::: toolkit/content/textbox.css:25
(Diff revision 4)
>    -moz-box-flex: 1;
>  }
>  
> +.textbox-search-icon {
> +  list-style-image: url(chrome://global/skin/icons/search-textbox.svg);
> +  margin-right: 5px;

Should this be margin-inline-start? Have you tested this in the RTL locale configuration?

::: toolkit/content/widgets/textbox.xml:324
(Diff revision 4)
>        <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context,spellcheck" align="center">
> +        <xul:image class="textbox-search-icon" anonid="searchbutton-icon"
> +                    xbl:inherits="src=image,label=searchbuttonlabel,searchbutton,disabled"/>
>          <html:input class="textbox-input" anonid="input" mozactionhint="search"
>                      xbl:inherits="value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey,mozactionhint,spellcheck"/>
>          <xul:deck class="textbox-search-icons" anonid="search-icons">

Doesn't make much sense to keep this deck around if we're only putting a single icon in it. We should probably pull the deck.

::: toolkit/content/widgets/textbox.xml:325
(Diff revision 4)
>          <html:input class="textbox-input" anonid="input" mozactionhint="search"
>                      xbl:inherits="value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey,mozactionhint,spellcheck"/>
>          <xul:deck class="textbox-search-icons" anonid="search-icons">
> -          <xul:image class="textbox-search-icon" anonid="searchbutton-icon"
> -                     xbl:inherits="src=image,label=searchbuttonlabel,searchbutton,disabled"/>
>            <xul:image class="textbox-search-clear"

So the clear button and the search icon will not occupy the same space, ever, in any of the instances of search-toolbox. That means not only in about:preferences, but sidebars, the Library search input, etc. I just want to be absolutely clear this is what UX wants. Is that so?

::: toolkit/themes/windows/global/textbox.css
(Diff revision 4)
>    padding: 0px !important;
>  }
>  
>  /* ::::: search textbox ::::: */
>  
> -.textbox-search-icon {

What about http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/toolkit/themes/osx/global/textbox.css#57-61 (for things like the Library search input)? Is it necessary to keep the `-moz-appearance: searchfield;` rule? Or can we cut it?
Attachment #8864041 - Flags: review?(mconley) → review-
Hey dao, just putting this on your radar - it _looks_ as if this is going to be change to all input[type="search"]. Any concerns beyond what I've already brought up?
Flags: needinfo?(dao+bmo)
I'm not sure what the context is here. What motivates this change? Is this even a valid bug? It seems to have been filed based on Mac not using a search icon in search fields, but that was a deliberate choice as far as I can tell.
Flags: needinfo?(dao+bmo)
Component: Preferences → Themes
Product: Firefox → Toolkit
Target Milestone: Firefox 55 → ---
Version: unspecified → Trunk
@tina:

I would like to make sure our spec is finalized and it's going to impact on all input[type="search"]. Note that we have different look and feel in current style of input[type="search"] depends on platforms, such as there is no show search icon on Mac but the icon will be rendered when running Linux or Mac.

We just want to confirm that are we going to draw the same style for all platforms? Do you think it's reasonable to add the search icon for all input[type="search"] elements or merely apply for about:preferences page?

Thanks
Flags: needinfo?(thsieh)
Hi Ricky,
Yes, please add the magnifying glass icon to all search fields :) And please also notice that the direction of the icon is different on windows and mac.

Also, please see the the search field visual spec from Helen: https://docs.google.com/presentation/d/1EA-qGEjhkebW9IbUZ_fv_XYflu5dlutc60Nw57nT5sQ/edit#slide=id.g1823faf4d4_4_10
Flags: needinfo?(thsieh)
Flags: needinfo?(hhuang)
We'll need some rationale here for dropping the native style rather than just a decision.

I did some digging. Bug 450800 implemented the native search field appearance on Mac. I believe at that time, this would render a search glass at the start of the field. Has macOS stopped doing this?
Flags: needinfo?(mstange)
On Mac the search glass icon is drawn as part of the searchfield -moz-appearance. I think the problem in the preferences is that the native styling gets disabled, so you lose the search glass together with it.

For example, the search fields in the bookmarks sidebar or the bookmarks library window still have search glass icons.
Flags: needinfo?(mstange)
Thanks Markus.
Component: Themes → Preferences
Product: Toolkit → Firefox
The current native macOS widget comes with a search glass too. You can find it on Finder. XCode also show that.

Removing 

http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/toolkit/themes/shared/in-content/common.inc.css#442

will make that show up within preferences. It would apply to in-content preferences and sub-dialogs (as illustrated in comment 19), and about:addons.

I tend to think changing the XUL impl is out of scope of this bug. We can consider this kind of refractor later (pre- or post-Photon).
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review144420

Please drop all toolkit widget changes from this patch.

I suspect the easiest and arguably preferable path will be to make toolkit/themes/shared/in-content/common.inc.css stop messing with <textbox type="search"/>.
Attachment #8864041 - Flags: review-
(In reply to Dão Gottwald [::dao] from comment #26)
> I suspect the easiest and arguably preferable path will be to make
> toolkit/themes/shared/in-content/common.inc.css stop messing with <textbox
> type="search"/>.

You mean make it use -moz-appearance again? I'm not sure the native styling will look good with the large font size that's used in the preferences, and the search field border will look different from all other control borders in the preferences.
IIRC, there are two approaches (1) changing XUL textbox (platform native styling and it will affect the look and feel of -moz-appearance) (2) changing XBL toolkit widget textbox.xml itself (which disables the native styling with -moz-appearance: none). 

The current approach of preferences page is using (2), so I believe Tim's suggestion on comment 23 is reasonable, and it matches our UX requirement as well (see also comment 19) since there is only slightly styling differences (direction of icon) depends on platforms.
(In reply to Ricky Chien [:rickychien] from comment #29)
> IIRC, there are two approaches (1) changing XUL textbox (platform native
> styling and it will affect the look and feel of -moz-appearance) (2)
> changing XBL toolkit widget textbox.xml itself (which disables the native
> styling with -moz-appearance: none).

This makes no sense to me. The fix for this should be entirely in toolkit/themes/shared/in-content/common.inc.css.
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review144578

::: toolkit/content/widgets/textbox.xml
(Diff revision 7)
>      <content>
>        <children/>
>        <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context,spellcheck" align="center">
> -        <html:input class="textbox-input" anonid="input" mozactionhint="search"
> -                    xbl:inherits="value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey,mozactionhint,spellcheck"/>
> -        <xul:deck class="textbox-search-icons" anonid="search-icons">

Please stop messing with toolkit/content/widgets/textbox.xml.

::: toolkit/themes/windows/global/textbox.css
(Diff revision 7)
>  .textbox-search-icon {
> -  list-style-image: url(chrome://global/skin/icons/Search-glass.png);
> -  -moz-image-region: rect(0, 16px, 16px, 0);
> -}
> -
> -.textbox-search-icon:-moz-locale-dir(rtl) {

Please stop messing with toolkit/themes/*/global/textbox.css.
Attachment #8864041 - Flags: review-
(In reply to Dão Gottwald [::dao] from comment #30)
> (In reply to Ricky Chien [:rickychien] from comment #29)
> > IIRC, there are two approaches (1) changing XUL textbox (platform native
> > styling and it will affect the look and feel of -moz-appearance) (2)
> > changing XBL toolkit widget textbox.xml itself (which disables the native
> > styling with -moz-appearance: none).
> 
> This makes no sense to me. The fix for this should be entirely in
> toolkit/themes/shared/in-content/common.inc.css.

... or toolkit/themes/osx/global/in-content/common.css of course.
Dao, if I understand correctly, this bug is not only about osx missing the search icon in the in-content prefs.

But it's also about moving the search icon to the beginning of the input as mentioned in the spec, and I think it wouldn't make sense to do this change only for the in-content prefs. Comment 19 also mentions those changes need to apply to all inputs.

So the changes in textbox.xml and textbox.css seem right, but there might need to be an extra rule in osx/textbox.css to hide the icon on OSX.
(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #33)
> But it's also about moving the search icon to the beginning of the input as
> mentioned in the spec

*on Windows and Linux. On these platforms, the icon is at the end atm.
(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #34)
> (In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from
> comment #33)
> > But it's also about moving the search icon to the beginning of the input as
> > mentioned in the spec
> 
> *on Windows and Linux. On these platforms, the icon is at the end atm.

We should keep it like that for the time being. Changing this is way out of scope here.
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review143186

> So the clear button and the search icon will not occupy the same space, ever, in any of the instances of search-toolbox. That means not only in about:preferences, but sidebars, the Library search input, etc. I just want to be absolutely clear this is what UX wants. Is that so?

Could you give me more details about your examples such as sidebars and Library search input, and perhaps we can ni? Tina for UX confirmation.
Flags: needinfo?(mconley)
(In reply to Dão Gottwald [::dao] from comment #31)
> ::: toolkit/content/widgets/textbox.xml
> (Diff revision 7)
> >      <content>
> >        <children/>
> >        <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context,spellcheck" align="center">
> > -        <html:input class="textbox-input" anonid="input" mozactionhint="search"
> > -                    xbl:inherits="value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey,mozactionhint,spellcheck"/>
> > -        <xul:deck class="textbox-search-icons" anonid="search-icons">
> 
> Please stop messing with toolkit/content/widgets/textbox.xml.

Dao, is there any way to avoid touching widgets/textbox.xml but it's able to change the look and feel of search field for preferences?

(The structure of textbox element will be changed and magnifying icon will always appear at the left).
Flags: needinfo?(mconley) → needinfo?(dao+bmo)
Target Milestone: --- → Firefox 55
(In reply to Ricky Chien [:rickychien] from comment #41)
> Dao, is there any way to avoid touching widgets/textbox.xml but it's able to
> change the look and feel of search field for preferences?

Yes: Just use background-image to add the icon on Mac. The scope creep here makes this bug much more complicated than it needs to be.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #43)
> (In reply to Ricky Chien [:rickychien] from comment #41)
> > Dao, is there any way to avoid touching widgets/textbox.xml but it's able to
> > change the look and feel of search field for preferences?
> 
> Yes: Just use background-image to add the icon on Mac. The scope creep here
> makes this bug much more complicated than it needs to be.

You would also need to move the icon to the left on Windows and Linux.
(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #44)
> (In reply to Dão Gottwald [::dao] from comment #43)
> > (In reply to Ricky Chien [:rickychien] from comment #41)
> > > Dao, is there any way to avoid touching widgets/textbox.xml but it's able to
> > > change the look and feel of search field for preferences?
> > 
> > Yes: Just use background-image to add the icon on Mac. The scope creep here
> > makes this bug much more complicated than it needs to be.
> 
> You would also need to move the icon to the left on Windows and Linux.

As I said before, we should not do that in this bug as it has implications for in-product and add-on consumers of the search textbox widget. I mean, it's doable, but I'd argue it shouldn't be part of this focused effort. There's no strong need to open this can of worms in the limited time that we're focused on photon.
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review145926

I do not think we should fork the binding for this.
Attachment #8864041 - Flags: review?(dao+bmo)
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review145946

::: toolkit/themes/linux/global/in-content/common.css:119
(Diff revision 12)
>  
>  xul|treechildren::-moz-tree-row(selected) {
>    background-color: var(--in-content-item-selected);
>  }
>  
> +xul|textbox .textbox-search-main-icon {

textbox-search-main-icon doesn't exist

::: toolkit/themes/shared/in-content/common.inc.css:467
(Diff revision 12)
>    min-height: unset;
>    padding-right: unset;
>    padding-left: unset;
>  }
>  
> +xul|textbox .textbox-input-box {

Please use the child selector

::: toolkit/themes/shared/in-content/common.inc.css:468
(Diff revision 12)
>    padding-right: unset;
>    padding-left: unset;
>  }
>  
> +xul|textbox .textbox-input-box {
> +  background: url(chrome://global/skin/icons/search-textbox.svg) no-repeat center left;

Looks like this adds the icon to the homepage field too, which I suspect we don't want.

::: toolkit/themes/shared/in-content/common.inc.css:473
(Diff revision 12)
> +  background: url(chrome://global/skin/icons/search-textbox.svg) no-repeat center left;
> +  padding-inline-start: 16px;
> +}
> +
> +xul|textbox .textbox-input-box:-moz-locale-dir(rtl) {
> +  transform: scaleX(-1);

This doesn't seem to make sense. I think you need to adjust background-position to move the icon to the right spot for RTL.

::: toolkit/themes/shared/in-content/common.inc.css:476
(Diff revision 12)
> +
> +xul|textbox .textbox-input-box:-moz-locale-dir(rtl) {
> +  transform: scaleX(-1);
> +}
> +
> +xul|textbox .textbox-search-icon {

Please use the child selector

::: toolkit/themes/windows/global/in-content/common.css:80
(Diff revision 12)
>    xul|listbox xul|listitem[selected="true"] {
>       border: 2px dotted Highlight;
>    }
>  }
> +
> +xul|textbox .textbox-search-main-icon {

textbox-search-main-icon doesn't exist
Attachment #8864041 - Flags: review?(dao+bmo) → review-
I think you meant to review Revision 13 not Revision 12.
Flags: needinfo?(dao+bmo)
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review146190

::: toolkit/themes/shared/in-content/common.inc.css:473
(Diff revision 13)
> +  background: url(chrome://global/skin/icons/search-textbox.svg) no-repeat center left;
> +  padding-inline-start: 16px;
> +}
> +
> +xul|textbox .textbox-input-box:-moz-locale-dir(rtl) {
> +  transform: scaleX(-1);

Shouldn't this be background-position-x: right; ?
(In reply to Tim Nguyen :ntim from comment #50)
> I think you meant to review Revision 13 not Revision 12.

No, I reviewed the revision that was the latest when I opened it. Most review comments I made still seem to apply.
Flags: needinfo?(dao+bmo)
Dao, I know this question could be out of topic.

Are you using slack or irc? (Cannot find your id on both) I have few questions would like to ask you online and hopefully we can address this issue more efficiently :)

Thanks
Flags: needinfo?(dao+bmo)
(In reply to Ricky Chien [:rickychien] from comment #53)
> Dao, I know this question could be out of topic.
> 
> Are you using slack or irc? (Cannot find your id on both) I have few
> questions would like to ask you online and hopefully we can address this
> issue more efficiently :)
> 
> Thanks

I'm on irc as dao but today is a bank holiday.
Flags: needinfo?(dao+bmo)
Comment on attachment 8864041 [details]
Bug 1360491 - Add the search icon in the search input field

https://reviewboard.mozilla.org/r/135766/#review146400
Attachment #8864041 - Flags: review?(dao+bmo) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96ef311ad9c0
Add the search icon in the search input field r=dao
https://hg.mozilla.org/mozilla-central/rev/96ef311ad9c0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
The search icon in the search input field for Windows and Linux is not matching the spec, it looks the same as on mac.
is this the intended behavior?
Flags: needinfo?(evan)
Hi Hani,

I think you want to needinfo from Ricky. :)
Flags: needinfo?(evan) → needinfo?(rchien)
Hani, yep you're right. Please file a bug we will deal with this soon..
Flags: needinfo?(rchien) → needinfo?(hani.yacoub)
I logged this bug 1369640.
Thanks.
Flags: needinfo?(hani.yacoub)
Build ID: 20170601030206
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Depends on: 1378330
On Firefox 62 or above, any <input type="search"> has still no difference between text type.
Can some one reopen this?
Flags: needinfo?(jaws)
(In reply to lolipopplus from comment #64)
> On Firefox 62 or above, any <input type="search"> has still no difference
> between text type.
> Can some one reopen this?

This bug is about the searchbox in about:preferences, <input type=search> is covered by bug 558594.
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: