Closed Bug 1182338 Opened 4 years ago Closed 4 years ago

Bring in-content search UI keyboard navigation up to parity with main searchbar UI

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

(Whiteboard: [ui][fxsearch])

Attachments

(2 files, 3 obsolete files)

Keyboard navigation in the new "flare" in-content search UI (bug 1171344) is currently basic (up/down) and should support the more advanced features in the main searchbar UI.
Attached patch WIP (obsolete) — Splinter Review
This patch implements all the keyboard nav behaviors of the main searchbox in the in-content UI.

I'm not looking for code review with this patch, but would be grateful if you could test it out and let me know if I'm missing something or if something is broken (I think it's working fine).

This patch needs comments explaining all the if's, and needs a patch with tests to go with it.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8636355 - Flags: feedback?(florian)
Attachment #8636355 - Flags: feedback?(adw)
Comment on attachment 8636355 [details] [diff] [review]
WIP

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

Works well for me, nice job.
Attachment #8636355 - Flags: feedback?(adw) → feedback+
FYI I'm seeing this error:

JavaScript error: chrome://browser/content/contentSearchUI.js, line 449: TypeError: button is undefined
Attached patch Patch (obsolete) — Splinter Review
Added comments and cleaned some stuff up. Should be good.

There are enough behaviors that I ended up with a lot of conditionals, making the code hard to understand.
I'm trying to decide whether a rewrite of the keypress handler is warranted, making every case separate and obvious to understand which case is being handled. What do you think?
Attachment #8636355 - Attachment is obsolete: true
Attachment #8636355 - Flags: feedback?(florian)
Attachment #8638273 - Flags: review?(adw)
Attached patch TestsSplinter Review
Tests! I think I covered everything.
Attachment #8638274 - Flags: review?(adw)
Forgot to include the link to my try push when I attached the tests patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cf43d895bce
Attached patch Patch v2 (obsolete) — Splinter Review
Rewrote some of the code to have less nested |if| conditionals. Should be much easier to read.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=614e5fbf20f4
Attachment #8638273 - Attachment is obsolete: true
Attachment #8638273 - Flags: review?(adw)
Attachment #8638840 - Flags: review?(adw)
Comment on attachment 8638840 [details] [diff] [review]
Patch v2

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

Looks good, just a few superficial comments.

::: browser/base/content/contentSearchUI.js
@@ +11,5 @@
>  const ONE_OFF_ID_PREFIX = "oneOff";
>  const CSS_URI = "chrome://browser/content/contentSearchUI.css";
>  
> +const accelKey =
> +#ifdef XP_MACOSX

It looks like you may be able to call nsIDOMKeyEvent.getModifierState("Accel") to tell whether the accel key is pressed.  See for example: http://mxr.mozilla.org/mozilla-central/source/dom/events/test/test_accel_virtual_modifier.html?force=1

If that works, it'd be better than an #ifdef, so could you try that please?

@@ +328,5 @@
> +        if (event.shiftKey) {
> +          selectedSuggestionDelta = 1;
> +          break;
> +        }
> +        this._cycleCurrentEngine();

Please pass false.

@@ +476,5 @@
>    },
>  
>    _onBlur: function () {
>      if (this._mousedown) {
> +      // At this point, this.input has losts focus, but a new element has not yet

losts -> lost
Attachment #8638840 - Flags: review?(adw) → review+
Thanks for the review.

(In reply to Drew Willcoxon :adw from comment #8)
> Comment on attachment 8638840 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8638840 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +476,5 @@
> >    },
> >  
> >    _onBlur: function () {
> >      if (this._mousedown) {
> > +      // At this point, this.input has losts focus, but a new element has not yet
> 
> losts -> lost

Wat. I don't know how that change got in there.
Comment on attachment 8638274 [details] [diff] [review]
Tests

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

Nice.
Attachment #8638274 - Flags: review?(adw) → review+
Attached patch Patch v3Splinter Review
getModifierState("Accel") WFM locally. Another try push to confirm on all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8229c94e9ada
Attachment #8638840 - Attachment is obsolete: true
Attachment #8639593 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/da180640c3f6
https://hg.mozilla.org/mozilla-central/rev/04144901baf6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
sorry had to back this out, seems after landing of this on m-c and the other trees bug 1186327 was failing more frequently as before.
Status: RESOLVED → REOPENED
Flags: needinfo?(nhnt11)
Resolution: FIXED → ---
Target Milestone: Firefox 42 → ---
Flags: needinfo?(nhnt11)
https://hg.mozilla.org/mozilla-central/rev/b5cd383beb2a
https://hg.mozilla.org/mozilla-central/rev/b7144dca1cac
https://hg.mozilla.org/mozilla-central/rev/ca054f13ad08
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.