Log selections of searchSuggestTable in about:home.

VERIFIED FIXED in Firefox 35

Status

()

Firefox
General
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: bwinton, Assigned: bwinton)

Tracking

Trunk
Firefox 35
x86
All
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
…when it lands, that is.
Flags: firefox-backlog+
(Assignee)

Comment 1

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

Updated

4 years ago
Blocks: 1036925

Updated

4 years ago
QA Whiteboard: [qa+]

Updated

4 years ago
Points: --- → 2

Updated

4 years ago
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Iteration: --- → 34.1
QA Contact: florin.mezei
QA Contact: florin.mezei → petruta.rasa
Removed from Iteration 34.1 based on Blake's request since dependency 612453 is not ready.
Status: ASSIGNED → NEW
Iteration: 34.1 → ---
(Assignee)

Comment 3

4 years ago
(And removing myself, since I'm not taking this bug for this iteration.)
Assignee: bwinton → nobody

Updated

4 years ago
QA Whiteboard: [qa+]
Flags: qe-verify+

Updated

4 years ago
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Iteration: --- → 35.1
(Assignee)

Comment 4

4 years ago
Created attachment 8483511 [details] [diff] [review]
bug1036908.diff

Okay, let's give this a try, and see how it goes.  :)
Attachment #8483511 - Flags: review?(mak77)
(Assignee)

Comment 5

4 years ago
Created attachment 8484206 [details] [diff] [review]
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!
Attachment #8483511 - Attachment is obsolete: true
Attachment #8483511 - Flags: review?(mak77)
Attachment #8484206 - Flags: review?(mak77)
I started looking at this yesterday but was stolen by other stuff, no worries.
(Assignee)

Comment 7

4 years ago
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}}}}
(Assignee)

Updated

4 years ago
Blocks: 1036912
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.
Attachment #8484206 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 9

4 years ago
(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!
(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.
(Assignee)

Comment 11

4 years ago
Created attachment 8484548 [details] [diff] [review]
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!  :)
Attachment #8484206 - Attachment is obsolete: true
Attachment #8484548 - Flags: review?(mak77)
(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 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;
Attachment #8484548 - Flags: review?(mak77) → review+
oh well, then you could also just add
  selection: selectionData
when you build the eventData object...
(Assignee)

Comment 15

4 years ago
Created attachment 8485213 [details] [diff] [review]
The final version, ready for checkin.

And the final version, with all changes made.
Attachment #8484548 - Attachment is obsolete: true
Attachment #8485213 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/b556a4fdfe52
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 18

4 years ago
Fixed by the latest patch in bug 1036912.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/57b1a2b98388
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/57b1a2b98388
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Verified as fixed using Nightly 35.0a1 20140912030202 under Win 7 64-bit, Ubuntu 12.10 32-bit and Mac OSX 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.