bold part of the suggestion "button" that is not mUserSearchTerm for search suggestions

RESOLVED FIXED in Firefox 44

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ally, Assigned: ally)

Tracking

unspecified
Firefox 44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
https://invis.io/6749S3PTN <-- worth a 1000 words

Note that mUserSearchTerm, (text that the user has typed in the urlbar) may be any part of search suggestion string. In the case of the first 'button' is the entire string (see 1201670 for whether or not we'll even keep that button long term).

This is expected to apply to both saved search queries and suggestions supplied from the default search engine.
(Assignee)

Updated

2 years ago
Blocks: 1086952
(Assignee)

Updated

2 years ago
Assignee: nobody → ally
(Assignee)

Comment 1

2 years ago
Created attachment 8672732 [details]
device-2015-10-12-150520.png
Flags: needinfo?(alam)
(Assignee)

Comment 2

2 years ago
Created attachment 8672761 [details] [diff] [review]
SavedSearchesBoldSuggestions

Notes about a couple cases not covered in the mock:

- If mUserSearchTerm (the text the user has in the urlbar) is not found, nothing is bolded (yahoo returns "dog quest" for the search term "dogq" for example)

- If there is more than one copy of mUserSearchTerm, only the first is not bolded, see screenshot to follow. We won't have that many cases where this applies. Most of the time each word in a search query is unique.

Eng Notes:
- There are two identical style spans because if you reuse a style, SpannableStringBuilder interprets that to mean changing/moving the original span, not setting a second new span. After much stackoverflow, I still don't like that setSpan() does this, but I can see the logic of wanting the api to allow people to move a span around without destroying it.
(Assignee)

Comment 3

2 years ago
Created attachment 8672762 [details]
dogq example
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #2)
> Created attachment 8672761 [details] [diff] [review]
> SavedSearchesBoldSuggestions
> 
> Notes about a couple cases not covered in the mock:
> 
> - If mUserSearchTerm (the text the user has in the urlbar) is not found,
> nothing is bolded (yahoo returns "dog quest" for the search term "dogq" for
> example)

Can this even happen? We explicitly place the user entered text in the SearchEngineRow, right?

> - If there is more than one copy of mUserSearchTerm, only the first is not
> bolded, see screenshot to follow. We won't have that many cases where this
> applies. Most of the time each word in a search query is unique.

I suppose this depends on the answer to the above, but this makes me think even more that we are no longer adding the user entered text in the row.

> Eng Notes:
> - There are two identical style spans because if you reuse a style,
> SpannableStringBuilder interprets that to mean changing/moving the original
> span, not setting a second new span. After much stackoverflow, I still don't
> like that setSpan() does this, but I can see the logic of wanting the api to
> allow people to move a span around without destroying it.

I see the mocks "bold" the part of the string that is _not_ what the user typed. Why is that?
(Assignee)

Comment 5

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4e09412c42d
(Assignee)

Comment 6

2 years ago
(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #2)
> > Created attachment 8672761 [details] [diff] [review]
> > SavedSearchesBoldSuggestions
> > 
> > Notes about a couple cases not covered in the mock:
> > 
> > - If mUserSearchTerm (the text the user has in the urlbar) is not found,
> > nothing is bolded (yahoo returns "dog quest" for the search term "dogq" for
> > example)
> 
> Can this even happen? We explicitly place the user entered text in the
> SearchEngineRow, right?

Sure, for the first of the buttons. This is per button. The ones from yahoo, we have no control over and don't enforce any sort of rules about what a suggestion can be or that it must contain mUserSearchTerm, just the number.  See screenshot of the 'dogq' example.


> 
> > - If there is more than one copy of mUserSearchTerm, only the first is not
> > bolded, see screenshot to follow. We won't have that many cases where this
> > applies. Most of the time each word in a search query is unique.
> 
> I suppose this depends on the answer to the above, but this makes me think
> even more that we are no longer adding the user entered text in the row.

Per suggestion button.

> 
> > Eng Notes:
> > - There are two identical style spans because if you reuse a style,
> > SpannableStringBuilder interprets that to mean changing/moving the original
> > span, not setting a second new span. After much stackoverflow, I still don't
> > like that setSpan() does this, but I can see the logic of wanting the api to
> > allow people to move a span around without destroying it.
> 
> I see the mocks "bold" the part of the string that is _not_ what the user
> typed. Why is that?

To draw attention to what is _different_ about each suggestion button, as I understand it from :antlam.
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #1)
> Created attachment 8672732 [details]
> device-2015-10-12-150520.png

This is looking good!(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #6)
> (In reply to Mark Finkle (:mfinkle) from comment #4)
> > I see the mocks "bold" the part of the string that is _not_ what the user
> > typed. Why is that?
> 
> To draw attention to what is _different_ about each suggestion button, as I
> understand it from :antlam.

yep! exactly :) AFICT, there's still no unified best practice for this right now. From my experience though, this is making the most sense. Drawing attention to multiple instances of what the user already typed seemed overkill.
Flags: needinfo?(alam)
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #1)
> Created attachment 8672732 [details]
> device-2015-10-12-150520.png

BTW This is looking great ally! well done! \o/
(Assignee)

Comment 9

2 years ago
Comment on attachment 8672761 [details] [diff] [review]
SavedSearchesBoldSuggestions

Please keep in mind the notes in comment 2 :)
Attachment #8672761 - Flags: review?(michael.l.comella)
Comment on attachment 8672761 [details] [diff] [review]
SavedSearchesBoldSuggestions

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

(In reply to Allison Naaktgeboren PTO(Oct16-Nov-2) from comment #2)
> - If there is more than one copy of mUserSearchTerm, only the first is not
> bolded, see screenshot to follow. We won't have that many cases where this
> applies. Most of the time each word in a search query is unique.

This sucks, but I can see the that the added complexity sucks more if we need a new span for each bold. wfm, though potential mentored follow-up?

> - There are two identical style spans because if you reuse a style,
> SpannableStringBuilder interprets that to mean changing/moving the original
> span, not setting a second new span. After much stackoverflow, I still don't
> like that setSpan() does this, but I can see the logic of wanting the api to
> allow people to move a span around without destroying it.

The Android source could also clue you in here, but it's probably not worth the time.

::: mobile/android/base/home/SearchEngineRow.java
@@ +173,5 @@
>          }
>  
>          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
> +        String searchTerm = getSuggestionTextFromView(mUserEnteredView);
> +        int startOfSearchTerm = suggestion.indexOf(searchTerm);

nit: final, here & elsewhere

@@ +174,5 @@
>  
>          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
> +        String searchTerm = getSuggestionTextFromView(mUserEnteredView);
> +        int startOfSearchTerm = suggestion.indexOf(searchTerm);
> +        

nit: ws. If you're running IJ, you can configure it to automatically remove WS on save.
Attachment #8672761 - Flags: review?(michael.l.comella) → review+
> (In reply to Allison Naaktgeboren PTO(Oct16-Nov-2) from comment #2)
> > - If there is more than one copy of mUserSearchTerm, only the first is not
> > bolded, see screenshot to follow. We won't have that many cases where this
> > applies. Most of the time each word in a search query is unique.
> 
> This sucks, but I can see the that the added complexity sucks more if we
> need a new span for each bold. wfm, though potential mentored follow-up?

Also, add a comment to say this is intended.

Comment 12

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/55bd1cef745f
(Assignee)

Comment 13

2 years ago
Bug 1214811 filed for mentored followup
https://hg.mozilla.org/mozilla-central/rev/55bd1cef745f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.