Remove the "searchbar-treebody" binding and instead migrate the behavior to an attribute on "autocomplete-treebody"

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bgrins, Assigned: 86ecce74)

Tracking

(Blocks 1 bug)

unspecified
Firefox 59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [xbl-flatten-inheritance][xbl-available])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
The "autocomplete-treebody" binding is extended by one other binding, "searchbar-treebody".

This allows the searchbar-treebody binding to preventDefault on a mousemove [0] as a way to signal to the parent binding to early return in its own mousemove handler [1]. The .searchbar-treebody selector [2] can instead bind to autocomplete-treebody and we can have a new attribute that prevents the mousemove from causing a selection on mousemove.

[0]: https://dxr.mozilla.org/mozilla-central/rev/cd7217cf05a2332a8fd7b498767a07b2c31ea657/browser/components/search/content/search.xml#1157
[1]: https://dxr.mozilla.org/mozilla-central/rev/cd7217cf05a2332a8fd7b498767a07b2c31ea657/toolkit/content/widgets/autocomplete.xml#2523
[2]: https://dxr.mozilla.org/mozilla-central/rev/cd7217cf05a2332a8fd7b498767a07b2c31ea657/browser/components/search/content/searchbarBindings.css#12
(Reporter)

Updated

2 years ago
Whiteboard: [xbl-flatten-inheritance]
(Reporter)

Updated

2 years ago
Whiteboard: [xbl-flatten-inheritance] → [xbl-flatten-inheritance][xbl-available]
(Assignee)

Comment 1

2 years ago
The searchbar-treebody binding was removed and it's behavior moved to "noSelectRows" attribute on autocomplete-treebody binding. 
The binding was removed from .searchbar-treebody selector, because it has only one use, and that use already has a ".autocomplete-treebody".
Attachment #8927808 - Flags: review?(jaws)
Assignee: nobody → 86ecce74
Status: NEW → ASSIGNED
Attachment #8927808 - Flags: review?(jaws) → review?(adw)
Comment on attachment 8927808 [details] [diff] [review]
Removed searchbar-treebody binding and moved the behavior to autocomplete-treebody.

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

Thanks, this looks good.  I just have a few fairly unimportant comments.  Please post a new patch that addresses them and then request review from me again.

"noSelectRows" is a bit misleading.  To me, it sounds like it should make it impossible to select rows at all.  Could you please name it "noSelectOnMouseMove" or something like that (both the attribute and the property)?  "Rows" isn't important, "select" and "mousemove" are.

::: toolkit/content/widgets/autocomplete.xml
@@ +2507,5 @@
>      <implementation>
>        <field name="mLastMoveTime">Date.now()</field>
> +      <property name="noSelectRows">
> +        <getter><![CDATA[
> +          return this.getAttribute("noSelectRows");

Please return `this.getAttribute("noSelectRows") == true`.  It won't currently matter in practice, but I think we want to treat non-"true" values as false.  Seems like that's what we usually do.

@@ +2510,5 @@
> +        <getter><![CDATA[
> +          return this.getAttribute("noSelectRows");
> +        ]]></getter>
> +        <setter><![CDATA[
> +          this.setAttribute("noSelectRows", val);

Please do this instead:

if (val) {
  this.setAttribute("noSelectRows", "true");
} else {
  this.removeAttribute("noSelectRows");
}
return !!val;

@@ +2530,2 @@
>            // Allow bindings that extend this one to cancel the event so that
>            // nothing is selected.

Please update this comment.  It's not quite right anymore.
Attachment #8927808 - Flags: review?(adw)
(Assignee)

Comment 3

a year ago
Attachment #8927808 - Attachment is obsolete: true
Attachment #8928736 - Flags: review?(adw)
Comment on attachment 8928736 [details] [diff] [review]
Updated patch based on review.

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

These changes look good, thanks, but it looks like you lost the other parts of the patch.  Could you please post a combined patch that includes all changes, including those from your previous patch to search.xml and searchbarBindings.css?
Attachment #8928736 - Flags: review?(adw)
(Assignee)

Comment 5

a year ago
Attachment #8928736 - Attachment is obsolete: true
Attachment #8928894 - Flags: review?(adw)
Comment on attachment 8928894 [details] [diff] [review]
Combined changes

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

Thanks!  Let's run this by tryserver, and then we can add the "checkin-needed" keyword.
Attachment #8928894 - Flags: review?(adw) → review+
Tryserver results will appear here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22d0bd38997cd7424e28f1c5bfafaf975dd033af

Once we see that there are no problems, we can mark this bug checkin-needed.

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d71a752544e
Remove the "searchbar-treebody" binding and instead migrate the behavior to an attribute on "autocomplete-treebody". r=adw
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2d71a752544e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.