Closed
Bug 719035
Opened 13 years ago
Closed 12 years ago
[New Tab Page] Write keyboard navigation tests
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 16
People
(Reporter: ttaubert, Assigned: andreshm)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → andres
Assignee | ||
Updated•13 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #636460 -
Flags: review?(ttaubert)
Reporter | ||
Updated•12 years ago
|
Target Milestone: Firefox 12 → Firefox 16
Version: 13 Branch → Trunk
Reporter | ||
Comment 2•12 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•12 years ago
|
||
Updated patch.
Attachment #636460 -
Attachment is obsolete: true
Attachment #636863 -
Flags: review?(ttaubert)
Reporter | ||
Comment 4•12 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•12 years ago
|
||
Applied changes.
Attachment #636863 -
Attachment is obsolete: true
Attachment #638863 -
Flags: review?(ttaubert)
Reporter | ||
Comment 6•12 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•12 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Reporter | ||
Comment 8•12 years ago
|
||
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.
Description
•