Last Comment Bug 696975 - extend the list of legitimate data table structures for layout-guess object attribute
: extend the list of legitimate data table structures for layout-guess object a...
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: tablea11y
  Show dependency treegraph
 
Reported: 2011-10-24 17:48 PDT by alexander :surkov
Modified: 2012-03-13 04:47 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.69 KB, patch)
2012-02-26 12:43 PST, Mark Capella [:capella]
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch (v2) (4.37 KB, patch)
2012-02-28 12:25 PST, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (4.14 KB, patch)
2012-02-29 13:54 PST, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v5) (4.85 KB, patch)
2012-03-01 04:59 PST, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v6) (5.25 KB, patch)
2012-03-01 10:37 PST, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v7) (5.33 KB, patch)
2012-03-02 18:04 PST, Mark Capella [:capella]
hub: review+
surkov.alexander: feedback-
Details | Diff | Splinter Review
Patch (v8) (7.47 KB, patch)
2012-03-08 00:47 PST, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v9) (7.79 KB, patch)
2012-03-09 13:51 PST, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v10) (7.82 KB, patch)
2012-03-11 08:26 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v11) (7.24 KB, patch)
2012-03-12 00:03 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-10-24 17:48:34 PDT
James N comment:
6) Legitimate data cell structures should include the headers or scope attributes on any cell within the table. Also the abbr attribute on header cells is only really valid in data tables.
Comment 1 alexander :surkov 2012-02-16 01:50:56 PST
Put the logic under "Has th -- legitimate table structures" check, see http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHTMLTableAccessible.cpp#1438. You can reuse nsCoreUtils::IsHTMLTableHeader for th and scope check, for abbr check get an idea from nsHTMLTableCellAccessible::GetAttributesInternal.

add a test under table/test_layoutguess.html (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/table/test_layoutguess.html?force=1)
Comment 2 Mark Capella [:capella] 2012-02-26 12:43:43 PST
Created attachment 600808 [details] [diff] [review]
Patch (v1)

Attempt at the first part of the request ... the new test for table 4.5 fails before my change, passes after. Let me know if this is on/off track ... I'm looking at the ABBR stuff next.
Comment 3 alexander :surkov 2012-02-26 17:02:35 PST
Comment on attachment 600808 [details] [diff] [review]
Patch (v1)

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

this looks correct

::: accessible/tests/mochitest/table/test_layoutguess.html
@@ +225,5 @@
>      </tr>
>    </table>
>  
> +  <!-- table with th element       -->
> +  <!-- s/b DATA TABLE              -->

maybe shorter:
<!-- table with th element: data table -->
Comment 4 Mark Capella [:capella] 2012-02-28 07:53:55 PST
Ok, I'm stuck on the ABBR portion of the request here.

   Re: Also the ABBR attribute on header cells is only really valid in data tables.

Adding an ABBR through use of the <th> always forces the code to assume a DATA table, which seems to be the spec that we wish to follow. Are we saying there is a bug such that a LAYOUT table can be defined with an ABBR attribute?

Am I missing something really obvious?
Comment 5 alexander :surkov 2012-02-28 08:55:50 PST
(In reply to Mark Capella [:capella] from comment #4)
> Ok, I'm stuck on the ABBR portion of the request here.
> 
>    Re: Also the ABBR attribute on header cells is only really valid in data
> tables.
> 
> Adding an ABBR through use of the <th> always forces the code to assume a
> DATA table,

put it under <td>

> Are we saying
> there is a bug such that a LAYOUT table can be defined with an ABBR
> attribute?

nah, opposite: layout table can't contain abbr stuff

> Am I missing something really obvious?
Comment 6 Mark Capella [:capella] 2012-02-28 12:25:28 PST
Created attachment 601367 [details] [diff] [review]
Patch (v2)

Hmmm ... here we have a couple new testcases, mostly for comparison.

The interesting one is Table 4.4.1 that fails before this new patch (seems to be a layout table), passes after the patch (seems to be a data table).
Comment 7 alexander :surkov 2012-02-28 17:15:45 PST
Comment on attachment 601367 [details] [diff] [review]
Patch (v2)

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

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1442,5 @@
>            for (nsIContent* cellElm = rowElm->GetFirstChild(); cellElm;
>                 cellElm = cellElm->GetNextSibling()) {
> +            if (cellElm->IsHTML() && 
> +                 (nsCoreUtils::IsHTMLTableHeader(cellElm) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::abbr))) {

you don't check whether cell contains abbr element (see nsHTMLTableCellAccessible::GetAttributesInternal to get a hint)

::: accessible/tests/mochitest/table/test_layoutguess.html
@@ +39,5 @@
>        testAttrs("table4.2", attr, true);
>  
>        // table with th element
> +      // test for layout-guess attr ABSENT, it's a DATA TABLE
> +      testAbsentAttrs("table4.3", attr);

it's dupe of "table5"

@@ +265,5 @@
> +  <table id="table4.5">
> +    <tr>
> +      <td scope="a">table4.5 cell</td>
> +    </tr>
> +  </table>

seems like a dupe of the block above
Comment 8 Mark Capella [:capella] 2012-02-29 13:54:20 PST
Created attachment 601744 [details] [diff] [review]
Patch (v3)

Re the above...

> +      // test for layout-guess attr ABSENT, it's a DATA TABLE
> +      testAbsentAttrs("table4.3", attr);
it's dupe of "table5"

Yes, over-kill on my part ... I've removed table4.3 testcase.

>> +  <table id="table4.5">
>> +    <tr>
>> +      <td scope="a">table4.5 cell</td>
>> +    </tr>
>> +  </table>
> seems like a dupe of the block above

Table 4.5 is my testcase of the "Legitimate data cell structures should include the headers or scope attributes on any cell within the table" fix. We now check nsCoreUtils::IsHTMLTableHeader(cellElm) during nsHTMLTableAccessible::IsProbablyForLayout. What is this a dupe of?

> you don't check whether cell contains abbr element (see 
> nsHTMLTableCellAccessible::GetAttributesInternal to get a hint)

Table 4.4.1 is my testcase of the "abbr attribute on header cells is only really valid in data tables" fix. nsHTMLTableCellAccessible::GetAttributesInternal is where I found the logic that populates the nsGkAtoms::abbr information... isn't it enough to check cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::abbr) during nsHTMLTableAccessible::IsProbablyForLayout?

Thanks for your patience by the way ...  while you explain this stuff to me I'm reading through the nsHTML TableHeaderCell/ TableCell/ TableAccessible/ AccessibleWrap/ nsIaccessible and etc. classes ... looking for the bigger picture :-)
Comment 9 alexander :surkov 2012-02-29 18:18:10 PST
(In reply to Mark Capella [:capella] from comment #8)

> > seems like a dupe of the block above
> 
> Table 4.5 is my testcase of the "Legitimate data cell structures should
> include the headers or scope attributes on any cell within the table" fix.
> We now check nsCoreUtils::IsHTMLTableHeader(cellElm) during
> nsHTMLTableAccessible::IsProbablyForLayout. What is this a dupe of?

I misread your patch

> > you don't check whether cell contains abbr element (see 
> > nsHTMLTableCellAccessible::GetAttributesInternal to get a hint)
> 
> Table 4.4.1 is my testcase of the "abbr attribute on header cells is only
> really valid in data tables" fix.
> nsHTMLTableCellAccessible::GetAttributesInternal is where I found the logic
> that populates the nsGkAtoms::abbr information... isn't it enough to check
> cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::abbr) during
> nsHTMLTableAccessible::IsProbablyForLayout?

I meant the case
<td><abbr>bla</abbr></td>
an element inside the cell (not attribute)

> Thanks for your patience by the way ...  while you explain this stuff to me
> I'm reading through the nsHTML TableHeaderCell/ TableCell/ TableAccessible/
> AccessibleWrap/ nsIaccessible and etc. classes ... looking for the bigger
> picture :-)

that's awesome
Comment 10 Mark Capella [:capella] 2012-03-01 04:59:08 PST
Created attachment 601928 [details] [diff] [review]
Patch (v5)

Table 4.5 is for headers/scope, Table 4.4.1 is now for abbr/attribute, Table 4.4.3 is now for an abbr/element inside the cell ... yes?
Comment 11 alexander :surkov 2012-03-01 05:10:31 PST
(In reply to Mark Capella [:capella] from comment #10)
> Created attachment 601928 [details] [diff] [review]
> Patch (v5)
> 
> Table 4.5 is for headers/scope, 

you should have two tables, one is for @headers, another one is for @scope

> Table 4.4.1 is now for abbr/attribute, Table
> 4.4.3 is now for an abbr/element inside the cell ... yes?

yes

btw, I'd numbered them as "5.number" after th element
Comment 12 alexander :surkov 2012-03-01 09:51:20 PST
(In reply to Mark Capella [:capella] from comment #10)
> Created attachment 601928 [details] [diff] [review]
> Patch (v5)

is it ready for review?
Comment 13 Mark Capella [:capella] 2012-03-01 10:37:19 PST
Created attachment 602017 [details] [diff] [review]
Patch (v6)

Let's try this one...
Comment 14 alexander :surkov 2012-03-01 18:54:13 PST
Comment on attachment 602017 [details] [diff] [review]
Patch (v6)

let's redirect review request to hub, I'll do feedback after.
Comment 15 Hubert Figuiere [:hub] 2012-03-02 06:38:46 PST
Comment on attachment 602017 [details] [diff] [review]
Patch (v6)

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

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1446,5 @@
> +              if (cellElm->NodeInfo()->Equals(nsGkAtoms::th) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::scope) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::headers) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::abbr) ||
> +                  innrElm->NodeInfo()->Equals(nsGkAtoms::abbr))

Are we guarranted that innrElm is never nsnull ? ie that cellElm->GetFirstChild() always return something valid?
Comment 16 alexander :surkov 2012-03-02 07:12:54 PST
(In reply to Hub Figuiere [:hub] from comment #15)

> Are we guarranted that innrElm is never nsnull ? ie that
> cellElm->GetFirstChild() always return something valid?

we aren't
Comment 17 Mark Capella [:capella] 2012-03-02 18:04:04 PST
Created attachment 602550 [details] [diff] [review]
Patch (v7)

:-( Dang!

Ok, let's protect against that ...
Comment 18 Mark Capella [:capella] 2012-03-06 20:50:56 PST
Comment on attachment 602550 [details] [diff] [review]
Patch (v7)

The previous patch ran successfully for this (targeted) case:

  <!-- table with td/abbr element: Data Table -->
  <table id="table4.4.3" border="1">
    <tr>
      <td>table4.4.3 cell1</td>
      <td><abbr>table4.4.3 cell2</abbr></td>
    </tr>
  </table>

But abended spectacularly when modified as such:

  <!-- table with td/abbr element: Data Table -->
  <table id="table4.4.3" border="1">
    <tr>
      <td>table4.4.3 cell1</td>
      <td><abbr></abbr></td>
    </tr>
  </table>

The new patch seems to perform properly ... please let me know if I've missed anything further -- mark
Comment 19 alexander :surkov 2012-03-07 00:24:00 PST
Comment on attachment 602550 [details] [diff] [review]
Patch (v7)

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

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1441,5 @@
>          if (rowElm->IsHTML() && rowElm->Tag() == nsGkAtoms::tr) {
>            for (nsIContent* cellElm = rowElm->GetFirstChild(); cellElm;
>                 cellElm = cellElm->GetNextSibling()) {
> +            if (cellElm->IsHTML()) {
> +              nsIContent* innrElm = cellElm->GetFirstChild();

I think that would fail for
<td>
  <abbr>abbr</abbr>
</td>

also I wouldn't get a first child early if I was going to test it in the end

@@ +1446,5 @@
> +              if (cellElm->NodeInfo()->Equals(nsGkAtoms::th) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::scope) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::headers) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::abbr) ||
> +                 (innrElm && innrElm->IsHTML() && innrElm->NodeInfo() && 

NodeInfo() is never null

@@ +1448,5 @@
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::headers) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::abbr) ||
> +                 (innrElm && innrElm->IsHTML() && innrElm->NodeInfo() && 
> +                  innrElm->NodeInfo()->Equals(nsGkAtoms::abbr)))
> +                  RETURN_LAYOUT_ANSWER(false, "Has th -- legitimate table structures");

update the comment

::: accessible/tests/mochitest/table/test_layoutguess.html
@@ +241,5 @@
> +  <table id="table4.4">
> +    <tr>
> +      <td>cell1</td>
> +    </tr>
> +  </table>

the alg doesn't stop here, I meant whether this is layout table depends on the code below.

I think it's covered by 'layout table with 1 column' test.

@@ +244,5 @@
> +    </tr>
> +  </table>
> +
> +  <!-- table with td/abbr attribute: Data Table -->
> +  <table id="table4.4.1" border="1">

why is it prefixed by 4 while 'th' test is prefixed by '5'

@@ +250,5 @@
> +      <td abbr="abbr4.4.1">table4.4.1 cell1</td>
> +    </tr>
> +  </table>
> +
> +  <!-- table with th/abbr attribute: Data Table -->

adding a test for th@abbr when th results in data table and not having tests for th@scope and others look strange. I'd say it's not necessary to have it.

@@ +263,5 @@
> +    <tr>
> +      <td>table4.4.3 cell1</td>
> +      <td><abbr>table4.4.3 cell2</abbr></td>
> +    </tr>
> +  </table>

please add failing example I pointed here.
Comment 20 Mark Capella [:capella] 2012-03-08 00:47:29 PST
Created attachment 603989 [details] [diff] [review]
Patch (v8)

Alex, I've addressed the comments in the above.

   I added the failing test as "table6.4" then modified the code to examine all inner elements, not just the first one to fix it ... I also updated a couple RETURN_LAYOUT_ANSWER comments, and restructured / renamed the testcases to more closely match the top/down processing order in the IsProbablyForLayout() routine ...

   The DIFF as attached now looks ugly, but (to me) the final processing code and the test code correspond and read better ...

   I've flagged you for review ... I hope this is appropriate instead of the feedback flag ...
Comment 21 alexander :surkov 2012-03-08 17:44:59 PST
Comment on attachment 603989 [details] [diff] [review]
Patch (v8)

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

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1432,2 @@
>          childElm->Tag() == nsGkAtoms::colgroup ||
> +        childElm->Tag() == nsGkAtoms::col)

what's the reason to change the order?

@@ +1443,5 @@
> +            if (cellElm->IsHTML()) {
> +              
> +              if (cellElm->NodeInfo()->Equals(nsGkAtoms::th))
> +                 RETURN_LAYOUT_ANSWER(false,
> +                                      "Has th -- legitimate table structures");

I think I'd prefer to wrap that single statement if by {} since its body is multiline

@@ +1449,5 @@
> +              if (cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::headers) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::scope) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::abbr))
> +                 RETURN_LAYOUT_ANSWER(false,
> +                                     "Has headers, scope, or abbr attribute -- legitimate table structures");

nice but {} please

@@ +1452,5 @@
> +                 RETURN_LAYOUT_ANSWER(false,
> +                                     "Has headers, scope, or abbr attribute -- legitimate table structures");
> +
> +              for (nsIContent* innrElm = cellElm->GetFirstChild(); innrElm;
> +                innrElm = innrElm->GetNextSibling()) {

wrong indentation

@@ +1456,5 @@
> +                innrElm = innrElm->GetNextSibling()) {
> +                if (innrElm->IsHTML() && innrElm->NodeInfo()->Equals(nsGkAtoms::abbr))
> +                    RETURN_LAYOUT_ANSWER(false,
> +                                         "Has abbr element -- legitimate table structures");
> +              }

that will do a trick for 
<td>
  This is a really long text (<abbr>tiarlt</abbr>) inside layout table 
</td>

::: accessible/tests/mochitest/table/test_layoutguess.html
@@ +237,5 @@
>      </thead>
>    </table>
>  
>    <!-- table with tfoot element -->
> +  <table id="table5.2">

5.1 is reserved for something? :)

@@ +295,5 @@
> +      <td><abbr>table6.3 cell2</abbr></td>
> +    </tr>
> +  </table>
> +
> +  <!-- table with abbr element -->

(having empty text nodes)
Comment 22 Mark Capella [:capella] 2012-03-08 18:20:36 PST
::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1432,2 @@
>          childElm->Tag() == nsGkAtoms::colgroup ||
> +        childElm->Tag() == nsGkAtoms::col)
what's the reason to change the order?

Cosmetic change to match the TESTLAYOUTGUESS.HTML ... I changed it back ...

@@ +1443,5 @@
> +            if (cellElm->IsHTML()) {
> +              
> +              if (cellElm->NodeInfo()->Equals(nsGkAtoms::th))
> +                 RETURN_LAYOUT_ANSWER(false,
> +                                      "Has th -- legitimate table structures");
I think I'd prefer to wrap that single statement if by {} since its body is
multiline

I wrapped it ...

@@ +1449,5 @@
> +              if (cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::headers) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::scope) ||
> +                  cellElm->HasAttr(kNameSpaceID_None, nsGkAtoms::abbr))
> +                 RETURN_LAYOUT_ANSWER(false,
> +                                     "Has headers, scope, or abbr attribute -- legitimate table structures");
nice but {} please

I wrapped it ...

@@ +1452,5 @@
> +                 RETURN_LAYOUT_ANSWER(false,
> +                                     "Has headers, scope, or abbr attribute -- legitimate table structures");
> +
> +              for (nsIContent* innrElm = cellElm->GetFirstChild(); innrElm;
> +                innrElm = innrElm->GetNextSibling()) {
wrong indentation

Quite right ... I repaired it ...

@@ +1456,5 @@
> +                innrElm = innrElm->GetNextSibling()) {
> +                if (innrElm->IsHTML() && innrElm->NodeInfo()->Equals(nsGkAtoms::abbr))
> +                    RETURN_LAYOUT_ANSWER(false,
> +                                         "Has abbr element -- legitimate table structures");
> +              }
that will do a trick for 
<td>
  This is a really long text (<abbr>tiarlt</abbr>) inside layout table 
</td>

Is this just an observation or do you require a change?

::: accessible/tests/mochitest/table/test_layoutguess.html
@@ +237,5 @@
>      </thead>
>    </table>
>  
>    <!-- table with tfoot element -->
> +  <table id="table5.2">
5.1 is reserved for something? 

Not reserved for anything, the original tests skipped a number and when I re-ordered them I followed the pattern. I can fix it from table5/table5.2/table5.3/and table5.4 to table5/table5.1/table5.2/ and table5.3 if you wish ... yes?

@@ +295,5 @@
> +      <td><abbr>table6.3 cell2</abbr></td>
> +    </tr>
> +  </table>
> +
> +  <!-- table with abbr element -->
(having empty text nodes)

I'm not sure what you mean there, I thought table6.4 (below) addressed the failing case you pointed to in comment#19. Is this something new?

  <!-- table with abbr element -->
  <table id="table6.4">
    <tr>
      <td>
        <abbr>abbr</abbr>
      </td>
    </tr>
  </table>
Comment 23 alexander :surkov 2012-03-08 22:22:01 PST
(In reply to Mark Capella [:capella] from comment #22)
> ::: accessible/src/html/nsHTMLTableAccessible.cpp
> @@ +1432,2 @@
> >          childElm->Tag() == nsGkAtoms::colgroup ||
> > +        childElm->Tag() == nsGkAtoms::col)
> what's the reason to change the order?
> 
> Cosmetic change to match the TESTLAYOUTGUESS.HTML ... I changed it back ...

thank you, sort of strange to change cpp to match tests :)

> > +                innrElm = innrElm->GetNextSibling()) {
> > +                if (innrElm->IsHTML() && innrElm->NodeInfo()->Equals(nsGkAtoms::abbr))
> > +                    RETURN_LAYOUT_ANSWER(false,
> > +                                         "Has abbr element -- legitimate table structures");
> > +              }
> that will do a trick for 
> <td>
>   This is a really long text (<abbr>tiarlt</abbr>) inside layout table 
> </td>
> 
> Is this just an observation or do you require a change?

you need to change that, you could copy logic from GetAttrbiutesInternal:

if (GetChildCount() == 1) {
    nsAccessible* abbr = FirstChild();
    if (abbr->IsAbbreviation()) {
}

if you get an accessible for table row.

> ::: accessible/tests/mochitest/table/test_layoutguess.html
> @@ +237,5 @@
> >      </thead>
> >    </table>
> >  
> >    <!-- table with tfoot element -->
> > +  <table id="table5.2">
> 5.1 is reserved for something? 
> 
> Not reserved for anything, the original tests skipped a number and when I
> re-ordered them I followed the pattern. I can fix it from
> table5/table5.2/table5.3/and table5.4 to table5/table5.1/table5.2/ and
> table5.3 if you wish ... yes?

that'd be nice

> > +  <!-- table with abbr element -->
> (having empty text nodes)
> 
> I'm not sure what you mean there, I thought table6.4 (below) addressed the
> failing case you pointed to in comment#19. Is this something new?

just fit the comment to show what case we test here
Comment 24 Mark Capella [:capella] 2012-03-09 13:51:22 PST
Created attachment 604521 [details] [diff] [review]
Patch (v9)

The attached is where I am currently.

The original bug description says "Legitimate data cell structures should include the headers or scope attributes on any cell within the table" and that seems covered by test for table6.2 and table 6.2.2.

Regarding ATTR attributes/elements ... the original bug description says "Also the abbr attribute on header cells is only really valid in data tables." and that seems covered by test for table6.2.3. 

Comment #5 says "layout table can't contain abbr stuff", comment #9 clarifies "I meant the case <td><abbr>bla</abbr></td> an element inside the cell (not attribute)" and that seems covered by tests for table6.3, table 6.4 and table 6.5.

Where I'm lost is in comment #23 where you point me to "GetAttrbiutesInternal" again ... I miss what this routine gives us that scanning the table and testing as attached doesn't currently address ...

Obviously you've worked with this a lot longer than I, and I don't mean to be dumb on purpose :)
Comment 25 alexander :surkov 2012-03-10 06:47:30 PST
(In reply to Mark Capella [:capella] from comment #24)

> Comment #5 says "layout table can't contain abbr stuff", comment #9
> clarifies "I meant the case <td><abbr>bla</abbr></td> an element inside the
> cell (not attribute)" and that seems covered by tests for table6.3, table
> 6.4 and table 6.5.

it seems you treat it wide. I think layout table can contain abbr element like
<td>this is a cell of layout table <abbr>LT</abbr></td>
But we are going to treat the table as data if abbr element inside td was used to provide an abbreviation for td element itself (not for elements inside td). We fall into this case when td contains nothing but abbr element (note: due to mark up it allowed to contain whitespace nodes).

> Where I'm lost is in comment #23 where you point me to
> "GetAttrbiutesInternal" again ... I miss what this routine gives us that
> scanning the table and testing as attached doesn't currently address ...

So you either follow comment #23 or run through td children, ignore whitespace nodes and look for abbr element. If you find anything else then abbr element wasn't used for td.

I think working on accessible tree (comment #23) is easier.
Comment 26 alexander :surkov 2012-03-10 06:51:15 PST
Comment on attachment 604521 [details] [diff] [review]
Patch (v9)

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

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1431,5 @@
>          childElm->Tag() == nsGkAtoms::colgroup ||
>          childElm->Tag() == nsGkAtoms::tfoot ||
>          childElm->Tag() == nsGkAtoms::thead) {
>        RETURN_LAYOUT_ANSWER(false,
> +                           "Has thead, tfoot, colgroup, or col elements -- legitimate table structures");

it doesn't make a big sense to change the comment

@@ +1458,5 @@
> +              for (nsIContent* innrElm = cellElm->GetFirstChild(); innrElm;
> +                 innrElm = innrElm->GetNextSibling()) {
> +                if (innrElm->IsHTML() && innrElm->NodeInfo()->Equals(nsGkAtoms::abbr)) {
> +                  RETURN_LAYOUT_ANSWER(false,
> +                                       "Has abbr element -- legitimate table structures");

please address this following comment #25.

::: accessible/tests/mochitest/table/test_layoutguess.html
@@ +66,5 @@
> +      // table with abbr element having empty text node
> +      testAbsentAttrs("table6.4", attr);
> +
> +      // table with abbr element and non-empty text node 
> +      testAbsentAttrs("table6.5", attr, true);

it doesn't pass, right? because of comment above
Comment 27 Mark Capella [:capella] 2012-03-10 07:01:24 PST
Ok, I think it's starting to sink in... scanning td children vs. accessible tree ... 

All three tests 6.3, 6.4 and 6.5 as currently coded pass. That is, the IsProbablyForLayout() routine determines them all to be data tables not layout tables.
Comment 28 alexander :surkov 2012-03-10 07:26:44 PST
(In reply to Mark Capella [:capella] from comment #27)
> Ok, I think it's starting to sink in... scanning td children vs. accessible
> tree ... 

right.

> All three tests 6.3, 6.4 and 6.5 as currently coded pass. That is, the
> IsProbablyForLayout() routine determines them all to be data tables not
> layout tables.

oh yes, because you test absent attributes. btw, testAbsentAttrs doesn't take 3 arguments.
Comment 29 Mark Capella [:capella] 2012-03-11 08:26:16 PDT
Created attachment 604774 [details] [diff] [review]
Patch (v10)

Here's the latest ... I hope this is what you had in mind, this works by scanning the accessible table, has nits addressed, and table6.5 is now considered by the code as a layout table ...
Comment 30 alexander :surkov 2012-03-11 18:29:49 PDT
Comment on attachment 604774 [details] [diff] [review]
Patch (v10)

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

otherwise is good

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1479,5 @@
> +          }
> +        }
> +      }
> +    }
> +  }

this sort of cleaner but makes us to traverse the table twice. I'd say to put the logic into the loop above

nsAccessible* cell = mDoc->GetAccessible(cellElm);
if (cell && cell->GetChildCount() == 1 && cell->FirstChild()->IsAbbreviation())
  RETURN_LAYOUT_ANSWER
Comment 31 Mark Capella [:capella] 2012-03-12 00:03:11 PDT
Created attachment 604843 [details] [diff] [review]
Patch (v11)

Getting to an nsAccessible* from an nsIContent* was the final trick I was looking for :)
Comment 32 alexander :surkov 2012-03-12 07:59:34 PDT
Comment on attachment 604843 [details] [diff] [review]
Patch (v11)

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

r=me, thanks
Comment 34 Marco Bonardo [::mak] 2012-03-13 04:47:21 PDT
https://hg.mozilla.org/mozilla-central/rev/b3b14faed003

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