736886 - test_nsIAccessibleDocument.html silently dies

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

accessible/tests/mochitest/test_nsIAccessibleDocument.html seems to be running no tests, as indicated by <https://tbpl.mozilla.org/php/getParsedLog.php?id=10128146&tree=Try#error1>.  Serge says in bug 735805 comment 36:

> This a11y test is broken: getRootDirectory() is defined in
> chrome-harness.js, but including the latter just silently breaks somehow.
> Move this issue to a blocking bug.

For now I'll most likely have to add a todo(), because I can't figure out exactly how it's broken -- I have trouble getting it to work on localhost.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I got the test to work on localhost with Serge's help, and tracked down the basic problem -- including chrome-harness.js seems to break.  I commented it out for now, along with the check that relied on it, in a patch I'll attach to bug 735805.  I'm leaving this open as a followup to actually fix the test.  (Previously the test wasn't running any checks at all, but with the patch it's at least running some of them.)

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1)

Small change ... asking surkov for feedback ...

Comment 3

[Serge Gautherie (:sgautherie)]

Mark, do you have any idea about what goes wrong when using the chrome:// url in this test?

http://mxr.mozilla.org/mozilla-central/search?string=%2Fchrome-harness.js&case=1&find=%2Faccessible%2Ftests%2Fmochitest%2F

Comment 4

[Mark Capella [:capella]]

Well, I played with it until I got the correct syntax. The below substitution works also...

Your original line was... src="chrome://mochikit/content/chrome-harness.js"/>
You could change it to... src="chrome://mochikit/content/chrome-harness.js"></script>

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to Mark Capella [:capella] from comment #4)
> You could change it to...
> src="chrome://mochikit/content/chrome-harness.js"></script>

I would prefer that syntax over the '..' one in your patch ;-)

Yet, my question was really "is the breakage due to some code is that file?"...

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 624467 [details] [diff] [review]
Patch (v1)

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

thank you for fixing this!

::: accessible/tests/mochitest/test_nsIAccessibleDocument.html
@@ +18,2 @@
>    <script type="application/javascript"
> +    src="../../chrome-harness.js"></script>

nit: please align it properly

@@ +39,5 @@
>          attributes = docAcc.attributes;
>          is(attributes.getStringProperty("tag"), "body",
>             "Wrong attribute on document!");
>  
> +        // Test for correct root directory.

if you want comments for this then it'd be nice to have
// Document URL

and

// Document title and mime type.

@@ +50,4 @@
>             "Wrong title for document!");
>          is(docAcc.mimeType, "text/html",
>             "Wrong mime type for document!");
> +

nit: whitespace on empty line

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Serge Gautherie (:sgautherie) from comment #5)

> Yet, my question was really "is the breakage due to some code is that
> file?"...

it seems so, HTML is not friendly to <script> close tag.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I feel extremely stupid about this.  Feeding it into validator.nu would have showed up the problem immediately -- self-closing tags are not allowed in .html files and don't do anything.  <foo /> is exactly the same as <foo>.  So it was being parsed as a <script> with both src="" and contents, and the contents were therefore being ignored.

Thanks for spotting this!  This is a motivation for fixing bug 736517, which would have caught the problem -- although apparently that's not easy.

Comment 9

[Mark Capella [:capella]]

Attachment: Patch (final)

This is the final version.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #9)
> Created attachment 624706 [details] [diff] [review]
> Patch (final)
> 
> This is the final version.

cool, thank you!

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 624706 [details] [diff] [review]
Patch (final)

(In reply to Serge Gautherie (:sgautherie) from comment #3)
> http://mxr.mozilla.org/mozilla-central/search?string=%2Fchrome-harness.
> js&case=1&find=%2Faccessible%2Ftests%2Fmochitest%2F

Then: 1 XML with />, 2 HTML with </, and this broken HTML with />.


(In reply to Serge Gautherie (:sgautherie) from comment #5)
> (In reply to Mark Capella [:capella] from comment #4)
> > You could change it to...
> > src="chrome://mochikit/content/chrome-harness.js"></script>
> 
> I would prefer that syntax over the '..' one in your patch ;-)

Please, do use </script> as in the 2 other HTML files.

Comment 12

[Mark Capella [:capella]]

Serge, is there something you'd like to suggest we change in the final patch? I don't see your concern at this stage.

Comment 13

[Serge Gautherie (:sgautherie)]

I meant "use your comment 4 suggestion": no need to add a new/third (less good) syntax.

Comment 14

[Mark Capella [:capella]]

Without objections.

TRY push ... https://tbpl.mozilla.org/?tree=Try&rev=a77b0a028414

Comment 15

[Mark Capella [:capella]]

Inbound status:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a56ebbf0f293

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/a56ebbf0f293

Comment 17

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1337377564.1337381641.22500.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/18 14:46:04
{
16725 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html | Wrong URL for document! - chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html should equal chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html
16726 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html | Wrong title for document! - nsIAccessibleDocument chrome tests should equal nsIAccessibleDocument chrome tests
}

V.Fixed