Last Comment Bug 719035 - [New Tab Page] Write keyboard navigation tests
: [New Tab Page] Write keyboard navigation tests
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Andres Hernandez [:andreshm]
:
Mentors:
Depends on: 765729
Blocks: 455553
  Show dependency treegraph
 
Reported: 2012-01-18 07:30 PST by Tim Taubert [:ttaubert]
Modified: 2012-07-04 14:29 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (2.53 KB, patch)
2012-06-25 13:06 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Review
Patch v2 (3.56 KB, patch)
2012-06-26 13:46 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback+
Details | Diff | Review
Patch v3 (3.00 KB, patch)
2012-07-03 14:18 PDT, Andres Hernandez [:andreshm]
ttaubert: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2012-01-18 07:30:40 PST
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.
Comment 1 Andres Hernandez [:andreshm] 2012-06-25 13:06:17 PDT
Created attachment 636460 [details] [diff] [review]
Patch v1
Comment 2 Tim Taubert [:ttaubert] 2012-06-26 05:56:03 PDT
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.
Comment 3 Andres Hernandez [:andreshm] 2012-06-26 13:46:16 PDT
Created attachment 636863 [details] [diff] [review]
Patch v2

Updated patch.
Comment 4 Tim Taubert [:ttaubert] 2012-07-03 02:44:39 PDT
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' :)
Comment 5 Andres Hernandez [:andreshm] 2012-07-03 14:18:26 PDT
Created attachment 638863 [details] [diff] [review]
Patch v3

Applied changes.
Comment 6 Tim Taubert [:ttaubert] 2012-07-03 14:25:45 PDT
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!
Comment 7 Tim Taubert [:ttaubert] 2012-07-04 08:46:42 PDT
https://hg.mozilla.org/integration/fx-team/rev/a19d621cdc4f
Comment 8 Tim Taubert [:ttaubert] 2012-07-04 14:29:34 PDT
https://hg.mozilla.org/mozilla-central/rev/a19d621cdc4f

Note You need to log in before you can comment on or make changes to this bug.