Closed Bug 1101996 Opened 5 years ago Closed 5 years ago

Add icons to history-based suggestions in search bar dropdown

Categories

(Firefox :: Search, defect)

36 Branch
x86
All
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox34 - wontfix
firefox35 + verified
firefox36 + verified
firefox37 + verified

People

(Reporter: phlsa, Assigned: dao)

References

Details

Attachments

(1 file, 9 obsolete files)

We want to indicate which suggestions come from the users history in the search bar dropdown.

@Stephen: Could you upload the icons/mockups for that here? Thanks!
[Tracking Requested - why for this release]:
Flags: firefox-backlog+
Should the icon be aligned to the left or to the right?

Can we use an SVG icon here that would automatically adapt to varying text sizes?
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: --- → 3
Flags: qe-verify+
Thanks, this answers my first question.
Flags: needinfo?(philipp)
(In reply to Dão Gottwald [:dao] from comment #4)
> Should the icon be aligned to the left or to the right?
> 
> Can we use an SVG icon here that would automatically adapt to varying text
> sizes?

Left aligned as seen in Comment 5. SVG is fine with me, but do you mean that the icon should also scale based on text size?
Flags: needinfo?(shorlander) → needinfo?(dao)
(In reply to Stephen Horlander [:shorlander] from comment #7)
> SVG is fine with me, but do you mean that the icon should also scale based on text size?

Yes, although even if we didn't want this I would still want to use SVG instead of two PNGs as it allows us to be smarter about the color, e.g. use the same native color as the text but with reduced opacity. Can you provide the SVG?
Flags: needinfo?(dao) → needinfo?(shorlander)
Attached image icon-search-history.svg (obsolete) —
Search History icon. Made it the color it should be in most dark on light cases. Might take some tweaking to get it to be more adaptable.
Attachment #8525695 - Attachment is obsolete: true
Attachment #8525697 - Attachment is obsolete: true
Flags: needinfo?(shorlander)
Attached image search-history-icon.svg (obsolete) —
colors adjusted
Attachment #8534624 - Attachment is obsolete: true
Florian, do you know if we already have a property on the tree elements to differentiate history entries?
Flags: needinfo?(florian)
(In reply to Dão Gottwald [:dao] from comment #11)
> Florian, do you know if we already have a property on the tree elements to
> differentiate history entries?

The only property we already have is "suggesthint" on the first suggestion row (and "suggestfirst" if the first suggestion row is also the very first row).

It's used at http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#1487 and implemented at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormAutoCompleteResult.jsm#126

It's not what you need, but can maybe help you as a starting point.
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> It's not what you need, but can maybe help you as a starting point.

Yeah, thanks for the pointer.
Attached patch patch (obsolete) — Splinter Review
Attachment #8538697 - Flags: review?(florian)
Comment on attachment 8538697 [details] [diff] [review]
patch

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

Just some small comments on the SVG.

::: browser/themes/shared/search/history-icon.svg
@@ +8,5 @@
> +     y="0"
> +     width="16"
> +     height="16"
> +     viewBox="0 0 16 16">
> +<style type="text/css">

type="text/css" can be removed.

@@ +9,5 @@
> +     width="16"
> +     height="16"
> +     viewBox="0 0 16 16">
> +<style type="text/css">
> +<![CDATA[

<!CDATA[ isn't needed.

@@ +23,5 @@
> +use[id$="-active"] {
> +  fill: HighlightText;
> +}
> +
> +]]>

Same for this line.
Comment on attachment 8538697 [details] [diff] [review]
patch

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

Looks good to me overall, but there's some misalignment (at least on Mac): http://i.imgur.com/PVVPo88.png
- it seems the suggestion text is 2px off to the right; it should be aligned with the textfield's text.
- the mockup in comment 5 had the clock icon horizontally centered with the search engine icon. It seems your patch has them right aligned.

Note: I'm not officially a toolkit peer so you may want to ask someone to rs the nsFormAutoCompleteResult.jsm change.
Attachment #8538697 - Flags: review?(florian) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8538491 - Attachment is obsolete: true
Attachment #8538697 - Attachment is obsolete: true
Attachment #8538782 - Flags: review?(mak77)
Attachment #8538782 - Flags: review?(florian)
The text field's text and the suggestions text still doesn't seem aligned on Mac:
retina screenshot: http://i6.minus.com/ibjgU3LFeSfanl.png
non-retina screenshot: http://i2.minus.com/iTS2aXm1Jj3PW.png
(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> The text field's text and the suggestions text still doesn't seem aligned on
> Mac:
> retina screenshot: http://i6.minus.com/ibjgU3LFeSfanl.png
> non-retina screenshot: http://i2.minus.com/iTS2aXm1Jj3PW.png

So you're asking me to move the text further to the right, while keeping the clock both centered horizontally and aligned with the engine icon. That's not going to work out. ;) It seems that the engine label is misaligned; different bug?
All I'm saying is the alignments on these 2 lines http://i.imgur.com/EzrCn3d.png are intentional and shouldn't be broken unless we explicitly decide to do so.
Oh, you're talking about the text input. Similar problem, then: if I move the suggestions left, then the clock isn't centered horizontally, and if I move the clock left, then it doesn't line up with the engine icon. So this seems that I'd need to reshuffle a number of other things in that popup, e.g. the engine icon / label, and then the one-off search header too. Can this be a different bug?
(I don't have a Mac and won't be able to make these changes before the weekend, be it in this bug or in a separate one.)
... and it looks like I won't have enough time to land this today either. So if you choose to take this patch for now, feel free to land it. Presumably any followup alignment changes wouldn't be too complex code-wise and could still be uplifted in the next two weeks.
(In reply to Dão Gottwald [:dao] from comment #21)
> Oh, you're talking about the text input. Similar problem, then: if I move
> the suggestions left, then the clock isn't centered horizontally, and if I
> move the clock left, then it doesn't line up with the engine icon.

I don't understand this. All the things are aligned correctly on the mockup: http://cl.ly/image/1t473m0e2k2u
What in this mockup isn't implementable?
(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> The text field's text and the suggestions text still doesn't seem aligned on
> Mac:
> retina screenshot: http://i6.minus.com/ibjgU3LFeSfanl.png
> non-retina screenshot: http://i2.minus.com/iTS2aXm1Jj3PW.png

This looks aligned enough to me, there's slightly more padding on the left though.
But the mockup (http://cl.ly/image/1t473m0e2k2u) also has more padding on the left.
Attached patch Alignment on Windows (obsolete) — Splinter Review
http://i.imgur.com/S88otKb.png is the best I could get on Windows. I ended up left-aligning the history icon with the engine icon otherwise it was almost touching the text.
(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> (In reply to Dão Gottwald [:dao] from comment #21)
> > Oh, you're talking about the text input. Similar problem, then: if I move
> > the suggestions left, then the clock isn't centered horizontally, and if I
> > move the clock left, then it doesn't line up with the engine icon.
> 
> I don't understand this. All the things are aligned correctly on the mockup:
> http://cl.ly/image/1t473m0e2k2u
> What in this mockup isn't implementable?

I didn't say anything was unimplementable. I said more things in the popup needed to be adjusted and that I didn't have time for that before the weekend.

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Created attachment 8540100 [details] [diff] [review]
> Alignment on Windows
> 
> http://i.imgur.com/S88otKb.png is the best I could get on Windows. I ended
> up left-aligning the history icon with the engine icon otherwise it was
> almost touching the text.

This looks pretty bad to me. It seems that, to match the mockup, the search glass icon in the text field needs to move more to the right. Do you want me to do that?
(In reply to Dão Gottwald [:dao] from comment #27)

> It seems that, to match the mockup, the search
> glass icon in the text field needs to move more to the right. Do you want me
> to do that?

That's probably the right long term solution (also note that we need to replace the current glass icon with something platform specific to match the other icons; shorlander as a note somewhere that new icons are needed for this).

For something to uplift, I would really prefer not touching anything outside the panel and not changing the appearance of non-history results.
I'd suggest taking my patch as the short-term solution then, since with your change the history icon is off-center and the popup looks rather crammed.
(In reply to Dão Gottwald [:dao] from comment #29)
> I'd suggest taking my patch as the short-term solution

Your patch moves the non-history items a lot to the right, creating a large empty area.
(In reply to Dão Gottwald [:dao] from comment #29)
> with your change the history icon is off-center

Would you prefer centering that icon in the empty area and ignoring completely the alignment with the engine icon?
Attached patch patch v3 (obsolete) — Splinter Review
I think this should be better...
Attachment #8538782 - Attachment is obsolete: true
Attachment #8538782 - Flags: review?(mak77)
Attachment #8538782 - Flags: review?(florian)
Attachment #8540140 - Flags: review?(florian)
Attached patch Alignment on Linux (obsolete) — Splinter Review
Same thing as attachment 8540100 [details] [diff] [review] but adapted for Linux. This actually looks better on Linux than Windows: http://i.imgur.com/L430Q1Z.png

Going to look at your updated patch now. Just wanted to save this somewhere as I already had it and will likely reboot to Windows to test your patch.
Comment on attachment 8540140 [details] [diff] [review]
patch v3

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

Looks good enough. Being able to reduce the size of the icon without requesting a new image file from UX is a nice benefit of using SVG here :-).

The text of the suggestions is still not perfectly aligned with the text in the input box, but if I wasn't paying specific attention to this detail, I don't think I would have noticed it while using the browser.

In my testing of this patch, the suggestions appeared a tiny bit less than 1px to the left on Mac (subpixel anti-aliasing makes the exact value hard to figure out), 1px to the right on Linux, and 2px to the right on Windows. Adding 1px of padding on Mac is trivial, removing 1 or 2 on Linux/Windows would be more difficult for the reasons we discussed before.

::: browser/themes/osx/searchbar.css
@@ +261,5 @@
>  }
>  
> +.search-panel-tree > .autocomplete-treebody::-moz-tree-image {
> +  -moz-padding-start: 4px;
> +  -moz-padding-end: 1px;

This looks slightly better with -moz-padding-end: 2px; but I may be the only one who will notice.
Attachment #8540140 - Flags: review?(florian) → review+
Attached patch patch v4Splinter Review
> ::: browser/themes/osx/searchbar.css
> @@ +261,5 @@
> >  }
> >  
> > +.search-panel-tree > .autocomplete-treebody::-moz-tree-image {
> > +  -moz-padding-start: 4px;
> > +  -moz-padding-end: 1px;
> 
> This looks slightly better with -moz-padding-end: 2px; but I may be the only
> one who will notice.

done

Marco, please review the nsFormAutoCompleteResult.jsm bit.
Attachment #8540140 - Attachment is obsolete: true
Attachment #8540172 - Flags: review?(mak77)
Attachment #8540172 - Flags: review?(mak77) → review+
Tracking this as part of search refinements targeted for 35.  This will have to be nominated (and have a low risk) by Mon Dec 29th to get into the final beta of 35.
Comment on attachment 8540172 [details] [diff] [review]
patch v4

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

Approval Request Comment
[Feature/regressing bug #]: not showing any distinction between history suggestions and remote suggestions was kind of a regression of the design implemented in bug 1088660. This restores the visual distinction by showing an icon.
[User impact if declined]: No way to know if a suggestion is from the history or a remote server.
[Describe test coverage new/current, TBPL]: I tested the patch locally when reviewing.
[Risks and why]: I think still acceptable; the patch looks larger than it really is, and it contains a CSS addition duplicated for our 3 supported platforms.
[String/UUID change made/needed]: none.
Attachment #8540172 - Flags: approval-mozilla-beta?
Attachment #8540172 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8998763a34f5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment on attachment 8540172 [details] [diff] [review]
patch v4

Approving for uplift since these look to have gotten merged to central in https://hg.mozilla.org/mozilla-central/rev/d5167dc0ded3
Attachment #8540172 - Flags: approval-mozilla-beta?
Attachment #8540172 - Flags: approval-mozilla-beta+
Attachment #8540172 - Flags: approval-mozilla-aurora?
Attachment #8540172 - Flags: approval-mozilla-aurora+
Verified fixed on Firefox 35.0b6 (20141222200458) using Ubuntu 12.04 LTS 32-bit, Windows 8.1 64-bit and Mac OS X 10.9.5.

Slight misalignments are visible (trivial stuff), but that has been already discussed throughout the comments of this bug.
Status: RESOLVED → VERIFIED
Blocks: 1115307
Verified as fixed using Developer Edition 36.0a2 and Nightly 37.0a1 2014-01-05 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.
You need to log in before you can comment on or make changes to this bug.