1036908 - Log selections of searchSuggestTable in about:home.

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

…when it lands, that is.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

"Selections" mean both "clicks on" or "enter keypresses when focused".
Also, be sure to log the type of selection (enter key or mouse click).

Comment 2

[Marco Mucci [:MarcoM]]

Removed from Iteration 34.1 based on Blake's request since dependency 612453 is not ready.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

(And removing myself, since I'm not taking this bug for this iteration.)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug1036908.diff

Okay, let's give this a try, and see how it goes.  :)

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: An updated patch to account for hitting enter when not selecting.

While implementing a similar patch for about:newtab, I remembered that we might hit enter while not selecting one of the entries, and so this code covers that case as well.

(Also, if you don't want to review this, please feel free to redirect it to someone else.  :)

Thanks!

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I started looking at this yesterday but was stolen by other stuff, no worries.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

To test this change:

Open about:home
Make sure the search box is using Google.
Type "test".
Hit enter.

Open about:telemetry in a new tab.
Expand the "Simple Measurements" section.
Look for the "UITelemetry" key.
Make sure it contains:
"search":{"abouthome":1}}

Go back to about:home
Type "test"
Hit arrow-down, then enter.

Go back to about:telemetry
Refresh the page
Re-expand the "Simple Measurements" section.
Make sure the "UITelemetry" key now contains:
"search":{"abouthome":2,"selection":{"abouthome":{"0":{"key":1}}}}

Go back to about:home
Type "test"
Click on the first entry

Go back to about:telemetry
Refresh the page
Re-expand the "Simple Measurements" section.
Make sure the "UITelemetry" key now contains:
"search":{"abouthome":3,"selection":{"abouthome":{"0":{"key":1,"mouse":1}}}}

Go back to about:home
Type "test"
Click on the second entry

Go back to about:telemetry
Refresh the page
Re-expand the "Simple Measurements" section.
Make sure the "UITelemetry" key now contains:
"search":{"abouthome":4,"selection":{"abouthome":{"0":{"key":1,"mouse":1},"1":{"mouse":1}}}}

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8484206 [details] [diff] [review]
An updated patch to account for hitting enter when not selecting.

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

::: browser/base/content/abouthome/aboutHome.js
@@ +313,4 @@
>        engineName: engineName,
>        searchTerms: searchTerms
> +    };
> +    if (selectionIndex && selectionIndex != -1) {

why is this skipping selectionIndex == 0? that's a valid index, should be the first suggestion result... I guess you really wanted to check selectionIndex !== null?
it also doesn't seem to make much sense to set "selection-index" to -1, in first stance.

::: browser/base/content/browser.js
@@ +3120,5 @@
>     * @param engine
>     *        (nsISearchEngine) The engine handling the search.
>     * @param source
>     *        (string) Where the search originated from. See the FHR
>     *        SearchesProvider for allowed values.

need to update this javadoc to add selection description and specify it's [optional]

::: browser/base/content/searchSuggestionUI.js
@@ +151,5 @@
>        this._hideSuggestions();
>      }
>      this.selectedIndex = -1;
> +    this.input.removeAttribute("selection-index");
> +    this.input.removeAttribute("selection-kind");

Doesn't seem very important to do this oninput, since regardless you update the data on mouse/keyboard action.
Could make more sense to do it in _hideSuggestion.

Even better, I'd split an internal helper method to empty the suggestions table, and also do this additional cleanup there. Then use that helper method in both points where we currently clear the table: that is in _hideSuggestions and _onMsgSuggestions. So the handling would be a little bit more centralized.

@@ +183,5 @@
>          this.input.value = this.suggestionAtIndex(this.selectedIndex);
>        }
>        this._stickyInputValue = this.input.value;
> +      this.input.setAttribute("selection-index", this.selectedIndex);
> +      this.input.setAttribute("selection-kind", "key");

shouldn't these happen only if this.selectedIndex >=0? do you also care about plain enter in the input field?
(just test we won't report a previous value if we don't set these here every time)

@@ +291,5 @@
>  
>      this._table.hidden = false;
>      this.input.setAttribute("aria-expanded", "true");
> +    this.input.removeAttribute("selection-index");
> +    this.input.removeAttribute("selection-kind");

I'm tempted to say this should happen some rows earlier, just before we empty the table, from that point on the old index doesn't make sense anymore. If you split out a table cleanup helper as suggested above basically that will do for you.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

(In reply to Marco Bonardo [:mak] (needinfo? me) from comment #8)
> ::: browser/base/content/abouthome/aboutHome.js
> @@ +313,4 @@
> > +    };
> > +    if (selectionIndex && selectionIndex != -1) {
> 
> why is this skipping selectionIndex == 0? that's a valid index, should be
> the first suggestion result... I guess you really wanted to check
> selectionIndex !== null?

Doh!  Yes, I'll fix that.  Although, it seemed to work in my tests…  Any idea why?

> it also doesn't seem to make much sense to set "selection-index" to -1, in
> first stance.

That's not something I do.  It's behaviour I've seen when you hit enter in the text field without selecting anything.

> ::: browser/base/content/browser.js
> @@ +3120,5 @@
> >     * @param engine
> >     *        (nsISearchEngine) The engine handling the search.
> >     * @param source
> >     *        (string) Where the search originated from. See the FHR
> >     *        SearchesProvider for allowed values.
> need to update this javadoc to add selection description and specify it's
> [optional]

Will do!

> ::: browser/base/content/searchSuggestionUI.js
> @@ +151,5 @@
> >        this._hideSuggestions();
> >      }
> >      this.selectedIndex = -1;
> > +    this.input.removeAttribute("selection-index");
> > +    this.input.removeAttribute("selection-kind");
> 
> Doesn't seem very important to do this oninput, since regardless you update
> the data on mouse/keyboard action.
> Could make more sense to do it in _hideSuggestion.
> 
> Even better, I'd split an internal helper method to empty the suggestions
> table, and also do this additional cleanup there. Then use that helper
> method in both points where we currently clear the table: that is in
> _hideSuggestions and _onMsgSuggestions. So the handling would be a little
> bit more centralized.

Okay, I could do that.

> @@ +183,5 @@
> >          this.input.value = this.suggestionAtIndex(this.selectedIndex);
> >        }
> >        this._stickyInputValue = this.input.value;
> > +      this.input.setAttribute("selection-index", this.selectedIndex);
> > +      this.input.setAttribute("selection-kind", "key");
> 
> shouldn't these happen only if this.selectedIndex >=0? do you also care
> about plain enter in the input field?
> (just test we won't report a previous value if we don't set these here every
> time)

I think I care about not logging if we hit enter in the input field.

> @@ +291,5 @@
> >  
> >      this._table.hidden = false;
> >      this.input.setAttribute("aria-expanded", "true");
> > +    this.input.removeAttribute("selection-index");
> > +    this.input.removeAttribute("selection-kind");
> 
> I'm tempted to say this should happen some rows earlier, just before we
> empty the table, from that point on the old index doesn't make sense
> anymore. If you split out a table cleanup helper as suggested above
> basically that will do for you.

Will do.

Thanks for the review!

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Blake Winton (:bwinton) from comment #9)
> (In reply to Marco Bonardo [:mak] (needinfo? me) from comment #8)> 
> > it also doesn't seem to make much sense to set "selection-index" to -1, in
> > first stance.
> 
> That's not something I do.  It's behaviour I've seen when you hit enter in
> the text field without selecting anything.

yes, it's because you just set the attribute without checking this.selectedIndex >=0... As I said that might solve this problem but you should ensure you won't wrongly bring over the old index from the previous search.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Attachment: Attempt number 3. :)

(In reply to Blake Winton (:bwinton) from comment #9)
> > ::: browser/base/content/abouthome/aboutHome.js
> > @@ +313,4 @@
> > > +    };
> > > +    if (selectionIndex && selectionIndex != -1) {
> > why is this skipping selectionIndex == 0? that's a valid index, should be
> > the first suggestion result... I guess you really wanted to check
> > selectionIndex !== null?

Switched to 'if (selectionIndex !== "")' since selectionIndex is an attribute (and so will return empty or a string).  I'm now removing (or not setting) the attribute if the selectedIndex is -1, as you mentioned.

> > ::: browser/base/content/browser.js
> > @@ +3120,5 @@
> > >     * @param engine
> > >     *        (nsISearchEngine) The engine handling the search.
> > >     * @param source
> > >     *        (string) Where the search originated from. See the FHR
> > >     *        SearchesProvider for allowed values.
> > need to update this javadoc to add selection description and specify it's
> > [optional]

Fixed.

> > ::: browser/base/content/searchSuggestionUI.js
> > @@ +151,5 @@
> > >        this._hideSuggestions();
> > >      }
> > >      this.selectedIndex = -1;
> > > +    this.input.removeAttribute("selection-index");
> > > +    this.input.removeAttribute("selection-kind");
> > 
> > Doesn't seem very important to do this oninput, since regardless you update
> > the data on mouse/keyboard action.

Removed.

> > Could make more sense to do it in _hideSuggestion.

I can't, because I want these to persist after the popup is hidden.  (This is also why I skipped the internal helper method, even though I liked that idea.)

> > @@ +183,5 @@
> > >          this.input.value = this.suggestionAtIndex(this.selectedIndex);
> > >        }
> > >        this._stickyInputValue = this.input.value;
> > > +      this.input.setAttribute("selection-index", this.selectedIndex);
> > > +      this.input.setAttribute("selection-kind", "key");
> > 
> > shouldn't these happen only if this.selectedIndex >=0? do you also care
> > about plain enter in the input field?
> > (just test we won't report a previous value if we don't set these here every
> > time)

Yes.  Changed so that they get set if selectedIndex >= 0, and removed otherwise.

> > @@ +291,5 @@
> > > +    this.input.removeAttribute("selection-index");
> > > +    this.input.removeAttribute("selection-kind");
> > I'm tempted to say this should happen some rows earlier, just before we
> > empty the table, from that point on the old index doesn't make sense
> > anymore. If you split out a table cleanup helper as suggested above
> > basically that will do for you.

So, since I'm now clearing them on enter (if we have nothing selected), and since I couldn't add the helper method for the reasons above, I just removed this code, because they'll be cleared (or set) when something is picked.

I think that's all, but please let me know if I missed anything, or if there's anything else you'ld like changed!  Thanks!  :)

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Blake Winton (:bwinton) from comment #11)
> > > Could make more sense to do it in _hideSuggestion.
> 
> I can't, because I want these to persist after the popup is hidden.  (This
> is also why I skipped the internal helper method, even though I liked that
> idea.)

oops, you're right, sorry for the red herring.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8484548 [details] [diff] [review]
Attempt number 3.  :)

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

::: browser/base/content/abouthome/aboutHome.js
@@ +313,4 @@
>        engineName: engineName,
>        searchTerms: searchTerms
> +    };
> +    if (selectionIndex !== "") {

I hate the discrepancy between empty getAttribute in xul and the web. what about moving to a safer hasAttribute here? above:

let selectionData;
if (searchText.hasAttribute("selection-index")) {
  selectionData = {
    index: searchText.getAttribute("selection-index"),
    kind: searchText.getAttribute("selection-kind");
  }
}

and later just:
eventData.selection = selectionData;

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

oh well, then you could also just add
  selection: selectionData
when you build the eventData object...

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: The final version, ready for checkin.

And the final version, with all changes made.

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Carsten Book [:Tomcat]]

sorry had to backout this changes (for Bug 1036914, Bug 1036912 and Bug 1036908) since one of this changes caused test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=47585310&tree=Fx-Team

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Fixed by the latest patch in bug 1036912.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/57b1a2b98388

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/57b1a2b98388

Comment 21

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Nightly 35.0a1 20140912030202 under Win 7 64-bit, Ubuntu 12.10 32-bit and Mac OSX 10.9.5.