1110767 - Hovering over a one-click button should immediately show the name of the search engine

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

One issue with the current one-click search design is that it makes it hard to distinguish search engines that have the same icon (e.g. Wikipedia in two languages). There is a tooltip on the buttons, but it takes a while for it to show up.

To remedy this issue in the short term, we should make the header of the button section (the one that says »Search for xyz on:«) change when hovering over a one-click button.
It then should say e.g.: »Search for xyz on Wikipedia (de)«

This should happen immediately when rolling over the button or selecting it using the keyboard.

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Oops, I just realized that I messed up that string. It should say:
»Search for %searchterm with %enginename«

Comment 2

[Florian Quèze [:florian]]

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #0)

> To remedy this issue in the short term

With a string, "short term" here can't be earlier than 37.

> It then should say e.g.: »Search for xyz on Wikipedia (de)«

For a relatively small panel and when xyz is relatively long, we already have to crop the xyz with an ellipsis. Search engine names can also be quite long. Should which of the searchterm or enginename should we crop the most aggressively?

Comment 4

[Florian Quèze [:florian]]

An alternative proposal was to show the name of the currently highlighted (either through mouse or keyboard) one-off engine at the top of the panel, at the place where we display the name of the default engine.

Comment 5

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> An alternative proposal was to show the name of the currently highlighted
> (either through mouse or keyboard) one-off engine at the top of the panel,
> at the place where we display the name of the default engine.

My concern here is that this would put a lot of space between the mouse and the label. Plus it would also change the way the panel is read (do the suggestions now come from Wikipedia?).

Aislinn is currently working on the same problem in bug 1103315 (no dupe, but it really is the UX work for this bug).

Comment 6

[Ash Grigas]

Hi Florian, If it helps here is my work in progress on bug 1103315: http://invis.io/ZC263ZSS5

You can arrow key or hover over the icons to see the provider reflect in the text unit above.

Comment 7

[Florian Quèze [:florian]]

(In reply to agrigas from comment #6)

Thanks! What happens when the text typed by the user is significantly longer?

Comment 8

[Ash Grigas]

I would suggest we truncate the word since its clearly visible above in the search box, ie. 'Search for amaghdif... on Wikipedia(en)'

a second approach might be on hover/select with arrow - swapping out the 'Search for amaghdif' string with the provider readout only 'on Wikipedia (en) since until hover/select we read out 'Search for'

Comment 9

[Chris]

Attachment: WIP.patch

Taking this, I've made the changes necessary to show the name of the search service being hovered over in the one click list.  I haven't implemented truncating for the search text yet.

Right now if none of the search services are being hovered over in the one click list the header text will show "Search for xyz on".  Is this okay or is there something else that we can show instead.

Comment 10

[Chris]

Florian feel free to reassign this if you'd rather take it.

Comment 11

[Chris]

Attachment: WIP.patch

Changed crop settings to favor truncating the search text.

Comment 12

[Florian Quèze [:florian]]

(In reply to Chris from comment #10)
> Florian feel free to reassign this if you'd rather take it.

I'm happy to review/mentor here :-).

Note that the UX has been designed in bug 1103315, especially see bug 1103315 comment 25:

(agrigas from bug 1103315 comment #25)
> Updated design based on text size issue. Only show Search provider name on
> hover/select with arrow key of provider icon.
> 
> Default (not selected) show current UI: 'Search xxx... on:'
> Hover/arrow select show new UI: 'Search Wikipedia(en)'

Comment 13

[Chris]

okay, it shouldn't be too difficult to make the changes needed to get the UI to match the specs as per bug 1103315 comment 25.  I'll let you know if I have any problems.

Thanks Florian

Comment 14

[Chris]

Attachment: bug-1110767.patch

Changed patch to match the new UI specs.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8568248 [details] [diff] [review]
bug-1110767.patch

Review of attachment 8568248 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/urlbarBindings.xml
@@ +999,3 @@
>          <xul:treechildren class="autocomplete-treebody"/>
>        </xul:tree>
>        <xul:deck anonid="search-panel-one-offs-header"

Adding another child to this deck will be easier than having to tweak "hidden" attributes with JS code.

@@ +1079,5 @@
>  
>          // Show the current default engine in the top header of the panel.
>          this.updateHeader();
>  
> +        // Update the 'Search for <keywords> on" header.

I don't think changing this string was really intentional in the UX bug/comment. I would prefer that we keep this bug focused on fixing "Hovering over a one-click button should immediately show the name of the search engine".

@@ +1240,5 @@
>          // Required to receive click events from the buttons on Linux.
>          event.preventDefault();
>        ]]></handler>
>  
>        <handler event="mouseover"><![CDATA[

The mouse events aren't the easiest place to integrate your changes, as that would force you to handle separately a search icon selected from the keyboard or hovered. I think you really want your changes to be in the selectedButton setter (http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml?rev=50bf2ec4b075#1013).

Note that bug 1107695 modified this code very recently, so you may want to ensure you have an updated tree before you start editing that setter.

Comment 16

[Chris]

> Adding another child to this deck will be easier than having to tweak "hidden" attributes with JS
> code.

Could you elaborate on this?  What child would I add, how would it help me to avoid accessing
hidden attributes with JS code?

> The mouse events aren't the easiest place to integrate your changes, as that would force you to
> handle separately a search icon selected from the keyboard or hovered. I think you really want your
> changes to be in the selectedButton setter (http://mxr.mozilla.org/mozilla-central/source/browser
> /components/search/content/search.xml?rev=50bf2ec4b075#1013).

This sounds like a much better option than what I did.

Comment 17

[Florian Quèze [:florian]]

(In reply to Chris from comment #16)
> > Adding another child to this deck will be easier than having to tweak "hidden" attributes with JS
> > code.
> 
> Could you elaborate on this?  What child would I add, how would it help me
> to avoid accessing
> hidden attributes with JS code?

A xul deck (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/deck) is more or less an automated way to hide/show elements.

Comment 18

[Chris]

Comment on attachment 8568248 [details] [diff] [review]
bug-1110767.patch

This patch is bitrotted.  Will upload another one as soon as I get the changes finished.

Comment 19

[Chris]

Attachment: WIP.patch

Quick update on this

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8574844 [details] [diff] [review]
WIP.patch

Review of attachment 8574844 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/urlbarBindings.xml
@@ +1154,5 @@
>              groupText = headerSearchText.previousSibling.value +
>                          '"' + headerSearchText.value + '"' +
>                          headerSearchText.nextSibling.value;
> +            if (this.selectedButton &&
> +                this.selectedButton.label != "Change Search Settings") {

You probably want to check this.selectedButton.getAttribute("anonid") != "search-settings"

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +425,5 @@
>  <!-- LOCALIZATION NOTE (searchWithHeader.label):
>       The wording of this string should be as close as possible to
>       searchFor.label and searchWith.label. This string will be used instead of
>       them when the user has not typed any keyword. -->
> +<!ENTITY searchOnHeader.label       "Search on:">

I don't think there has been a UX decision to change "with" to "on". It appeared in mockups, but it could have been just an accident.

Comment 21

[Ash Grigas]

yes we can keep 'with'

Comment 22

[Chris]

> I don't think there has been a UX decision to change "with" to "on". It appeared in mockups, but it could > have been just an accident.

Yes, I had changed the search header text to 'on' according to the mockup, but that's not a big deal to change back.

> You probably want to check this.selectedButton.getAttribute("anonid") != "search-settings"

Thanks for the suggestion, I went ahead and made this change.

Comment 23

[Chris]

Attachment: WIP.patch

I fixed an issue where the header index was not changing properly when the Change Search Settings button was selected (IE: should be "Search abc with:", was "Search ")

I still need to check and make sure that I have the synthesizeNativeMouseMove method correct for all OSes.  I also may end up needing to disable these test cases on e10s due to an issue where altering the searchbar textbox under e10s seems to cause problems with later tests that attempt to populate search history (Such as keyboard navigation).

Comment 24

[Chris]

Attachment: bug-1110767.patch

Try server run at:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a9202a2438f

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 8578148 [details] [diff] [review]
bug-1110767.patch

Review of attachment 8578148 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the test! :)

::: browser/base/content/urlbarBindings.xml
@@ +1042,5 @@
>                  class="search-panel-header search-panel-current-input">
>          <xul:label anonid="searchbar-oneoffheader-search" value="&searchWithHeader.label;"/>
>          <xul:hbox anonid="search-panel-searchforwith"
>                    class="search-panel-current-input">
> +          <xul:label anonid="searchbar-oneoffheader-before" value="&search.label;"/>

Similar to not changing "with" to "on", I don't think there has been a decision to remove the word "for", so let's not change this string please.

@@ +1048,5 @@
>            <xul:label anonid="searchbar-oneoffheader-after" flex="10000" value="&searchWith.label;"/>
>          </xul:hbox>
> +        <xul:hbox anonid="search-panel-searchonengine"
> +                  class="search-panel-current-input">
> +          <xul:label anonid="searchbar-oneoffheader-beforeengine" flex="1" value="&search.label;"/>

And I guess my comment above means that you do need a new string for this case. Please add a localization note above it in the .dtd file, to specify how it's going to be used (especially mention that the engine name will be displayed after it).

@@ +1162,5 @@
>                                                        "searchbar-oneoffheader-search");
>              groupText = noSearchHeader.value;
>              headerPanel.selectedIndex = 0;
>            }
> +          // Override header index if a oneOff button has been selected.

Is there any way the search panel can open with a one-off button already selected?

::: browser/components/search/content/search.xml
@@ +1037,5 @@
>            if (val && !val.classList.contains("dummy")) {
>              val.setAttribute("selected", "true");
>              this._selectedButton = val;
> +            // Override header index and show search engine.
> +            if (val.getAttribute("anonid") != "search-settings") {

I don't think this test is enough, it could still be an open search item.

Instead, you could check val.classList.contains("searchbar-engine-one-off-item")

@@ +1039,5 @@
>              this._selectedButton = val;
> +            // Override header index and show search engine.
> +            if (val.getAttribute("anonid") != "search-settings") {
> +              header.setAttribute("selectedIndex", 2);
> +              engineText.setAttribute("value", val.getAttribute("tooltiptext"));

I'm not sure getting the tooltip text is future proof. How would you feel about val.engine.name (that's the value currently set to the tooltiptext attribute)?

::: browser/components/search/test/browser_oneOffHeader.js
@@ +17,5 @@
> +  document.getAnonymousElementByAttribute(searchPopup, "anonid",
> +                                          "search-panel-one-offs-header");
> +function getHeaderText() {
> +  let label = header.selectedPanel;
> +  while(label.hasChildNodes()) {

nit: space between 'while' and '('.

@@ +25,5 @@
> +  while(label) {
> +    headerStrings.push(label.value);
> +    label = label.nextSibling;
> +  }
> +  return headerStrings.join("");

Would |header.textContent;| return a different value?

Comment 26

[Chris]

Thanks for the feedback!

> Similar to not changing "with" to "on", I don't think there has been a decision to remove the word
> "for", so let's not change this string please.

Sounds reasonable, I made a new string with a localization tip attached specifying where it is used, and I went ahead and changed the other string back to searchFor.label.

>                                                       "searchbar-oneoffheader-search");
>             groupText = noSearchHeader.value;
>             headerPanel.selectedIndex = 0;
>           }
> +          // Override header index if a oneOff button has been selected.
> Is there any way the search panel can open with a one-off button already selected?

This is a part of the inputHandler function which fires when text is entered into the searchbar.  This logic is needed to cover the case where the user has selected a one off button and then enters in more text.  My intention is that in this case the header should continue to show "Search <Engine name>" instead of switching back to "Search for <Search terms> with:"

I went ahead and changed the comment to try and make that more clear.

> I don't think this test is enough, it could still be an open search item.
> 
> Instead, you could check val.classList.contains("searchbar-engine-one-off-item")

I went ahead and made this change to search.xml and URLBindings.xml

> I'm not sure getting the tooltip text is future proof. How would you feel about val.engine.name (that's > the value currently set to the tooltiptext attribute)?

Yes, thank you.  I thought there had to be a better value to use for this.

> nit: space between 'while' and '('.

 >.<

> Would |header.textContent;| return a different value?

When I tried using header.textContent I got an empty string.  I'll have to fool around with it some more and see if I can get better results with it.

Comment 27

[Chris]

Attachment: bug-1110767.patch

I updated the logic in the input function for the textbox in URLBindings.xml and in the setter in search.xml for clarity and to reduce extra work.  I haven't made any changes to the test cases yet.

Comment 28

[Chris]

Attachment: bug-1110767.patch

It doesn't look like I can use textContent in that way since the only thing I was able to get from it was an empty string, I've posted a new try server run at:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f91e398bb75d

Comment 29

[Florian Quèze [:florian]]

Comment on attachment 8585211 [details] [diff] [review]
bug-1110767.patch

Review of attachment 8585211 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Chris, sorry for the delay in reviewing this, I was away for the last 10 days or so. The patch looks good, all the comments I left are details :-).

::: browser/base/content/urlbarBindings.xml
@@ +1040,5 @@
>            <xul:label anonid="searchbar-oneoffheader-after" flex="10000" value="&searchWith.label;"/>
>          </xul:hbox>
> +        <xul:hbox anonid="search-panel-searchonengine"
> +                  class="search-panel-current-input">
> +          <xul:label anonid="searchbar-oneoffheader-beforeengine" flex="1" value="&search.label;"/>

Is the flex="1" needed here?

::: browser/components/search/content/search.xml
@@ +1031,5 @@
> +            if (val.classList.contains("searchbar-engine-one-off-item")) {
> +              let headerEngineText =
> +                document.getAnonymousElementByAttribute(this.popup, "anonid",
> +                                                        "searchbar-oneoffheader-engine");
> +              header.setAttribute("selectedIndex", 2);

Would header.selectedIndex = 2; work here?

@@ +1037,5 @@
> +            }
> +            else {
> +              let textbox = document.getBindingParent(this).textbox;
> +              header.setAttribute("selectedIndex",
> +                                  textbox.value ? 1 : 0);

and here?

@@ -1028,4 @@
>              this.setAttribute("aria-activedescendant", val.id);
>              return;
>            }
> -

Please keep this empty line.

::: browser/components/search/test/browser_oneOffHeader.js
@@ +9,5 @@
> +const textbox = searchbar._textbox;
> +const searchPopup = document.getElementById("PopupSearchAutoComplete");
> +const searchIcon = document.getAnonymousElementByAttribute(searchbar, "anonid",
> +                                                           "searchbar-search-button");
> +const searchButton =

Shouldn't this be named 'searchSettings'?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +427,5 @@
> +<!-- LOCALIZATION NOTE (search.label):
> +     This string is used to build the header above the list of one-click search
> +     providers when a one off engine has been selected:
> +     "Search <selected engine name>" -->
> +<!ENTITY search.label                 "Search ">

I wonder if some locales would need to have a string after the engine name.

Comment 30

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] (PTO until April 7th) from comment #29)
> Comment on attachment 8585211 [details] [diff] [review]
> bug-1110767.patch
> 
> Review of attachment 8585211 [details] [diff] [review]:
> -----------------------------------------------------------------

> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +427,5 @@
> > +<!-- LOCALIZATION NOTE (search.label):
> > +     This string is used to build the header above the list of one-click search
> > +     providers when a one off engine has been selected:
> > +     "Search <selected engine name>" -->
> > +<!ENTITY search.label                 "Search ">
> 
> I wonder if some locales would need to have a string after the engine name.

flod, do you have an opinion on this?

Comment 31

[Francesco Lodolo [:flod]]

Yes, definitely need a "after" string, but it would be even better to have a string like "Search %S", with the searchengine replaced by a placeholder that localizers can move around.

The English is a bit confusing to me: "Search Google", so "Search" is a noun? Wouldn't "Google Search" sound more natural to a native speaker (and I'm not)?

Is there a screenshot or UX spec for this?

Comment 32

[Florian Quèze [:florian]]

(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #31)
> Yes, definitely need a "after" string, but it would be even better to have a
> string like "Search %S", with the searchengine replaced by a placeholder
> that localizers can move around.

This isn't easy, as we are showing the engine name with a different color (black; the rest of the line is gray).

> The English is a bit confusing to me: "Search Google", so "Search" is a
> noun? Wouldn't "Google Search" sound more natural to a native speaker (and
> I'm not)?
> 
> Is there a screenshot or UX spec for this?

UX happened in bug 1103315; unfortunately there's no mockup/screenshot of the final design (bug 1103315 comment 25).

Comment 33

[Francesco Lodolo [:flod]]

(In reply to Florian Quèze [:florian] [:flo] from comment #32)
> (In reply to Francesco Lodolo [:flod] (UTC+2) from comment #31)
> > Yes, definitely need a "after" string, but it would be even better to have a
> > string like "Search %S", with the searchengine replaced by a placeholder
> > that localizers can move around.
> 
> This isn't easy, as we are showing the engine name with a different color
> (black; the rest of the line is gray).

Then 2 keys, one "before" and one "after" empty for English, with clear instructions in the localization note.

Comment 34

[Chris]

I went ahead and created a searchAfter.label string in browser.dtd and changed the localization note attached to it and search.label to include it.  On a related note, are the test cases run in different localizations?  browser_oneOffHeader test cases currently aren't going to pass if the strings in the localization change.

Comment 35

[Florian Quèze [:florian]]

(In reply to Chris from comment #34)

> On a related note, are the test cases run in different localizations? 
> browser_oneOffHeader test cases currently aren't going to pass if the
> strings in the localization change.

I'm not aware of anybody running our automated tests on localized builds.

flod, the patch here includes tests that hardcode the en-US strings, based on the assumption that tests are always run on en-US builds, is this assumption correct?

Comment 36

[Francesco Lodolo [:flod]]

Let's start by saying that I don't know much about tests.

There are automated tests on localized builds (MozMill), but they're a completely different thing from the ones integrated in m-c. Looking around in that folder (eg. browser_contextmenu.js) I assume it's OK to check for English text.
http://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/browser_contextmenu.js#62

Comment 37

[Chris]

Attachment: bug-1110767.patch

As mentioned before I added in a new string for non-english localizations.  I also fixed a logic bug in the selectable buttons setter in search.xml which would have prevented the header string from switching it's value correctly and  added in a flex and crop value to the search engine text to handle search engines with long names.  This is in addition to making the suggested changes.

Comment 38

[Francesco Lodolo [:flod]]

Comment on attachment 8590394 [details] [diff] [review]
bug-1110767.patch

Review of attachment 8590394 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +426,5 @@
> +
> +<!-- LOCALIZATION NOTE (search.label, searchAfter.label):
> +     This string is used to build the header above the list of one-click search
> +     providers when a one off engine has been selected.  The searchAfter text is
> +     intentionally left empty for english localizations and is presented as a way

… is intentionally left empty for en-US and can be used by other localizations to display a string after the search engine name.

Comment 39

[Chris]

Attachment: bug-1110767.patch

Thanks for the feedback flod, I went ahead and changed the localization comment as suggested.

Comment 40

[Florian Quèze [:florian]]

Comment on attachment 8590404 [details] [diff] [review]
bug-1110767.patch

Review of attachment 8590404 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for the updated patch!

Can you please address the 2 following (completely trivial) nits, and push to try again? I think the patch has changed enough since comment 28 that sheriffs will want to see a newer green try before pushing this with the checkin-needed keyword.

::: browser/components/search/content/search.xml
@@ +1022,5 @@
>              this._selectedButton.removeAttribute("selected");
>  
> +          let textbox = document.getBindingParent(this).textbox;
> +          let header =
> +              document.getAnonymousElementByAttribute(this.popup, "anonid",

nit: indent.

::: browser/components/search/test/browser_oneOffHeader.js
@@ +72,5 @@
> +  yield promiseNewEngine("testEngine.xml");
> +});
> +
> +add_task(function* test_notext() {
> +

nit: this empty line doesn't help readability, please remove it.

Comment 41

[Chris]

Thanks for the review Florian.

I posted a new try run at:
     https://treeherder.mozilla.org/#/jobs?repo=try&revision=dae18d16e952

I'll post a checkin patch after this has completed.

Comment 42

[Chris]

Attachment: bug-1110767.patch

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/2c52d252d29b

Comment 44

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/2c52d252d29b

Comment 45

[Petruta Horea [:phorea], Desktop QA]

Tested with Nightly 40.0a1 2015-04-23 under Mac OS X 10.9.5 and Win 7 64-bit. Please see my results:

No text entered, click the Search magnifying glass icon:
 - nothing hovered: "Search with"
 - hover one-off search engines: "Search Wikipedia (en)"
 - hover Change Search Settings: "Search with"

Enter search item:
 - nothing hovered: "Search for <text> with:"
 - hover one-off search engines: "Search Wikipedia (en)"
 - hover Change Search Settings: "Search for <text> with:"
 - arrow key over search suggestions: "Search for <text> with:"
 - continue typing: "Search for <text> with:"

Also longer one-off search engine names are truncated and there are no issues when performing searches.

As far as I understood, this is the expected behavior, so I'm marking this as verified.
Please check my results and let me know if there's something that I've missed. Thanks!

Comment 46

[Florian Quèze [:florian]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #45)

Thanks!