Closed Bug 1414020 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: General, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: bgrins, Assigned: u605551)

References

Details

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

Attachments

(1 file, 2 obsolete files)

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
Whiteboard: [xbl-flatten-inheritance]
Whiteboard: [xbl-flatten-inheritance] → [xbl-flatten-inheritance][xbl-available]
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)
Attached patch Updated patch based on review. (obsolete) — Splinter Review
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)
Attached patch Combined changesSplinter Review
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.
Keywords: checkin-needed
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: