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

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: WIP

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.

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 8636355 [details] [diff] [review]
WIP

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

Works well for me, nice job.

Comment 3

[Drew Willcoxon :adw]

FYI I'm seeing this error:

JavaScript error: chrome://browser/content/contentSearchUI.js, line 449: TypeError: button is undefined

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

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?

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Tests

Tests! I think I covered everything.

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Forgot to include the link to my try push when I attached the tests patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cf43d895bce

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

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

Comment 8

[Drew Willcoxon :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

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

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 10

[Drew Willcoxon :adw]

Comment on attachment 8638274 [details] [diff] [review]
Tests

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

Nice.

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v3

getModifierState("Accel") WFM locally. Another try push to confirm on all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8229c94e9ada

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/da180640c3f6
https://hg.mozilla.org/integration/fx-team/rev/04144901baf6

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da180640c3f6
https://hg.mozilla.org/mozilla-central/rev/04144901baf6

Comment 14

[Carsten Book [:Tomcat]]

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.

Comment 15

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/abd39f27c668
https://hg.mozilla.org/mozilla-central/rev/305ba37a62f8

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b5cd383beb2a
https://hg.mozilla.org/integration/fx-team/rev/b7144dca1cac

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ca054f13ad08

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5cd383beb2a
https://hg.mozilla.org/mozilla-central/rev/b7144dca1cac
https://hg.mozilla.org/mozilla-central/rev/ca054f13ad08