alexander :surkov (:asurkov) Assignee
	Description • 11 years ago

      No description provided.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 1 • 11 years ago

Comment on attachment 790265 [details] [diff] [review]
patch

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

First pass review done (I didn't apply and run it). This is valuable. You test a lot in test_HTMLSpec.html which I was squeamish about initially but I think it makes sense. It is probably good that you didn't add name tests?

One thing that came to mind while reading the various cases (obj's that encapsulate expected values), is that it might be neat to put the markup next to each case and insert it programmatically into the DOM. This would make the test file almost a document. It may make the test run slower though (because of DOM mutation). Maybe you could give each markup case a different var name and then before the test, concatenate them all and insert them all into the DOM document at once? (Just thinking aloud).

::: accessible/tests/mochitest/common.js
@@ +366,5 @@
>  
>    // Test accessible properties.
>    for (var prop in accTree) {
>      var msg = "Wrong value of property '" + prop + "' for " + prettyName(acc) + ".";
> +    if (prop == "actions") {

Since you added a lot of 'if' cases here it would be nicer to use a switch I think.

switch (prop)
case "actions":

etc.

@@ +419,5 @@
> +        testTextAttrs(acc, prevOffset, attrs, { }, prevOffset, charCount, true);
> +      }
> +
> +    } else if (prop.indexOf("todo_") == 0) {
> +      todo(false, "failing " + prop + " on " + prettyName(acc));

Neat.

::: accessible/tests/mochitest/elm/test_HTMLSpec.html
@@ +31,5 @@
> +    {
> +      //////////////////////////////////////////////////////////////////////////
> +      // HTML:a@href
> +
> +      var obj = { 

nitty nit: don't need extra space after '{'

@@ +56,5 @@
> +        children: [
> +          {
> +            role: ROLE_TEXT_LEAF,
> +            absentStates: STATE_LINKED,
> +            actions: null

Just curious why do you make 'actions: null' explicit for this case and leave it out of other cases?

@@ +764,5 @@
> +      obj = {
> +        role: ROLE_LABEL,
> +        todo_relations: {
> +          RELATION_LABEL_FOR: "label_input"
> +        }, 

nit: extra space after ','

	alexander :surkov (:asurkov) Assignee
	Comment 2 • 11 years ago

(In reply to David Bolter [:davidb] from comment #1)

> First pass review done (I didn't apply and run it). This is valuable. You
> test a lot in test_HTMLSpec.html which I was squeamish about initially but I
> think it makes sense. It is probably good that you didn't add name tests?

I think we will add them one day if it can be done in nice way

> One thing that came to mind while reading the various cases (obj's that
> encapsulate expected values), is that it might be neat to put the markup
> next to each case and insert it programmatically into the DOM. This would
> make the test file almost a document. It may make the test run slower though
> (because of DOM mutation). Maybe you could give each markup case a different
> var name and then before the test, concatenate them all and insert them all
> into the DOM document at once? (Just thinking aloud).

it's good idea to keep markup and test together, it's friendly to the eyes so we might do this, we get something like name testing

> ::: accessible/tests/mochitest/common.js

> Since you added a lot of 'if' cases here it would be nicer to use a switch I
> think.

ok

> ::: accessible/tests/mochitest/elm/test_HTMLSpec.html
> @@ +31,5 @@
> > +      // HTML:a@href
> > +
> > +      var obj = { 
> 
> nitty nit: don't need extra space after '{'

what big eyes :)

> @@ +56,5 @@
> > +        children: [
> > +          {
> > +            role: ROLE_TEXT_LEAF,
> > +            absentStates: STATE_LINKED,
> > +            actions: null
> 
> Just curious why do you make 'actions: null' explicit for this case and
> leave it out of other cases?

to make sure <a@href> and <a> don't have same accessible with same properties

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 3 • 11 years ago

OK.

Why do I get these failures:

 0:10.40 165 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/elm/test_HTMLSpec.html | Wrong value of property 'role' for ['embed@id="embed_plugin_windowless" node', address: [object HTMLEmbedElement], role: text container, address: [xpconnect wrapped nsIAccessible]].got 'text container', expected 'embedded object'
 0:10.40 166 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/elm/test_HTMLSpec.html | wrong state bits for ['embed@id="embed_plugin_windowless" node', address: [object HTMLEmbedElement], role: text container, address: [xpconnect wrapped nsIAccessible]]!got '0', expected 'unavailable'
 0:10.40 167 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/elm/test_HTMLSpec.html | Wrong value of property 'role' for ['embed@id="embed_plugin_windowed" node', address: [object HTMLEmbedElement], role: text container, address: [xpconnect wrapped nsIAccessible]].got 'text container', expected 'embedded object'
 0:10.40 412 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/elm/test_HTMLSpec.html | Wrong value of property 'role' for ['object@id="object_plugin_windowless" node', address: [object HTMLObjectElement], role: text container, address: [xpconnect wrapped nsIAccessible]].got 'text container', expected 'embedded object'
 0:10.40 413 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/elm/test_HTMLSpec.html | wrong state bits for ['object@id="object_plugin_windowless" node', address: [object HTMLObjectElement], role: text container, address: [xpconnect wrapped nsIAccessible]]!got '0', expected 'unavailable'
 0:10.40 415 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/elm/test_HTMLSpec.html | Wrong value of property 'role' for ['object@id="object_plugin_windowed" node', address: [object HTMLObjectElement], role: text container, address: [xpconnect wrapped nsIAccessible]].got 'text container', expected 'embedded object'

	alexander :surkov (:asurkov) Assignee
	Comment 4 • 11 years ago

which platform do you run at?

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 5 • 11 years ago

(In reply to alexander :surkov from comment #4)
> which platform do you run at?

Sorry I missed this in my bugmail (I filter on david*). OSX.

	alexander :surkov (:asurkov) Assignee
	Comment 6 • 11 years ago

here's updated version

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 7 • 11 years ago

Comment on attachment 792240 [details] [diff] [review]
patch2

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

r=me if tests pass everywhere :)

::: accessible/tests/mochitest/common.js
@@ +380,4 @@
>  
> +    case "attributes":
> +      testAttrs(acc, accTree[prop], true);
> +      break;

You know me... I'd put the {}'s but ok.

@@ +434,5 @@
> +        var attrs = accTree[prop][prevOffset];
> +        testTextAttrs(acc, prevOffset, attrs, { }, prevOffset, charCount, true);
> +      }
> +      break;
> +    } 

nit: extra space after }

	alexander :surkov (:asurkov) Assignee
	Comment 8 • 11 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/9f54f76085eb

	Ed Morley [:emorley]
	Comment 9 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/9f54f76085eb