685218 - expose layout-guess: true object attribute for datatable="0" attribute on HTML table

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

We should expose layout-guess: true object attribute for datatable="0" attribute on HTML table. This attribute is used for similar proposes in IE.

Comment 1

[Ed Morley [:emorley]]

Please can you add a MXR link to help out new contributors, otherwise it's not exactly good first bug (given today's IRC enquires on this bug), thanks! :-)

Comment 2

[alexander :surkov (:asurkov)]

nsHTMLTableAccessible::IsProbablyForLayout (http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHTMLTableAccessible.cpp#1344) should be fixed.

Mochitest should be added here http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/table/test_layoutguess.html?force=1

Comment 3

[Oussama BADR]

Attachment: the patch file for fixing layout-guess bug....

the new code, which it's eventually for fixing the bug, is in the block named "block:01", you can look for this word in the patch to view the changes

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Ouss. BADR from comment #3)

Thanks Oussama, can you create a patch instead?
https://developer.mozilla.org/en/creating_a_patch

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 564687 [details]
the patch file for fixing layout-guess bug....

canceling r? per comment 4

Comment 6

[Oussama BADR]

Attachment: the patch file for fixing layout-guess bug....

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 564725 [details] [diff] [review]
the patch file for fixing layout-guess bug....

I'm not sure I like the positioning of this test relative to the others, I think checking  ARIA role first might be a good  thing, but markup with both datatable=0 and  role="!presentation" seems pretty illformed to me.

>-
>+ 

please don't add whitespace to blank lines.

>+  //Check for datatable attribute

2 comments is a bit  exesive imho, also I'm not a big fan of comments following something like an if like below.

>+  PRInt32 datatableVar;

I'd probably use attrValue as the name, but Var on the end seems very silly to me.

>+  if ((mContent->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::datatable, datatableVar))){
>+     if (datatableVar <= 0){//Supposed that the negative value means 0

I think interpreting negative values as false is wrong.  I'd also consider using  IsAttrValue() here which would let  you get rid of the local variable. Or maybe FindAttrvalue() depending on what we want to do in the case datatable="true"

>+       RETURN_LAYOUT_ANSWER(PR_TRUE, "Has datatable<=0 attribute thus it's for layout");
>+     }

the braces for a single statement if have properties of the goblin (the goblin has got to go)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> >+  PRInt32 datatableVar;

btw, attribute values are strings. I'd say check "0" value and nothing else until you have strong reason because that's an empirical heuristic so we just borrow it as it is and nothing invent.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 564725 [details] [diff] [review]
the patch file for fixing layout-guess bug....

r- based on comments #7, #8.

Comment 10

[Oussama BADR]

> 
> >+  PRInt32 datatableVar;
> 
> I'd probably use attrValue as the name, but Var on the end seems very silly
> to me.

"Var" means variable, so the new developer will be ensured that it's a variable and not something else!!!  
....so I will name it 'datatableAttrValue', like that it will be clear that it's a value of datatable attribute, I think so ....!
Thks for your remark!!

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Ouss. BADR from comment #10)
> "Var" means variable, so the new developer will be ensured that it's a
> variable and not something else!!!  

"Var" postfix in variable name is not used in Mozilla code. Variable names are started in lowercase, while type names are started in uppercase. You should take a look at Mozilla coding style guide - https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide.

> ....so I will name it 'datatableAttrValue', like that it will be clear that
> it's a value of datatable attribute, I think so ....!

please use AttrValueIs instead as Trevor suggested

Comment 12

[Oussama BADR]

Attachment: the patch file for fixing layout-guess bug....

Comment 13

[Oussama BADR]

Attachment: the patch file for fixing layout-guess bug...

I have run my Mochitest and it was OK.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 566260 [details] [diff] [review]
the patch file for fixing layout-guess bug...

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

please file new patch when comments are addressed

::: accessible/src/base/nsAccessibilityAtomList.h
@@ +180,5 @@
>  ACCESSIBILITY_ATOM(_class, "class")
>  ACCESSIBILITY_ATOM(cycles, "cycles") // used for XUL cycler attribute
>  ACCESSIBILITY_ATOM(curpos, "curpos") // XUL
>  ACCESSIBILITY_ATOM(data, "data")
> +ACCESSIBILITY_ATOM(datatable, "datatable") //HTML table

update your sources to trunk, nsAccessibiityAtomList.h doesn't exist anymore. Use nsGkAtomList.h (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGkAtomList.h)

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1390,5 @@
> +
> +  //Check for datatable attribute
> +  nsAutoString AttrValueIs;
> +  if ((mContent->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::datatable, AttrValueIs))&&
> +     (AttrValueIs.EqualsLiteral("0"))) {

use AttrValueIs method instead GetAttr (http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContent.h#426)

::: accessible/tests/mochitest/table/test_layoutguess.html
@@ +6,5 @@
> +        href="chrome://mochikit/content/tests/SimpleTest/test.css" />
> +
> +  <script type="application/javascript"
> +          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +

it looks like you changed line breaks (presumptively from unix to windows style), don't do that)

Comment 15

[Oussama BADR]

Attachment: The patch file for fixing layout-guess bug...

This is my patch after I have addressed  the comments of Alexander.
The test was ok for table 22 (with datatbale="0").

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 566691 [details] [diff] [review]
The patch file for fixing layout-guess bug...

># HG changeset patch
># Parent e136897709967e43e35ce4678db19a6204ee94ec
># User Ouss. BADR <[e-mail]>
>Bug 685218 - Expose layout-guess attribute for non-datatables.
>
>diff --git a/accessible/src/base/nsAccessibilityAtomList.h b/accessible/src/base/nsAccessibilityAtomList.h
>--- a/accessible/src/base/nsAccessibilityAtomList.h
>+++ b/accessible/src/base/nsAccessibilityAtomList.h

there's no nsAccessibilityAtomList.h file
 
>+  //Check for datatable attribute  

nit: space after //, no spaces at the end of line

>+  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::datatable, NS_LITERAL_STRING("0"), eIgnoreCase)) {

use eCaseMatters since 0 is case independent and we are guaranteed there's no extra processing
nit: don't exceeds 80 characters, move arguments into next line and align them relative first argument offset

>+       RETURN_LAYOUT_ANSWER(PR_TRUE, "Has datatable = 0 attribute, it's for layout");

nit: use two spaces indentation relative 'if' indentation

>-      // table with two captions
>-      testAbsentAttrs("table4.3", attr);
>-

the patch contains changes that don't belong to the bug

>+      testAttrs("table21.6", attr, true);
>+	  
>+	  //datatable="0", layout table

nit: space after //

>+	  testAttrs("table22", attr, true);

nit: wrong indentation, don't use tabs

>+  <!-- layout table with datatable="0" -->
>+  <table id="table22" datatbale="0">
>+    <tr>
>+      <td>Cell1</td><td>Cell2</td><td>Cell3</td><td>Cell4</td>
>+    </tr>
>+  </table>

one row table is layout table, so presence of datatable="0" doesn't make a difference.


> </body>
> </html>
>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
>--- a/content/base/src/nsGkAtomList.h
>+++ b/content/base/src/nsGkAtomList.h
>@@ -258,16 +258,17 @@ GK_ATOM(crossorigin, "crossorigin")
> GK_ATOM(curpos, "curpos")
> GK_ATOM(current, "current")
> GK_ATOM(currentloop, "currentloop")
> GK_ATOM(cycler, "cycler")
> GK_ATOM(data, "data")
> GK_ATOM(datalist, "datalist")
> GK_ATOM(dataType, "data-type")
> GK_ATOM(dateTime, "date-time")
>+GK_ATOM(datatable, "datatable")

move it to accessibility section instead

Comment 17

[Oussama BADR]

(In reply to alexander surkov from comment #16)

> >diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
> >--- a/content/base/src/nsGkAtomList.h
> >+++ b/content/base/src/nsGkAtomList.h
> >@@ -258,16 +258,17 @@ GK_ATOM(crossorigin, "crossorigin")
> > GK_ATOM(curpos, "curpos")
> > GK_ATOM(current, "current")
> > GK_ATOM(currentloop, "currentloop")
> > GK_ATOM(cycler, "cycler")
> > GK_ATOM(data, "data")
> > GK_ATOM(datalist, "datalist")
> > GK_ATOM(dataType, "data-type")
> > GK_ATOM(dateTime, "date-time")
> >+GK_ATOM(datatable, "datatable")
> 
> move it to accessibility section instead

can you explain more? ...thanks!

Comment 18

[Oussama BADR]

(In reply to Ouss. BADR from comment #17)
> (In reply to alexander surkov from comment #16)
> 
> > >diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
> > >--- a/content/base/src/nsGkAtomList.h
> > >+++ b/content/base/src/nsGkAtomList.h
> > >@@ -258,16 +258,17 @@ GK_ATOM(crossorigin, "crossorigin")
> > > GK_ATOM(curpos, "curpos")
> > > GK_ATOM(current, "current")
> > > GK_ATOM(currentloop, "currentloop")
> > > GK_ATOM(cycler, "cycler")
> > > GK_ATOM(data, "data")
> > > GK_ATOM(datalist, "datalist")
> > > GK_ATOM(dataType, "data-type")
> > > GK_ATOM(dateTime, "date-time")
> > >+GK_ATOM(datatable, "datatable")
> > 
> > move it to accessibility section instead
> 
> can you explain more? ...thanks!

sorry.....I got it !!

Comment 19

[Oussama BADR]

Attachment: The patch file for fixing : bug 685218.

The make and test operations was OK!

Comment 20

[Oussama BADR]

Attachment: The patch file for fixing : bug 685218.

the make and test operations were OK!

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 567363 [details] [diff] [review]
The patch file for fixing : bug 685218.

># HG changeset patch
># Parent 349f3d4b2d877b99502b6aff9e826fe5250de028
># User Ouss. BADR <[e-mail]>
>Bug 685218 - Expose layout-guess attribute for non-datatables.
>
>diff --git a/accessible/src/html/nsHTMLTableAccessible.cpp b/accessible/src/html/nsHTMLTableAccessible.cpp
>--- a/accessible/src/html/nsHTMLTableAccessible.cpp
>+++ b/accessible/src/html/nsHTMLTableAccessible.cpp
>@@ -1382,16 +1382,22 @@ nsHTMLTableAccessible::IsProbablyForLayo
> 
>   if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::role)) {
>     // Role attribute is present, but overridden roles have already been dealt with.
>     // Only landmarks and other roles that don't override the role from native
>     // markup are left to deal with here.
>     RETURN_LAYOUT_ANSWER(false, "Has role attribute, weak role, and role is table");
>   }
> 
>+  //Check for if datatable attribute has "0" value

nit: space after //
also just "Check if " is enough I think

>+  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::datatable,
>+      NS_LITERAL_STRING("0"), eCaseMatters)) {

align it relative first argument, i.e. place it under kNameSpaceID_None argument.

>+      //testing for datatable ="0"
>+      //layout table with datatable="0"

nit: // layout table having datatable="0" attribute and containing data table structure (tfoot element)

>+      testAttrs("table22", attr, true);
>+      //data table table with  role="grid"  and datatable="0"
>+      testAbsentAttrs("table22.1", attr);

please move this test where other @role attribute tests are, for example, after "table2" test.

>+  <!-- layout/data table with datatable="0"-->
>+  <!-- layout table with datatable="0"-->

the second comment is enough

r=me with nits fixed (please attach new patch)

Comment 22

[Oussama BADR]

Attachment: The patch file for fixing : bug 685218.

I think it will be OK for this patch!!

Comment 23

[Oussama BADR]

Attachment: The patch file for fixing : bug 685218.

...

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 567737 [details] [diff] [review]
The patch file for fixing : bug 685218.

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

you don't need to attach new patch. I'll fix nits before landing. Thanks for working on it!

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1386,5 @@
>      // markup are left to deal with here.
>      RETURN_LAYOUT_ANSWER(false, "Has role attribute, weak role, and role is table");
>    }
>  
> +  //Check if datatable attribute has "0" value

again: space after // and dot in the end

@@ +1389,5 @@
>  
> +  //Check if datatable attribute has "0" value
> +  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::datatable,
> +                            NS_LITERAL_STRING("0"), eCaseMatters)) {
> +    RETURN_LAYOUT_ANSWER(PR_TRUE, "Has datatable = 0 attribute, it's for layout");

PR_TRUE -> true

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks for the patch!

Comment 26

[Oussama BADR]

thanks for your help too :)

Comment 27

[alexander :surkov (:asurkov)]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5033def96eb6

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/5033def96eb6