Closed Bug 1276565 Opened 3 years ago Closed 3 years ago

Clamp out-of-range tab selection shortcuts and indexes instead of ignoring them

Categories

(Firefox :: Keyboard Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

Details

Attachments

(1 file)

CMD + a number key is a keyboard shortcut to select that tab number. (CMD+9 will always select the right-most tab, regardless of the tab count.) Currently, if the number is greater than the tab count, then the keyboard shortcut is ignored. With this patch, the right-most tab will be selected instead. For example, if there are five tabs and the user accidentally enters CMD+6 instead of CMD+5, selecting tab #5 is probably what they wanted instead of ignoring the keyboard shortcut.

Also expand the tab selection tests to cover more edge cases.
Comment on attachment 8757769 [details] [diff] [review]
Clamp out-of-range tab selection shortcuts and indexes instead of ignoring them

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

This looks OK to me (nits below), but I'd like to have Dão rubberstamp this change as well. I'm assuming you also pushed to try? :-)

::: browser/base/content/tabbrowser.xml
@@ +2888,4 @@
>              aIndex += tabs.length;
> +            // clamp at index 0 if still negative.
> +            if (aIndex < 0)
> +                aIndex = 0;

Nit: 2-space indent, not 4 (here and below).

@@ +2890,5 @@
> +            if (aIndex < 0)
> +                aIndex = 0;
> +          } else {
> +            // clamp at right-most tab if out of range.
> +            if (aIndex >= tabs.length)

No lonely ifs please, so:

} else if (aIndex >= tabs.length) {
  // clamp at right-most tab if out of range.
  aIndex = tabs.length - 1;
}
Attachment #8757769 - Flags: review?(gijskruitbosch+bugs)
Attachment #8757769 - Flags: review?(dao+bmo)
Attachment #8757769 - Flags: review+
(In reply to :Gijs Kruitbosch from comment #2)
> This looks OK to me (nits below), but I'd like to have Dão rubberstamp this
> change as well. I'm assuming you also pushed to try? :-)

Yep. This final Try run is not the greenest, but none of the test failures are related to my change:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6f9613c190f3a07ea3d15a5e0676809dd1e2373

> > +            if (aIndex >= tabs.length)
> 
> No lonely ifs please, so:

Sounds good to me. I was just emulating the existing coding style in this file.
FWIW, I shared this keyboard shortcut proposal with Firefox UX design lead Philipp Sackl a couple weeks ago. He said, "that sounds pretty sensible to me! Please feel free to submit a patch!" :)
Attachment #8757769 - Flags: review?(dao+bmo) → review+
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2842eb7d2fb0
Clamp out-of-range tab selection shortcuts and indexes instead of ignoring them. r=Gijs r=dao
https://hg.mozilla.org/mozilla-central/rev/2842eb7d2fb0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.