Last Comment Bug 685218 - expose layout-guess: true object attribute for datatable="0" attribute on HTML table
: expose layout-guess: true object attribute for datatable="0" attribute on HTM...
Status: RESOLVED FIXED
[good first bug]
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Oussama BADR
:
Mentors:
Depends on:
Blocks: tablea11y
  Show dependency treegraph
 
Reported: 2011-09-07 10:37 PDT by alexander :surkov
Modified: 2011-10-19 03:23 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
the patch file for fixing layout-guess bug.... (46.29 KB, text/plain)
2011-10-04 15:33 PDT, Oussama BADR
no flags Details
the patch file for fixing layout-guess bug.... (1.58 KB, patch)
2011-10-04 18:04 PDT, Oussama BADR
surkov.alexander: review-
Details | Diff | Splinter Review
the patch file for fixing layout-guess bug.... (2.60 KB, patch)
2011-10-09 15:54 PDT, Oussama BADR
no flags Details | Diff | Splinter Review
the patch file for fixing layout-guess bug... (21.10 KB, patch)
2011-10-11 10:44 PDT, Oussama BADR
surkov.alexander: review-
Details | Diff | Splinter Review
The patch file for fixing layout-guess bug... (8.56 KB, patch)
2011-10-12 17:08 PDT, Oussama BADR
surkov.alexander: review-
Details | Diff | Splinter Review
The patch file for fixing : bug 685218. (3.70 KB, patch)
2011-10-16 15:32 PDT, Oussama BADR
no flags Details | Diff | Splinter Review
The patch file for fixing : bug 685218. (3.76 KB, patch)
2011-10-16 15:44 PDT, Oussama BADR
surkov.alexander: review+
Details | Diff | Splinter Review
The patch file for fixing : bug 685218. (4.45 KB, patch)
2011-10-18 07:01 PDT, Oussama BADR
no flags Details | Diff | Splinter Review
The patch file for fixing : bug 685218. (4.44 KB, patch)
2011-10-18 07:07 PDT, Oussama BADR
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-09-07 10:37:12 PDT
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] 2011-09-23 19:19:39 PDT
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 3 Oussama BADR 2011-10-04 15:33:26 PDT
Created attachment 564687 [details]
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] 2011-10-04 15:36:30 PDT
(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) 2011-10-04 15:41:55 PDT
Comment on attachment 564687 [details]
the patch file for fixing layout-guess bug....

canceling r? per comment 4
Comment 6 Oussama BADR 2011-10-04 18:04:37 PDT
Created attachment 564725 [details] [diff] [review]
the patch file for fixing layout-guess bug....
Comment 7 Trevor Saunders (:tbsaunde) 2011-10-04 18:49:26 PDT
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 2011-10-04 19:04:46 PDT
(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 2011-10-04 19:07:45 PDT
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 2011-10-05 10:32:19 PDT
> 
> >+  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 2011-10-05 19:44:23 PDT
(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 2011-10-09 15:54:00 PDT
Created attachment 565840 [details] [diff] [review]
the patch file for fixing layout-guess bug....
Comment 13 Oussama BADR 2011-10-11 10:44:12 PDT
Created attachment 566260 [details] [diff] [review]
the patch file for fixing layout-guess bug...

I have run my Mochitest and it was OK.
Comment 14 alexander :surkov 2011-10-11 10:50:26 PDT
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 2011-10-12 17:08:49 PDT
Created attachment 566691 [details] [diff] [review]
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 2011-10-12 23:35:09 PDT
Comment on attachment 566691 [details] [diff] [review]
The patch file for fixing layout-guess bug...

># HG changeset patch
># Parent e136897709967e43e35ce4678db19a6204ee94ec
># User Ouss. BADR <oussamabadr@gmail.com>
>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 2011-10-14 08:48:49 PDT
(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 2011-10-14 08:53:42 PDT
(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 2011-10-16 15:32:22 PDT
Created attachment 567362 [details] [diff] [review]
The patch file for fixing : bug 685218.

The make and test operations was OK!
Comment 20 Oussama BADR 2011-10-16 15:44:17 PDT
Created attachment 567363 [details] [diff] [review]
The patch file for fixing : bug 685218.

the make and test operations were OK!
Comment 21 alexander :surkov 2011-10-17 02:22:15 PDT
Comment on attachment 567363 [details] [diff] [review]
The patch file for fixing : bug 685218.

># HG changeset patch
># Parent 349f3d4b2d877b99502b6aff9e826fe5250de028
># User Ouss. BADR <oussamabadr@gmail.com>
>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 2011-10-18 07:01:11 PDT
Created attachment 567734 [details] [diff] [review]
The patch file for fixing : bug 685218.

I think it will be OK for this patch!!
Comment 23 Oussama BADR 2011-10-18 07:07:48 PDT
Created attachment 567737 [details] [diff] [review]
The patch file for fixing : bug 685218.

...
Comment 24 alexander :surkov 2011-10-18 07:12:22 PDT
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] 2011-10-18 07:15:25 PDT
Thanks for the patch!
Comment 26 Oussama BADR 2011-10-18 07:41:11 PDT
thanks for your help too :)
Comment 27 alexander :surkov 2011-10-18 18:40:32 PDT
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5033def96eb6
Comment 28 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-19 03:23:18 PDT
https://hg.mozilla.org/mozilla-central/rev/5033def96eb6

Note You need to log in before you can comment on or make changes to this bug.