extend the list of legitimate data table structures for layout-guess object attribute

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: surkov, Assigned: capella)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla13
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

5 years ago
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)
Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++]
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
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.
Attachment #600808 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 3

5 years ago
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 -->
Attachment #600808 - Flags: feedback?(surkov.alexander) → feedback+
(Assignee)

Comment 4

5 years ago
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?
(Reporter)

Comment 5

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

Comment 6

5 years ago
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).
(Reporter)

Comment 7

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

Comment 8

5 years ago
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 :-)
Attachment #600808 - Attachment is obsolete: true
Attachment #601367 - Attachment is obsolete: true
(Reporter)

Comment 9

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

Comment 10

5 years ago
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?
Attachment #601744 - Attachment is obsolete: true
(Reporter)

Comment 11

5 years ago
(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
(Reporter)

Comment 12

5 years ago
(In reply to Mark Capella [:capella] from comment #10)
> Created attachment 601928 [details] [diff] [review]
> Patch (v5)

is it ready for review?
(Assignee)

Comment 13

5 years ago
Created attachment 602017 [details] [diff] [review]
Patch (v6)

Let's try this one...
Attachment #601928 - Attachment is obsolete: true
Attachment #602017 - Flags: review?(surkov.alexander)
(Reporter)

Comment 14

5 years ago
Comment on attachment 602017 [details] [diff] [review]
Patch (v6)

let's redirect review request to hub, I'll do feedback after.
Attachment #602017 - Flags: review?(surkov.alexander) → review?(hub)
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?
(Reporter)

Comment 16

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

Comment 17

5 years ago
Created attachment 602550 [details] [diff] [review]
Patch (v7)

:-( Dang!

Ok, let's protect against that ...
(Assignee)

Comment 18

5 years ago
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
Attachment #602550 - Flags: feedback?(surkov.alexander)
(Reporter)

Updated

5 years ago
Attachment #602550 - Flags: review?(hub)
(Reporter)

Updated

5 years ago
Attachment #602017 - Attachment is obsolete: true
Attachment #602017 - Flags: review?(hub)

Updated

5 years ago
Attachment #602550 - Flags: review?(hub) → review+
(Reporter)

Comment 19

5 years ago
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.
Attachment #602550 - Flags: feedback?(surkov.alexander) → feedback-
(Assignee)

Comment 20

5 years ago
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 ...
Attachment #602550 - Attachment is obsolete: true
Attachment #603989 - Flags: review?(surkov.alexander)
(Reporter)

Comment 21

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

Comment 22

5 years ago
::: 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>
(Reporter)

Comment 23

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

Comment 24

5 years ago
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 :)
Attachment #603989 - Attachment is obsolete: true
Attachment #603989 - Flags: review?(surkov.alexander)
Attachment #604521 - Flags: review?(surkov.alexander)
(Reporter)

Comment 25

5 years ago
(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.
(Reporter)

Comment 26

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

Comment 27

5 years ago
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.
(Reporter)

Comment 28

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

Comment 29

5 years ago
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 ...
Attachment #604521 - Attachment is obsolete: true
Attachment #604774 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 30

5 years ago
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
Attachment #604774 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 31

5 years ago
Created attachment 604843 [details] [diff] [review]
Patch (v11)

Getting to an nsAccessible* from an nsIContent* was the final trick I was looking for :)
Attachment #604774 - Attachment is obsolete: true
Attachment #604843 - Flags: review?(surkov.alexander)
(Reporter)

Comment 32

5 years ago
Comment on attachment 604843 [details] [diff] [review]
Patch (v11)

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

r=me, thanks
Attachment #604843 - Flags: review?(surkov.alexander) → review+
(Reporter)

Comment 33

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b14faed003
https://hg.mozilla.org/mozilla-central/rev/b3b14faed003
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.