Closed Bug 719035 Opened 12 years ago Closed 12 years ago

[New Tab Page] Write keyboard navigation tests

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 16

People

(Reporter: ttaubert, Assigned: andreshm)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 455553
Target Milestone: --- → Firefox 12
Version: Trunk → 13 Branch
Assignee: nobody → andres
Status: NEW → ASSIGNED
Depends on: 765729
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #636460 - Flags: review?(ttaubert)
Target Milestone: Firefox 12 → Firefox 16
Version: 13 Branch → Trunk
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)
Attached patch Patch v2 (obsolete) — Splinter Review
Updated patch.
Attachment #636460 - Attachment is obsolete: true
Attachment #636863 - Flags: review?(ttaubert)
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+
Attached patch Patch v3Splinter Review
Applied changes.
Attachment #636863 - Attachment is obsolete: true
Attachment #638863 - Flags: review?(ttaubert)
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+
https://hg.mozilla.org/mozilla-central/rev/a19d621cdc4f
Status: ASSIGNED → RESOLVED
Closed: 12 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.