Closed Bug 1427350 Opened 7 years ago Closed 7 years ago

Use "autocomplete-rich-result-popup" instead of "autocomplete-result-popup" for the search bar

Categories

(Firefox :: Search, task)

Product:
Firefox
Component:
Search
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1427350 - Part 2 - Rename _matchCount to matchCount.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1427350 - Part 1 - Use "autocomplete-rich-result-popup" instead of "autocomplete-result-popup" for the search bar.
59 bytes, text/x-review-board-request
mak
: review+
Details
We should use "autocomplete-rich-result-popup" instead of "autocomplete-result-popup" for the search bar. This will provide highlighting of the typed words and allow removing "autocomplete-result-popup" eventually.
Comment on attachment 8939080 [details] Bug 1427350 - Part 1 - Use "autocomplete-rich-result-popup" instead of "autocomplete-result-popup" for the search bar. This is a preliminary version, not sure everything is correct. I borrowed some code from the base binding and replaced a few references. Marco, how can I style the results so that they appear similar to the ones in the location bar, minus the "Search with" on hover? Is this done just with CSS or should the results be modified? We may also want to use a different icon for search history results and remote suggestions, if this is not too difficult. I suppose we want the same item height as the location bar, with results slightly taller than now.
Attachment #8939080 - Flags: ui-review?(mak77)
Flags: needinfo?(mak77)
Blocks: 1427363
(In reply to :Paolo Amadini from comment #2) > Marco, how can I style the results so that they appear similar to the ones > in the location bar, minus the "Search with" on hover? Is this done just > with CSS or should the results be modified? What do you mean by "similar"? I don't think the scope of this bug is to visually unify the 2 bars, since that would require UX and owner involvement; the most we can keep things similar to the status-quo, the simpler will be to land this. Regarding how to modify the results, IIRC most is done through css, by checking attributes (things like "actiontypes")and display:none the non relevant parts. It's non-trivial because unfortunately a lot of details that should be urlbar only were put into the base widget, so it may require some tweaking. > We may also want to use a different icon for search history results and > remote suggestions, if this is not too difficult. Yes, we must retain all the existing features, the current panel shows a "clock" icon before history results and that's what we should still do. > I suppose we want the same > item height as the location bar, with results slightly taller than now. As I said, I think coalescing a code change and a style change in the same bug will just delay this, since at least it will require approvals from UX and the module owner. Personally I'd first try to just replace the widget maintaining the existing look, and then the style can be uniformed in a follow-up. Unless retaining the current style is far more expensive.
Flags: needinfo?(mak77)
Testing the patch, the functionality looks ok (there may be edge cases in the behavior though, that's why we should land this soon after a version merge imo), the styling is off, but that's what you meant before, it still needs work and imo we should just make it look like the existing one for now.
Attachment #8939080 - Flags: ui-review?(mak77)
The styling updates turned out to be simple, assuming we don't have to be pixel-perfect, if we just want to keep the current look, with the bonus of the search term highlighting. Note that there is an existing bug with the number of results not being really limited to 10, although now the height accommodates only 10 items the first time the panel is opened, because of the "maxrows" attribute. Just like we're doing for the styling, I'm inclined to think that fixing this bug by enforcing "maxrows" is out of scope here.
Comment on attachment 8939080 [details] Bug 1427350 - Part 1 - Use "autocomplete-rich-result-popup" instead of "autocomplete-result-popup" for the search bar. https://reviewboard.mozilla.org/r/209508/#review217496 The historical result icon seems to appear on every entry for me, not just historical entries, this should be fixed before the patch can be taken. The only other diff I see, is the text size, it's larger than before. I'm honestly not 100% sure whether that's a problem, the other text in the same panel is smaller, and that gives the whole thing a patchwork look. Though also the awesomebar has a smaller "search for..." text in the footer and this makes the 2 fields a bit more coherent. Thus probably not a big deal? Final thought, I wonder if we should try to land this in 59 or wait the merge to 60. ::: browser/components/search/content/search.xml:846 (Diff revision 2) > if (aEvent.getModifierState("Accel")) > return; > > let suggestionsHidden = > - popup.tree.getAttribute("collapsed") == "true"; > - let numItems = suggestionsHidden ? 0 : this.popup.view.rowCount; > + popup.richlistbox.getAttribute("collapsed") == "true"; > + let numItems = suggestionsHidden ? 0 : this.popup._matchCount; let's remove the "_" from popup._matchCount, since it's often used from the outside, at this point let's make it properly exposed. There aren't many uses https://searchfox.org/mozilla-central/search?q=._matchCount&path= (exclude the devtools instance) ::: browser/themes/linux/browser.css (Diff revision 2) > - color: GrayText; > - font-size: smaller; > -} > - > -.autocomplete-treebody::-moz-tree-cell(suggesthint) { > - border-top: 1px solid GrayText; ugh, this made me notice tags autocomplete seems to use tree autocomplete and it still uses this, even if I'm not sure what's the point of this border in general... But these explain why tag autocomplete looks out of context (graytext and smaller font). Check the Star panel for example. Btw, with these changes this code seems to become pointless and should be removed. https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/toolkit/components/places/nsTaggingService.js#548
Attachment #8939080 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #7) > The historical result icon seems to appear on every entry for me, not just > historical entries, this should be fixed before the patch can be taken. This is the case already, right? It's something I mentioned in comment 2, but since it's not a regression it is likely out of scope. > The only other diff I see, is the text size, it's larger than before. I'm > honestly not 100% sure whether that's a problem, the other text in the same > panel is smaller, and that gives the whole thing a patchwork look. Though > also the awesomebar has a smaller "search for..." text in the footer and > this makes the 2 fields a bit more coherent. Thus probably not a big deal? Yeah, I don't think this is an issue in practice. > Final thought, I wonder if we should try to land this in 59 or wait the > merge to 60. I don't feel strongly either way.
(In reply to :Paolo Amadini from comment #8) > (In reply to Marco Bonardo [::mak] from comment #7) > > The historical result icon seems to appear on every entry for me, not just > > historical entries, this should be fixed before the patch can be taken. > > This is the case already, right? It's something I mentioned in comment 2, > but since it's not a regression it is likely out of scope. It's not the case for me, at least on windows I see the clock icon on historical entries, nothing on the others.
Maybe it was a bug only on Mac, I'll take a look to fix it cross-platform.
I fixed some tests, but I don't know why this block isn't entered anymore, as I don't know how the autocomplete selection works: https://dxr.mozilla.org/mozilla-central/rev/474d58c9137360c0fa1c85cdd11e3313b33b7cad/toolkit/content/widgets/autocomplete.xml#504-509 Also, this code should probably check "index" rather than "currentIndex", but it appears to be irrelevant: https://dxr.mozilla.org/mozilla-central/rev/474d58c9137360c0fa1c85cdd11e3313b33b7cad/browser/components/search/content/search.xml#811-812 Marco, can you tell me how to handle the selection telemetry properly? Considering that we only have to support richlistbox autocomplete, maybe there is a better way than the one this code is using. The variable naming implies that these code paths are used only for telemetry.
Flags: needinfo?(mak77)
(In reply to :Paolo Amadini from comment #16) > I fixed some tests, but I don't know why this block isn't entered anymore, > as I don't know how the autocomplete selection works: > > https://dxr.mozilla.org/mozilla-central/rev/ > 474d58c9137360c0fa1c85cdd11e3313b33b7cad/toolkit/content/widgets/ > autocomplete.xml#504-509 This code looks completely broken from the beginning. It was landed in Bug 1102937, for UITelemetry (that likely we don't care about anymore). It seems to use nsAutoCompleteController::GetSelection(), that pretty much just returns mSelection, that can only be set by SetSelection. it's an nsITreeSelection, so I suppose it only matters for tree autocomplete. I cannot find any calls to "->SetSelection(", nor I cannot find any ".selection =" Off-hand looks like Set/GetSelection is a dead API that should be removed. And this code is just wrong. I think it should just check this.popup.selectedIndex >= 0, and use the same value for index. > Also, this code should probably check "index" rather than "currentIndex", > but it appears to be irrelevant: well, undefined != 1, so the check passes always, that means telemetrySearchDetails looks like being set also without a selection... it's surely a bug. > Considering that we only have to support richlistbox autocomplete, maybe > there is a better way than the one this code is using. The variable naming > implies that these code paths are used only for telemetry. I can't exclude that, indeed the whole thing is over-complex and comes from many different projects and hands. Exactly due to that, I'd probably avoid coalescing a telemetry code cleanup here. I hope the above is enough to unblock you, otherwise I can spend more time on it and do a deeper analysis of how/where things are used. I doubt I'd take less time than anyone else on that, since it predates most of the changes I worked on.
Flags: needinfo?(mak77)
Ah, Get/SetSelection are in reality part of nsITreeView, so I was partially wrong, it's not a dead API. Maybe the code is not really broken (apart the currentIndex thing), but selection is set by the tree itself.
and nsITreeSelection.idl has a currentIndex attribute too! so that may also be right. Btw, it's all related to supporting the tree, so we likely don't care and can use popup.selectedIndex regardless.
Comment on attachment 8945801 [details] Bug 1427350 - Part 2 - Rename _matchCount to matchCount. https://reviewboard.mozilla.org/r/215900/#review223304
Attachment #8945801 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #19) > and nsITreeSelection.idl has a currentIndex attribute too! so that may also > be right. The property is on the newly created object, so effectively the check doesn't take effect. I've just removed it because the index is checked again later. For the rest, I followed your suggestion for getting the correct selection index, thanks a lot for taking a look! I also rebased the patch on top of bug 1435019, and it now fixes the issue with the color of the historical suggestions icon.
Attachment #8939080 - Attachment is obsolete: true
Attachment #8939080 - Flags: review?(mak77)
All the tests seem to pass, so this should be ready for the final review.
Comment on attachment 8948258 [details] Bug 1427350 - Part 1 - Use "autocomplete-rich-result-popup" instead of "autocomplete-result-popup" for the search bar. https://reviewboard.mozilla.org/r/217760/#review223570 I'd suggest to run a more complete series of tests on Try, for example in the other case a11y tests failed. Also, please file a bug in Toolkit / Autocomplete about removing the controller support of tree autocomplete, depending on this; I may work on it in the near future.
Attachment #8948258 - Flags: review?(mak77) → review+
Blocks: 1435711
Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/0969471cf149 Part 1 - Use "autocomplete-rich-result-popup" instead of "autocomplete-result-popup" for the search bar. r=mak https://hg.mozilla.org/integration/mozilla-inbound/rev/d5c0c1275597 Part 2 - Rename _matchCount to matchCount. r=mak
https://hg.mozilla.org/mozilla-central/rev/0969471cf149 https://hg.mozilla.org/mozilla-central/rev/d5c0c1275597
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1436290
Depends on: 1436876
Depends on: 1457119
Depends on: 1498567
Type: enhancement → task
Regressions: 1529220
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: