Closed
Bug 719754
Opened 13 years ago
Closed 13 years ago
Rewrite a11y tests that put <tabbrowser> in random XUL documents and expect it to work
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: dao, Assigned: surkov)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files, 5 obsolete files)
(deleted),
patch
|
MarcoZ
:
review+
dao
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
MarcoZ
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
MarcoZ
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
Alexander seemed to suggest that this might be "nearly trivial (see events/test_focus_browserui.xul as example)".
Comment 1•13 years ago
|
||
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•13 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.
Comment 3•13 years ago
|
||
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•13 years ago
|
||
Add some browser testing helpers and enable events/test_docload.xul
Marco, can you handle other tests?
Attachment #590495 -
Flags: review?(marco.zehe)
Comment 5•13 years ago
|
||
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 6•13 years ago
|
||
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•13 years ago
|
Attachment #590495 -
Flags: feedback?(dao) → feedback+
Comment 7•13 years ago
|
||
Assignee: nobody → surkov.alexander
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Assignee | ||
Comment 8•13 years ago
|
||
reopen until other tests are fixed
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•13 years ago
|
||
Marco, just checking, is it on your plate?
Comment 10•13 years ago
|
||
Comment 11•13 years ago
|
||
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•13 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+
Comment 13•13 years ago
|
||
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)
Comment 14•13 years ago
|
||
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•13 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•13 years ago
|
||
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 17•13 years ago
|
||
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•13 years ago
|
||
inbound land with Marco's addressed comment http://hg.mozilla.org/integration/mozilla-inbound/rev/a5617ff71043
please do not mark it fixed
Updated•13 years ago
|
Whiteboard: [inbound - do not resolve (comment 18)]
Target Milestone: mozilla12 → ---
Comment 19•13 years ago
|
||
Assignee | ||
Comment 20•13 years ago
|
||
Marco, your call for other tests? ;)
Comment 21•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #594079 -
Flags: review?(marco.zehe)
Assignee | ||
Comment 24•13 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 25•13 years ago
|
||
Comment on attachment 594079 [details] [diff] [review]
name/test_nsRootAcc.xul
[Checked in: Comment 30]
r=me.
Attachment #594079 -
Flags: review?(marco.zehe) → review+
Comment 26•13 years ago
|
||
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•13 years ago
|
||
name/test_nsRootAcc.xul inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/4aeab9b424c1
Comment 28•13 years ago
|
||
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)
Comment 29•13 years ago
|
||
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)
Comment 30•13 years ago
|
||
Assignee | ||
Comment 31•13 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•13 years ago
|
Whiteboard: [inbound - do not resolve (comment 18)]
Updated•13 years ago
|
Attachment #590495 -
Attachment description: events/test_docload.xul → events/test_docload.xul
[Checked in: Comment 7]
Updated•13 years ago
|
Attachment #593073 -
Attachment description: events/test_scroll.xul → events/test_scroll.xul
[Checked in: Comment 19]
Updated•13 years ago
|
Attachment #594079 -
Attachment description: name/test_nsRootAcc.xul → name/test_nsRootAcc.xul
[Checked in: Comment 30]
Comment 32•13 years ago
|
||
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•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Updated•13 years ago
|
Attachment #594209 -
Attachment description: rewrite tree and relations test_tabbrowser.xul files → rewrite tree and relations test_tabbrowser.xul files
[Checked in: Comment 33]
You need to log in
before you can comment on or make changes to this bug.
Description
•