502154 - Mochitests for nsHTMLTableAccessible::IsProbablyForLayout

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Spun off bug 495388 to be able to use these tests elsewhere also.

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

Attachment: Correct patch with missing file

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 386681 [details] [diff] [review]
Correct patch with missing file


> 		test_table_indexes.html \
> 		test_table_indexes_ariagrid.html \
> 		test_table_sels.html \
> 		test_table_sels_ariagrid.html \
>+		test_table_layoutguess.html \

nit: keep them in alphabetical order

>+/**
>+ * Test object attributes that must not be present.
>+ *
>+ * @param aAccOrElmOrID         [in] the ID, DOM node or accessible
>+ * @param aAbsentAttrs          [in] map of attributes that should not be
>+ *                              present (name/value pairs)
>+ * @param aSkipUnexpectedAttrs  [in] points this function doesn't fail if
>+ *                              unexpected attribute is encountered

aSkipUnexpectedAttrs semantics isn't used for absent attributes and it doesn't make sense as far as I can see.

>   var enumerate = aAttrs.enumerate();
>   while (enumerate.hasMoreElements()) {
>     var prop = enumerate.getNext().QueryInterface(nsIPropertyElement);
> 
>     if (!(prop.key in aExpectedAttrs)) {
>       if (!aSkipUnexpectedAttrs)
>         ok(false, "Unexpected attribute '" + prop.key + "' having '" +
>            prop.value + "'" + aErrorMsg);
>+      if (aAbsentAttrs && prop.key in aAbsentAttrs)
>+        ok(false, "Attribute '" + prop.key + "' having '" +
>+           prop.value + "' must not be present" + aErrorMsg);

the minus of this approach we see an error only, it would be nice to see success as well.

Comment 4

[alexander :surkov (:asurkov)]

> + <table id="table17" width="980px">

isn't this a bit dangerous, it depends on firefox window size and screen resolution I guess?

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to comment #4)
> > + <table id="table17" width="980px">
> 
> isn't this a bit dangerous, it depends on firefox window size and screen
> resolution I guess?

Do you have a better idea? Our code tests for a certain percentage width, so we have to have something that comes close to whatever there is as the available width. Or can you give a percentage value to the width attribute?

Comment 6

[Marco Zehe (:MarcoZ)]

Attachment: Patch2

Addresses Surkov's comments.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 386688 [details] [diff] [review]
Patch2


>+ * Test object attributes that must not be present.
>+ *
>+ * @param aAccOrElmOrID         [in] the ID, DOM node or accessible

nit: accessible identifier please

>-function compareAttrs(aErrorMsg, aAttrs, aExpectedAttrs, aSkipUnexpectedAttrs)
>+function testAttrsInternal(aAccOrElmOrID, aAttrs, aSkipUnexpectedAttrs,
>+                   aAbsentAttrs)

nit: wrong identation

>     if (!(prop.key in aExpectedAttrs)) {
>       if (!aSkipUnexpectedAttrs)
>         ok(false, "Unexpected attribute '" + prop.key + "' having '" +
>            prop.value + "'" + aErrorMsg);

nit: add new line

>+      if (aAbsentAttrs)
>+        if (prop.key in aAbsentAttrs)
>+          ok(false, "Attribute '" + prop.key + "' having '" +
>+             prop.value + "' must not be present" + aErrorMsg);
>+        else
>+          ok(true, "Attribute '" + prop.key + "' having '" +
>+             prop.value + "' is not present" + aErrorMsg);

nope, it will report about success for every attribute we get.

logic should be

if we found absent attribute in attribute we get then we should fail. If we run through all attributes we get and didn't find any absent attribute then we should report about success for every absent attribute. Otherwise you could run through absent attributes and check if they are presented and report true or false.

Comment 8

[Marco Zehe (:MarcoZ)]

Attachment: Patch3

Is this better?

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 386694 [details] [diff] [review]
Patch3

>+ * @param aAccOrElmOrID         [in] the ID, DOM node or accessible identifier

 "accessible identifier", nothing more :) I mean "accessible identifier" is "the ID, DOM node or accessible". We just used this term already, so I asked you to have it here.

>+      if (value)
>+        ok(false,
>+           "There is an unexpected attribute '" + name + "' " + aErrorMsg);
>+      else
>+        ok(true,
>+           "There is no unexpected attribute '" + name + "' " + aErrorMsg);

I think ok(!value, ""); should be enough, though up to you if you really want to have two different messages.

r=me

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> Do you have a better idea? Our code tests for a certain percentage width, so we
> have to have something that comes close to whatever there is as the available
> width. Or can you give a percentage value to the width attribute?

percents are ok as you did.

Comment 11

[Marco Zehe (:MarcoZ)]

Pushed with comment addressed:
http://hg.mozilla.org/mozilla-central/rev/9493dd67dc31
I decided tokeep the separate messages.