Mochitests for nsHTMLTableAccessible::IsProbablyForLayout

RESOLVED FIXED

Status

()

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

People

(Reporter: MarcoZ, Assigned: MarcoZ)

Tracking

(Blocks: 1 bug, {access})

Trunk
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Updated

9 years ago
Blocks: 417472
(Assignee)

Comment 1

9 years ago
Created attachment 386679 [details] [diff] [review]
patch
Assignee: nobody → marco.zehe
Status: NEW → ASSIGNED
Attachment #386679 - Flags: review?(surkov.alexander)
(Assignee)

Comment 2

9 years ago
Created attachment 386681 [details] [diff] [review]
Correct patch with missing file
Attachment #386679 - Attachment is obsolete: true
Attachment #386681 - Flags: review?(surkov.alexander)
Attachment #386679 - Flags: review?(surkov.alexander)

Comment 3

9 years ago
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

9 years ago
> + <table id="table17" width="980px">

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

Comment 5

9 years ago
(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?
(Assignee)

Comment 6

9 years ago
Created attachment 386688 [details] [diff] [review]
Patch2

Addresses Surkov's comments.
Attachment #386681 - Attachment is obsolete: true
Attachment #386688 - Flags: review?(surkov.alexander)
Attachment #386681 - Flags: review?(surkov.alexander)

Comment 7

9 years ago
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.
Attachment #386688 - Flags: review?(surkov.alexander)
(Assignee)

Comment 8

9 years ago
Created attachment 386694 [details] [diff] [review]
Patch3

Is this better?
Attachment #386688 - Attachment is obsolete: true
Attachment #386694 - Flags: review?(surkov.alexander)

Updated

9 years ago
Attachment #386694 - Flags: review?(surkov.alexander) → review+

Comment 9

9 years ago
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

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

Comment 11

9 years ago
Pushed with comment addressed:
http://hg.mozilla.org/mozilla-central/rev/9493dd67dc31
I decided tokeep the separate messages.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.