[New Tab Page] Write keyboard navigation tests

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: andreshm)

Tracking

Trunk
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
We need a test to make sure we don't regress keyboard navigation behavior. The tests should make sure that hidden buttons and sites (when the grid is hidden or modified) are not focusable and that a disappearing button passes its focus onto the next button.
(Reporter)

Updated

5 years ago
Blocks: 455553
Target Milestone: --- → Firefox 12
Version: Trunk → 13 Branch
(Assignee)

Updated

5 years ago
Assignee: nobody → andres
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 765729
(Assignee)

Comment 1

5 years ago
Created attachment 636460 [details] [diff] [review]
Patch v1
Attachment #636460 - Flags: review?(ttaubert)
(Reporter)

Updated

5 years ago
Target Milestone: Firefox 12 → Firefox 16
Version: 13 Branch → Trunk
(Reporter)

Comment 2

5 years ago
Comment on attachment 636460 [details] [diff] [review]
Patch v1

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

Thank you, Andres. That looks like a good solution to me but needs some corrections. The test didn't run on my machine and/or yielded test failures. On Linux (and I think Windows, too) the counts are definitely 28 and 1. On Mac they *should* be different because Mac doesn't normally allow all elements to become focused, right? We should probably define these counts as constants and #ifdef them.

::: browser/base/content/test/newtab/Makefile.in
@@ +19,5 @@
>  	browser_newtab_private_browsing.js \
>  	browser_newtab_reset.js \
>  	browser_newtab_tabsync.js \
>  	browser_newtab_unpin.js \
> +	browser_newtab_focus.js \

Please move it after 'browser_newtab_drop_preview.js' so that they stay in alphabetic order.

::: browser/base/content/test/newtab/browser_newtab_focus.js
@@ +10,5 @@
> +  setPinnedLinks("");
> +  yield addNewTabPageTab();
> +
> +  // count the focus with the enabled page. 
> +  is(countFocus(), 20, "invalid focus count with enabled page.");

The count should be 28 here. The toggle button, plus 9 sites with a link a and two buttons = 9 * 3 + 1.

@@ +14,5 @@
> +  is(countFocus(), 20, "invalid focus count with enabled page.");
> +
> +  // disable page and count the focus with the disabled page.
> +  NewTabUtils.allPages.enabled = false;
> +  is(countFocus(), 2, "invalid focus count with disabled page.");

The count should be "1" here. It's only the one toggle button.

@@ +24,5 @@
> +function countFocus() {
> +  let urlbarFound = false;
> +  let focusCount = 0;
> +
> +  gURLBar.focus();

I think it's enough to just execute this after 'yield addNewTabPage()' once and from then the focus will cycle.

@@ +27,5 @@
> +
> +  gURLBar.focus();
> +  window.addEventListener("focus", function() {
> +    let focusedElement = document.commandDispatcher.focusedElement;
> +    //info("\n    " + focusedElement.getAttribute("class"));

Please remove that debug code.

@@ +28,5 @@
> +  gURLBar.focus();
> +  window.addEventListener("focus", function() {
> +    let focusedElement = document.commandDispatcher.focusedElement;
> +    //info("\n    " + focusedElement.getAttribute("class"));
> +    if (-1 != focusedElement.getAttribute("class").indexOf("urlbar")) {

if (focusedElement && focusedElement.classList.contains("urlbar-input")) {

@@ +29,5 @@
> +  window.addEventListener("focus", function() {
> +    let focusedElement = document.commandDispatcher.focusedElement;
> +    //info("\n    " + focusedElement.getAttribute("class"));
> +    if (-1 != focusedElement.getAttribute("class").indexOf("urlbar")) {
> +      urlbarFound = true;

If you pass a focus count argument to countFocus() then you could just call it like

> yield countFocus(20);

and then call TestRunner.next() here.

@@ +30,5 @@
> +    let focusedElement = document.commandDispatcher.focusedElement;
> +    //info("\n    " + focusedElement.getAttribute("class"));
> +    if (-1 != focusedElement.getAttribute("class").indexOf("urlbar")) {
> +      urlbarFound = true;
> +    } else {

} else if (focusedElement && focusedElement.ownerDocument == getContentDocument() && focusedElement instanceof HTMLElement) {

(We should put getContentDocument() in a variable. I think we should check if the the focused element is an HTML element and contained in about:newtab's document.)

@@ +36,5 @@
> +    }
> +  }, true);
> +
> +  while (!urlbarFound) {
> +    document.commandDispatcher.advanceFocus();

Please call this in the focus event handler.
Attachment #636460 - Flags: review?(ttaubert)
(Assignee)

Comment 3

5 years ago
Created attachment 636863 [details] [diff] [review]
Patch v2

Updated patch.
Attachment #636460 - Attachment is obsolete: true
Attachment #636863 - Flags: review?(ttaubert)
(Reporter)

Comment 4

5 years ago
Comment on attachment 636863 [details] [diff] [review]
Patch v2

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

Thank you, this looks good. Let's just revert the Makefile changes and fix a couple of nits.

::: browser/base/content/test/newtab/browser_newtab_focus.js
@@ +4,5 @@
> +#ifdef XP_MACOSX
> +const FOCUS_COUNT = 19;
> +#else
> +const FOCUS_COUNT = 28;
> +#endif

A bit hackier but without the need to introduce pre-processing just for this single test:

> if ("nsILocalFileMac" in Ci)
>   // we're on mac...

Please also explain the focus count numbers. Like...

28 = 9 * 3 + 1 = 9 sites and 1 toggle button, each site has a link and pin and remove buttons.
19 = Mac doesn't focus links, so 9 focus targets less than Windows/Linux.

@@ +17,5 @@
> +  yield addNewTabPageTab();
> +  gURLBar.focus();
> +
> +  // Count the focus with the enabled page.
> +  yield countFocus(FOCUS_COUNT);

Nit: A new line here would separate the two steps a bit.

@@ +28,5 @@
> + * Focus the urlbar and count how many focus stops to return again to the urlbar.
> + */
> +function countFocus(aExpectedCount) {
> +  var focusCount = 0;
> +  var contentDoc = getContentDocument();

'let' is the new 'var' :)
Attachment #636863 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 638863 [details] [diff] [review]
Patch v3

Applied changes.
Attachment #636863 - Attachment is obsolete: true
Attachment #638863 - Flags: review?(ttaubert)
(Reporter)

Comment 6

5 years ago
Comment on attachment 638863 [details] [diff] [review]
Patch v3

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

Awesome. Please push it to try and then we're good to go!
Attachment #638863 - Flags: review?(ttaubert) → review+
(Reporter)

Comment 7

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/a19d621cdc4f
Whiteboard: [fixed-in-fx-team]
(Reporter)

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a19d621cdc4f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.