Closed
Bug 1101996
Opened 10 years ago
Closed 9 years ago
Add icons to history-based suggestions in search bar dropdown
Categories
(Firefox :: Search, defect)
Tracking
()
People
(Reporter: phlsa, Assigned: dao)
References
Details
Attachments
(1 file, 9 obsolete files)
11.51 KB,
patch
|
mossop
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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!
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
Updated•10 years ago
|
Blocks: fx34-searchui
Updated•10 years ago
|
status-firefox34:
--- → wontfix
Updated•9 years ago
|
Flags: firefox-backlog+
Assignee | ||
Comment 4•9 years ago
|
||
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)
Updated•9 years ago
|
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: --- → 3
Flags: qe-verify+
Comment 5•9 years ago
|
||
The mockup for this is http://cl.ly/image/1t473m0e2k2u
Comment 7•9 years ago
|
||
(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)
Assignee | ||
Comment 8•9 years ago
|
||
(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)
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
colors adjusted
Attachment #8534624 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
Florian, do you know if we already have a property on the tree elements to differentiate history entries?
Flags: needinfo?(florian)
Comment 12•9 years ago
|
||
(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)
Assignee | ||
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8538697 -
Flags: review?(florian)
Comment 15•9 years ago
|
||
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 16•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8538491 -
Attachment is obsolete: true
Attachment #8538697 -
Attachment is obsolete: true
Attachment #8538782 -
Flags: review?(mak77)
Attachment #8538782 -
Flags: review?(florian)
Comment 18•9 years ago
|
||
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
Assignee | ||
Comment 19•9 years ago
|
||
(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?
Comment 20•9 years ago
|
||
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.
Assignee | ||
Comment 21•9 years ago
|
||
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?
Assignee | ||
Comment 22•9 years ago
|
||
(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.)
Assignee | ||
Comment 23•9 years ago
|
||
... 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.
Comment 24•9 years ago
|
||
(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?
Comment 25•9 years ago
|
||
(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.
Comment 26•9 years ago
|
||
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.
Assignee | ||
Comment 27•9 years ago
|
||
(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?
Comment 28•9 years ago
|
||
(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.
Assignee | ||
Comment 29•9 years ago
|
||
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.
Comment 30•9 years ago
|
||
(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.
Comment 31•9 years ago
|
||
(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?
Assignee | ||
Comment 32•9 years ago
|
||
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)
Comment 33•9 years ago
|
||
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 34•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8540100 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8540143 -
Attachment is obsolete: true
Assignee | ||
Comment 35•9 years ago
|
||
> ::: 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)
Updated•9 years ago
|
Attachment #8540172 -
Flags: review?(mak77) → review+
Comment 36•9 years ago
|
||
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.
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox37:
--- → affected
tracking-firefox35:
--- → +
tracking-firefox36:
--- → +
tracking-firefox37:
--- → +
Comment 37•9 years ago
|
||
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?
Comment 38•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8998763a34f5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment 39•9 years ago
|
||
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+
Comment 40•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ed7bc9bd681f https://hg.mozilla.org/releases/mozilla-beta/rev/2921722843b7
Comment 41•9 years ago
|
||
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
Comment 42•9 years ago
|
||
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.
Description
•