Last Comment Bug 719754 - Rewrite a11y tests that put <tabbrowser> in random XUL documents and expect it to work
: Rewrite a11y tests that put <tabbrowser> in random XUL documents and expect i...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: alexander :surkov
:
Mentors:
Depends on: 715857
Blocks: a11ytestdev 558589 542726 586818 725647 729474
  Show dependency treegraph
 
Reported: 2012-01-20 04:36 PST by Dão Gottwald [:dao]
Modified: 2012-02-22 04:17 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
events/test_docload.xul [Checked in: Comment 7] (27.29 KB, patch)
2012-01-21 10:56 PST, alexander :surkov
mzehe: review+
dao+bmo: feedback+
Details | Diff | Splinter Review
Rewrite and re-enable events/test_scroll.xul. (6.57 KB, patch)
2012-01-31 00:22 PST, Marco Zehe (:MarcoZ)
surkov.alexander: feedback+
Details | Diff | Splinter Review
Rewrite and re-enable events/test_scroll.xul (7.53 KB, patch)
2012-01-31 02:01 PST, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
Different take on test-scroll.xul (6.79 KB, patch)
2012-01-31 02:34 PST, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
events/test_scroll.xul [Checked in: Comment 19] (10.07 KB, patch)
2012-01-31 06:16 PST, alexander :surkov
mzehe: review+
Details | Diff | Splinter Review
name/test_nsRootAcc.xul [Checked in: Comment 30] (9.35 KB, patch)
2012-02-03 00:11 PST, alexander :surkov
mzehe: review+
Details | Diff | Splinter Review
tree/test_tabbrowser.xul (13.71 KB, patch)
2012-02-03 02:01 PST, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
reenable tree/test_tabbrowser.xul (14.08 KB, patch)
2012-02-03 08:26 PST, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
rewrite tree and relations test_tabbrowser.xul files [Checked in: Comment 33] (20.70 KB, patch)
2012-02-03 09:08 PST, Marco Zehe (:MarcoZ)
surkov.alexander: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-01-20 04:36:39 PST
Alexander seemed to suggest that this might be "nearly trivial (see events/test_focus_browserui.xul as example)".
Comment 1 Marco Zehe (:MarcoZ) 2012-01-20 07:07:20 PST
Alex, can you take this one, since you seem to have an idea about how this can be fixed without introducing a new test harness?

I looked at what browser-chrome tests are and feel that that is not the right harness for us, so a fix within the current harness and without the explicit use of xul:tabbrowser should be preferable.
Comment 2 alexander :surkov 2012-01-20 09:01:34 PST
(In reply to Marco Zehe (:MarcoZ) from comment #1)
> Alex, can you take this one, since you seem to have an idea about how this
> can be fixed without introducing a new test harness?

at least I'll put description how to fix it.

> I looked at what browser-chrome tests are and feel that that is not the
> right harness for us, so a fix within the current harness and without the
> explicit use of xul:tabbrowser should be preferable.

I thought about events/test_focus_browserui.xul approach. That testcase opens a browser window and then we test it the way we want.
Comment 3 David Bolter [:davidb] 2012-01-20 11:37:59 PST
Alexander I agree, this approach should work for us. We need an assignee. If anyone starts on this, please assign yourself. If we miss the merge on this we'll need to seek approval upchannel.
Comment 4 alexander :surkov 2012-01-21 10:56:13 PST
Created attachment 590495 [details] [diff] [review]
events/test_docload.xul
[Checked in: Comment 7]

Add some browser testing helpers and enable events/test_docload.xul

Marco, can you handle other tests?
Comment 5 Marco Zehe (:MarcoZ) 2012-01-23 00:37:36 PST
Comment on attachment 590495 [details] [diff] [review]
events/test_docload.xul
[Checked in: Comment 7]

>+ * Load the browser with the given url and then invokes the given function.

Nit: In accordande with all other docs, put "invoke" here without the s.

>+/**
>+ * Return reload button.
>+ */
>+function reloadButton()
>+{
>+  //return browserWindow().document.getElementById("star-button");

Nit: Get rid of that last line, it's not needed in that function and doesn't relate to it at all.

>+# test_scroll.xul disabled for misusing <tabbrowser> (bug 715857)

Please file another follow-up bug to fix that one, too.

>+    gA11yEventDumpToConsole = true; // debug

You may want to disable this before landing!

r=me in principle, but if possible, I'd like to ask that Dao take a look to see that we use it in the right fashion now.
Comment 6 Marco Zehe (:MarcoZ) 2012-01-23 00:39:08 PST
Comment on attachment 590495 [details] [diff] [review]
events/test_docload.xul
[Checked in: Comment 7]

Dão, could you take a look over this, esp browser.js, to make sure we're using it in the right fashion now?
Comment 7 Matt Brubeck (:mbrubeck) 2012-01-27 09:16:00 PST
https://hg.mozilla.org/mozilla-central/rev/afb95cbc720f
Comment 8 alexander :surkov 2012-01-27 18:09:29 PST
reopen until other tests are fixed
Comment 9 alexander :surkov 2012-01-30 23:24:29 PST
Marco, just checking, is it on your plate?
Comment 10 Marco Zehe (:MarcoZ) 2012-01-31 00:22:35 PST
Created attachment 593007 [details] [diff] [review]
Rewrite and re-enable events/test_scroll.xul.
Comment 11 Marco Zehe (:MarcoZ) 2012-01-31 00:28:06 PST
Comment on attachment 593007 [details] [diff] [review]
Rewrite and re-enable events/test_scroll.xul.

This is untested, I would just like to know if I'm going in the right direction here. I'm especially uncertain about the part where I move the function to get the anchorJumpTarget into browser.js, add a parameter, and then pass that to the function later (I#m not sure whether this will actually work since it originally was just the function object). Alex, any comments?
Comment 12 alexander :surkov 2012-01-31 00:47:09 PST
Comment on attachment 593007 [details] [diff] [review]
Rewrite and re-enable events/test_scroll.xul.

this sounds right
Comment 13 Marco Zehe (:MarcoZ) 2012-01-31 02:01:41 PST
Created attachment 593026 [details] [diff] [review]
Rewrite and re-enable events/test_scroll.xul

This test file starts to work, but after a bit, simply terminates all tests. The last lines of the log file are:

3742 INFO TEST-START | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul
3743 INFO TEST-INFO | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | must wait for load
3744 INFO TEST-INFO | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | must wait for focus
3745 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | test with ID = 'load tab chrome://mochitests/content/a11y/accessible/events/scroll.html#link1 for [ 'tabbrowser@id="content" node', address: [object XULElement] ]' failed. No reorder event.

Alex, any idea why this one hangs after testing for the reorder event? Note that I get a hanging open browser window with the scroll.html file loaded that doesn't close, but also doesn't seem to do anything else. After I close it manually, the tests finish with the log file being written up to these above lines.
Comment 14 Marco Zehe (:MarcoZ) 2012-01-31 02:34:45 PST
Created attachment 593031 [details] [diff] [review]
Different take on test-scroll.xul

This patch, compared to the other one purposely not marked as obsolete:
1. Restores the getAnchorJump part to the XUL file, undoing any change to browser.js. I thereby avoided the fact that, when I am supposed to pass a function object, I have to pass parameters to it, which seems to be causing trouble.
2. I also reverted so that I pass the function object rather than the function's return value in the AdvanceFocusIntoTab part of this test.

This gives me a different failure, although still quite strange:

3742 INFO TEST-START | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul
3743 INFO TEST-INFO | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | must wait for load
3744 INFO TEST-INFO | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | must wait for focus
3745 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | test with ID = 'load tab chrome://mochitests/content/a11y/accessible/events/scroll.html#link1 for [ 'tabbrowser@id="content" node', address: [object XULElement] ]' failed. No reorder event.
3746 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | wrong state bits for ['document node', address: [object HTMLDocument], role: document, name: 'nsIAccessible actions testing for anchors', address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessNode)]]!
3747 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | Focussed ['document node', address: [object HTMLDocument], role: document, name: 'nsIAccessible actions testing for anchors', address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessNode)]] must be focusable!
3748 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | Read-only ['document node', address: [object HTMLDocument], role: document, name: 'nsIAccessible actions testing for anchors', address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessNode)]] cannot be editable!
3749 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | an unexpected uncaught JS exception reported through window.onerror - tabBrowser() is undefined at chrome://mochitests/content/a11y/accessible/browser.js:41
3750 INFO TEST-END | chrome://mochitests/content/a11y/accessible/events/test_scroll.xul | finished in 92303ms

Why does it think the tabBrowser function is not defined? Something else must terribly break JS here, since:
a) I still get the same hang.
b) I get a ton of failures afterwards in the same log file.
Comment 15 alexander :surkov 2012-01-31 04:33:22 PST
the problem here when the page is loaded then it gets focused so advanceFocusIntoTab doesn't do a trick.

What we should do here:
1) load the page (scrolling_start is fired)
2) move focus somewhere else
3) focus document (scrolling_start is fired)

That's funny but I don't see how this test tests bug 437607 and bug 519303. Maybe they were moved to other tests but it should be figured out.
Comment 16 alexander :surkov 2012-01-31 06:16:58 PST
Created attachment 593073 [details] [diff] [review]
events/test_scroll.xul
[Checked in: Comment 19]
Comment 17 Marco Zehe (:MarcoZ) 2012-01-31 06:36:49 PST
Comment on attachment 593073 [details] [diff] [review]
events/test_scroll.xul
[Checked in: Comment 19]

@@ -44,16 +44,25 @@
>           ID: "anchor1",
>           actionName: "jump",
>           actionIndex: 0,
>           events: CLICK_EVENTS,
>           eventSeq: [
>             new scrollingChecker(getAccessible("bottom1"))
>           ]
>         },
>+        { // jump again (fix for bug 437607)
>+          ID: "anchor1",
>+          actionName: "jump",
>+          actionIndex: 0,
>+          events: CLICK_EVENTS,
>+          eventSeq: [
>+            new scrollingChecker(getAccessible("bottom1"))
>+          ]
>+        },

Ah, so we don't care if anything else has the focus in the meantime, whenever an anchor links has its action performed, fire the scrolling start event. Nice!

>+    function swithToBackgroundTab()

Nit: +    function switchToBackgroundTab()
(and all instances where this occurs of course).

r=me, thanks for picking this one up!
Comment 18 alexander :surkov 2012-01-31 21:17:04 PST
inbound land with Marco's addressed comment http://hg.mozilla.org/integration/mozilla-inbound/rev/a5617ff71043

please do not mark it fixed
Comment 19 Ed Morley [:emorley] 2012-02-02 08:14:36 PST
https://hg.mozilla.org/mozilla-central/rev/a5617ff71043
Comment 20 alexander :surkov 2012-02-02 17:06:22 PST
Marco, your call for other tests? ;)
Comment 21 Marco Zehe (:MarcoZ) 2012-02-02 21:59:21 PST
I'm currently working on tree/test_tabbrowser.xul, but since this uses very old event triggering code, it's going a bit slow since I need to figure out what goes where when creating a new invoker.

Alex, If you could take a look at name/nsRootAcc_wnd.xul? I must admit I have no clue what to do about this file.

The only other file left is relations/test_tabbrowser.xul, but I haven't looked at it yet to see what's involved there.
Comment 22 alexander :surkov 2012-02-02 22:31:50 PST
(In reply to Marco Zehe (:MarcoZ) from comment #21)
> Alex, If you could take a look at name/nsRootAcc_wnd.xul? I must admit I
> have no clue what to do about this file.

Ok, I'll take this one.
Comment 23 alexander :surkov 2012-02-03 00:11:12 PST
Created attachment 594079 [details] [diff] [review]
name/test_nsRootAcc.xul
[Checked in: Comment 30]
Comment 24 alexander :surkov 2012-02-03 00:15:18 PST
(In reply to alexander :surkov from comment #23)
> Created attachment 594079 [details] [diff] [review]
> name/test_nsRootAcc.xul

fixes bug 586818 and bug 525175
Comment 25 Marco Zehe (:MarcoZ) 2012-02-03 01:29:46 PST
Comment on attachment 594079 [details] [diff] [review]
name/test_nsRootAcc.xul
[Checked in: Comment 30]

r=me.
Comment 26 Marco Zehe (:MarcoZ) 2012-02-03 02:01:25 PST
Created attachment 594096 [details] [diff] [review]
tree/test_tabbrowser.xul

This one simply starts and stops about 20 ms later. I've tried all kinds of things, but seem to be missing something. Do you see what might be the problem?
Comment 27 alexander :surkov 2012-02-03 03:17:50 PST
name/test_nsRootAcc.xul inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/4aeab9b424c1
Comment 28 Marco Zehe (:MarcoZ) 2012-02-03 08:26:31 PST
Created attachment 594194 [details] [diff] [review]
reenable tree/test_tabbrowser.xul

Decided to change from listening to the REORDER event to listening to the DocLoadComplete event on the second document. I found reorder to be not the most reliable form, got random failures either because the target node wasn't found, or the tree wasn't correct. With listening to this DocLoadComplete on the second document, I found no problems.
Comment 29 Marco Zehe (:MarcoZ) 2012-02-03 09:08:42 PST
Created attachment 594209 [details] [diff] [review]
rewrite tree and relations test_tabbrowser.xul files
[Checked in: Comment 33]

Applied the same techniques to relations/test_tabbrowser.xul as I did to tree/test_tabbrowser.xul, also with the same rationale for testing for docLoad instead of Reorder events. I also cleaned up tree/test_tabbrowser.xul a bit to have less changed chunks. This concludes the rewrite of the files that had the xul:tabbrowser element.
Comment 30 Marco Bonardo [::mak] 2012-02-04 02:23:55 PST
https://hg.mozilla.org/mozilla-central/rev/4aeab9b424c1
Comment 31 alexander :surkov 2012-02-05 21:48:59 PST
Comment on attachment 594209 [details] [diff] [review]
rewrite tree and relations test_tabbrowser.xul files
[Checked in: Comment 33]

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

r=me

::: accessible/tests/mochitest/relations/Makefile.in
@@ +46,3 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  # test_tabbrowser.xul disabled for misusing <tabbrowser> (bug 715857)

remove this line please
the same for tree/Makefile.in

::: accessible/tests/mochitest/relations/test_tabbrowser.xul
@@ +28,5 @@
> +    // Invoker
> +    function testTabRelations()
> +    {
> +      this.eventSeq = [
> +        new invokerChecker(EVENT_DOCUMENT_LOAD_COMPLETE, tabDocumentAt, 1)

it makes sense to listen docloadcomplete for every loaded document

::: accessible/tests/mochitest/tree/test_tabbrowser.xul
@@ +27,5 @@
> +    // invoker
> +    function testTabHierarchy()
> +    {
> +      this.eventSeq = [
> +        new invokerChecker(EVENT_DOCUMENT_LOAD_COMPLETE, tabDocumentAt, 1)

same
Comment 32 Marco Zehe (:MarcoZ) 2012-02-06 04:16:36 PST
Comment on attachment 594209 [details] [diff] [review]
rewrite tree and relations test_tabbrowser.xul files
[Checked in: Comment 33]

Landed on inbound with comments addressed: http://hg.mozilla.org/integration/mozilla-inbound/rev/1faacf41fd5c

Please mark this bug as fixed after this lands on mozilla-central, it completes the work on this bug.
Comment 33 Ed Morley [:emorley] 2012-02-07 12:20:03 PST
https://hg.mozilla.org/mozilla-central/rev/1faacf41fd5c

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