Rewrite a11y tests that put <tabbrowser> in random XUL documents and expect it to work

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dao, Assigned: surkov)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
Alexander seemed to suggest that this might be "nearly trivial (see events/test_focus_browserui.xul as example)".
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.
(Assignee)

Comment 2

5 years ago
(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.
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.
(Assignee)

Comment 4

5 years ago
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?
Attachment #590495 - Flags: review?(marco.zehe)
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.
Attachment #590495 - Flags: review?(marco.zehe) → review+
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?
Attachment #590495 - Flags: feedback?(dao)
(Reporter)

Updated

5 years ago
Attachment #590495 - Flags: feedback?(dao) → feedback+
https://hg.mozilla.org/mozilla-central/rev/afb95cbc720f
Assignee: nobody → surkov.alexander
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Comment 8

5 years ago
reopen until other tests are fixed
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

5 years ago
Marco, just checking, is it on your plate?
Created attachment 593007 [details] [diff] [review]
Rewrite and re-enable events/test_scroll.xul.
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?
Attachment #593007 - Attachment is patch: true
Attachment #593007 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 12

5 years ago
Comment on attachment 593007 [details] [diff] [review]
Rewrite and re-enable events/test_scroll.xul.

this sounds right
Attachment #593007 - Flags: feedback?(surkov.alexander) → feedback+
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.
Attachment #593007 - Attachment is obsolete: true
Attachment #593026 - Flags: feedback?(surkov.alexander)
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.
Attachment #593031 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
Created attachment 593073 [details] [diff] [review]
events/test_scroll.xul
[Checked in: Comment 19]
Attachment #593026 - Attachment is obsolete: true
Attachment #593031 - Attachment is obsolete: true
Attachment #593026 - Flags: feedback?(surkov.alexander)
Attachment #593031 - Flags: feedback?(surkov.alexander)
Attachment #593073 - Flags: review?(marco.zehe)
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!
Attachment #593073 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 18

5 years ago
inbound land with Marco's addressed comment http://hg.mozilla.org/integration/mozilla-inbound/rev/a5617ff71043

please do not mark it fixed
Whiteboard: [inbound - do not resolve (comment 18)]
Target Milestone: mozilla12 → ---
https://hg.mozilla.org/mozilla-central/rev/a5617ff71043
(Assignee)

Comment 20

5 years ago
Marco, your call for other tests? ;)
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.
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
Created attachment 594079 [details] [diff] [review]
name/test_nsRootAcc.xul
[Checked in: Comment 30]
Attachment #594079 - Flags: review?(marco.zehe)
(Assignee)

Comment 24

5 years ago
(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 on attachment 594079 [details] [diff] [review]
name/test_nsRootAcc.xul
[Checked in: Comment 30]

r=me.
Attachment #594079 - Flags: review?(marco.zehe) → review+
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?
Attachment #594096 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 27

5 years ago
name/test_nsRootAcc.xul inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/4aeab9b424c1
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.
Attachment #594096 - Attachment is obsolete: true
Attachment #594096 - Flags: feedback?(surkov.alexander)
Attachment #594194 - Flags: review?(surkov.alexander)
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.
Attachment #594194 - Attachment is obsolete: true
Attachment #594194 - Flags: review?(surkov.alexander)
Attachment #594209 - Flags: review?(surkov.alexander)
https://hg.mozilla.org/mozilla-central/rev/4aeab9b424c1
(Assignee)

Comment 31

5 years ago
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
Attachment #594209 - Flags: review?(surkov.alexander) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [inbound - do not resolve (comment 18)]
Attachment #590495 - Attachment description: events/test_docload.xul → events/test_docload.xul [Checked in: Comment 7]
Attachment #593073 - Attachment description: events/test_scroll.xul → events/test_scroll.xul [Checked in: Comment 19]
Attachment #594079 - Attachment description: name/test_nsRootAcc.xul → name/test_nsRootAcc.xul [Checked in: Comment 30]
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.
Blocks: 586818
Blocks: 542726
Blocks: 558589
https://hg.mozilla.org/mozilla-central/rev/1faacf41fd5c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Attachment #594209 - Attachment description: rewrite tree and relations test_tabbrowser.xul files → rewrite tree and relations test_tabbrowser.xul files [Checked in: Comment 33]
Blocks: 725647
Blocks: 729474
You need to log in before you can comment on or make changes to this bug.