AX presentation role should be inherited to its required owned elements.

AX presentation role should be inherited to its required owned elements.

This patch makes AX presentation role be inherited to its required owned
elements. The variable, m_cachedHasInheritedPresentationRole, is added
and used to test whether AXObject has inheritance for presentation or not.
It’s recalculated on computeHasInheritedPresentationRole().
In testing presentation inheritance, several conditions are needed.
It should not be focusable, not have an explicit role definition and
be a required owned element.

BUG=471626
R=dmazzoni

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193042

[je_julie(Not used), 2015-03-28 10:54:01]

je_julie.kim@samsung.com changed reviewers:
+ dmazzoni@chromium.org

[je_julie(Not used), 2015-03-28 10:54:36]

Hi Dominic,
This patch makes presentation role be inherited to its required owned elements.
There are several roles which have required owned elements.

list->listitem
menu->menuitemradio,menuitem,menuitemcheckbox,menuitemradio
listBox->listBoxoption
row-gridcell, columheader, rowheader
table-row

In case of table,
if ascendants have a presentation role, it is inherited through child and
descendants.
So, I added m_hasInheritedPresentationRole at AXLayoutObject.

I compared chromium with this patch to IE and FF and here is the result.
*(p):presentation role is defined.
---------------------------------------------------------------
path                       chromium  |    IE    |   FF
---------------------------------------------------------------
List(p)->ListItem          inherited  inherited   inherited
Table(p)->Row              inherited  inherited   inherited
Row(p)->Cell               inherited  inherited     not
Table(p)->Row->Cell        inherited     not      inherited
Menu(p)->MenuItem*         inherited     not      Not supported
ListBox(p)->ListBoxOption  inherited  inherited     not

PTAL.

[dmazzoni, 2015-03-29 07:46:22]

Great fix.

Please file a bug and add a bug number. It's nice to have a bug whenever it's an
intentional change to the behavior.

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/AXLayoutObject.cpp:573:
m_hasInheritedPresentationRole = true;
I don't want to compute this inside computeAccessibilityIsIgnored. Over time I'd
actually like to simplify computeAccessibilityIsIgnored if possible.

Instead, please add a separate computeHasInheritedPresentationRole function that
we can call from AXObject::updateCachedAttributeValuesIfNeeded.

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/AXLayoutObject.h (right):

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/AXLayoutObject.h:197: bool
isPresentationRole(AXObject*) const;
Implement this in AXNodeObject instead of AXLayoutObject.

In general, use AXNodeObject when the only information you need is the node -
like ARIA attributes. AXLayoutObject should be used when the implementation
might depend on the CSS style, on its laid-out position / size, etc.

This isn't perfect now. Originally everything was in AXLayoutObject (or
AXRenderObject, as it was called until recently). I added AXNodeObject and
slowly we've been moving things from AXLayoutObject to AXNodeObject where they
make sense.

If you have a <canvas> element, the nodes inside the canvas aren't drawn. They
will have AXNodeObjects but they won't have AXLayoutObjects.

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/AXListBoxOption.cpp (right):

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/AXListBoxOption.cpp:63: // Currently IE handles
presentation role correctly with OptionElement.
I'd leave out this comment on IE unless the only reason we're doing something is
for IE compatibility. If our implementation is correct according to the spec,
let's just talk about the spec.

[je_julie(Not used), 2015-03-31 16:14:28]

Hi Dominic,
I filed a new bug and added bug number to description.
With the updated patchset, I moved implementation code to AXNodObject as you
told.
PTAL.

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/AXLayoutObject.cpp:573:
m_hasInheritedPresentationRole = true;
On 2015/03/29 07:46:22, dmazzoni wrote:
> I don't want to compute this inside computeAccessibilityIsIgnored. Over time
I'd
> actually like to simplify computeAccessibilityIsIgnored if possible.
> 
> Instead, please add a separate computeHasInheritedPresentationRole function
that
> we can call from AXObject::updateCachedAttributeValuesIfNeeded.
I added computeHasInheritedPresentationRole.

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/AXLayoutObject.h (right):

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/AXLayoutObject.h:197: bool
isPresentationRole(AXObject*) const;
On 2015/03/29 07:46:22, dmazzoni wrote:
> Implement this in AXNodeObject instead of AXLayoutObject.
> 
> In general, use AXNodeObject when the only information you need is the node -
> like ARIA attributes. AXLayoutObject should be used when the implementation
> might depend on the CSS style, on its laid-out position / size, etc.
> 
> This isn't perfect now. Originally everything was in AXLayoutObject (or
> AXRenderObject, as it was called until recently). I added AXNodeObject and
> slowly we've been moving things from AXLayoutObject to AXNodeObject where they
> make sense.
> 
> If you have a <canvas> element, the nodes inside the canvas aren't drawn. They
> will have AXNodeObjects but they won't have AXLayoutObjects.

Thanks for suggestion.
I move it at AXNodeObject.

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/AXListBoxOption.cpp (right):

https://codereview.chromium.org/1039873002/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/AXListBoxOption.cpp:63: // Currently IE handles
presentation role correctly with OptionElement.
On 2015/03/29 07:46:22, dmazzoni wrote:
> I'd leave out this comment on IE unless the only reason we're doing something
is
> for IE compatibility. If our implementation is correct according to the spec,
> let's just talk about the spec.

I replaced the comment with the spec description.

[dmazzoni, 2015-03-31 16:35:24]

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXLayoutObject.cpp:566: if
(isPresentationRole(static_cast<const AXObject*>(this)))
Can you do this without the cast?

If you do need a cast it should probably be const_cast instead of static_cast

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXListBoxOption.cpp (right):

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXListBoxOption.cpp:86: if
(selectElement.multiple() && isPresentationRole(parent))
selectElement.multiple() isn't the same as a select element being a listbox vs a
menulist, because <select size=5> also looks like a listbox.

Also note that on some platforms the logic may be different - for example on
Android a select element is always a menu list.

The best check is to get the layoutObject and check layoutObject->isListBox()

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:192: // If it has none or
presentation role explicitly by itself, return true.
I'd delete this comment - the code is clear by itself

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:197: if (canSetFocusAttribute())
Should this be above? If an element is focusable I think that should override
role=presentation, right?

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:210: // The parent should have
presentationalRole.
I think it's clear without this comment.

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:214: // ARIA spec says that when a
parent object is presentational, and it has required owned elements,
How about: ARIA spec says that when a parent object is presentational and this
object
is a required owned element of that parent, then this object is also
presentational.

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:2036: bool
AXNodeObject::isListElement(Node* node) const
This shouldn't be a member function since it's just checking its argument,
|node|, not |this|. Make it a static helper function and take it out of the
header file.

(If you need it in another source file, making it a static class function is
okay.)

static bool isListElement(Node* node) const
{
    ...
}

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:2041: bool
AXNodeObject::isPresentationRole(const AXObject* object) const
Same, looks like it should be static

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:2047: bool
AXNodeObject::isPresentationRoleInTable(AXObject* parent) const
same?

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:2073: bool
AXNodeObject::isRequiredOwnedElement(AXObject* parent) const
same?

[je_julie(Not used), 2015-04-01 09:18:13]

Patchset #3 (id:40001) has been deleted

[je_julie(Not used), 2015-04-01 12:51:28]

I updated code as you commented.
PTAL.

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXLayoutObject.cpp:566: if
(isPresentationRole(static_cast<const AXObject*>(this)))
On 2015/03/31 16:35:23, dmazzoni wrote:
> Can you do this without the cast?
> 
> If you do need a cast it should probably be const_cast instead of static_cast
> 

Because |this| is AXLayoutObject and parameter type is AXObject, I needed it.
But I removed parameter on new patchset.

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXListBoxOption.cpp (right):

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXListBoxOption.cpp:86: if
(selectElement.multiple() && isPresentationRole(parent))
On 2015/03/31 16:35:23, dmazzoni wrote:
> selectElement.multiple() isn't the same as a select element being a listbox vs
a
> menulist, because <select size=5> also looks like a listbox.
> 
> Also note that on some platforms the logic may be different - for example on
> Android a select element is always a menu list.
> 
> The best check is to get the layoutObject and check layoutObject->isListBox()
> 

I updated it with layoutObject->isListBox().

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:192: // If it has none or
presentation role explicitly by itself, return true.
On 2015/03/31 16:35:23, dmazzoni wrote:
> I'd delete this comment - the code is clear by itself

Done.

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:197: if (canSetFocusAttribute())
On 2015/03/31 16:35:23, dmazzoni wrote:
> Should this be above? If an element is focusable I think that should override
> role=presentation, right?

You're right!

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:210: // The parent should have
presentationalRole.
On 2015/03/31 16:35:23, dmazzoni wrote:
> I think it's clear without this comment.

Done.

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:214: // ARIA spec says that when a
parent object is presentational, and it has required owned elements,
On 2015/03/31 16:35:24, dmazzoni wrote:
> How about: ARIA spec says that when a parent object is presentational and this
> object
> is a required owned element of that parent, then this object is also
> presentational.

Good. Thanks!

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:2036: bool
AXNodeObject::isListElement(Node* node) const
On 2015/03/31 16:35:23, dmazzoni wrote:
> This shouldn't be a member function since it's just checking its argument,
> |node|, not |this|. Make it a static helper function and take it out of the
> header file.
> 
> (If you need it in another source file, making it a static class function is
> okay.)
> 
> static bool isListElement(Node* node) const
> {
>     ...
> }

Done.

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:2041: bool
AXNodeObject::isPresentationRole(const AXObject* object) const
On 2015/03/31 16:35:24, dmazzoni wrote:
> Same, looks like it should be static

It's used on AXLayoutObject and AXListBoxOption.
So, instead of changing to static API, I removed parameter and moved it to
AXObject like other role checking APIs.

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:2047: bool
AXNodeObject::isPresentationRoleInTable(AXObject* parent) const
On 2015/03/31 16:35:23, dmazzoni wrote:
> same?

I added more parameter and changed it to static function.

https://codereview.chromium.org/1039873002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:2073: bool
AXNodeObject::isRequiredOwnedElement(AXObject* parent) const
On 2015/03/31 16:35:23, dmazzoni wrote:
> same?

I also added more parameter and changed it to static function.

[dmazzoni, 2015-04-01 15:27:55]

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXListBoxOption.cpp (right):

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXListBoxOption.cpp:85: if
(layoutObject->isListBox() && (parent->isPresentation() ||
parent->hasInheritedPresentationRole()))
Do you need both isPresentation and hasInheritedPresentationRole?

I thought hasInheritedPresentationRole will return true if the role is
presentation.

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:236: return
isHTMLTableSectionElement(*parentNode);
Aren't the table section elements optional in html? I usually write:

  <table>
    <tr>
      <td>Cell</td>
    </tr>
  </table>

Does this work correctly or do we also need to check if the parent of a table
row is a table element?

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:251: if (roleValue() == NoneRole
|| roleValue() == PresentationalRole)
This can be isPresentation()

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:381: bool isPresentation() const {
return roleValue() == NoneRole || roleValue() == PresentationalRole; }
Name this isPresentational() to be consistent with:

    virtual bool ariaRoleHasPresentationalChildren() const { return false; }
    virtual bool isPresentationalChildOfAriaRole() const { return false; }

Maybe throughout the change, use "is presentational" to mean something whose
role is "presentation" or "none".

[je_julie(Not used), 2015-04-02 05:09:48]

Patchset #4 (id:80001) has been deleted

[je_julie(Not used), 2015-04-02 05:51:28]

Thanks for your comment.
I updated patchset. PTAL.

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXListBoxOption.cpp (right):

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXListBoxOption.cpp:85: if
(layoutObject->isListBox() && (parent->isPresentation() ||
parent->hasInheritedPresentationRole()))
On 2015/04/01 15:27:55, dmazzoni wrote:
> Do you need both isPresentation and hasInheritedPresentationRole?
> 
> I thought hasInheritedPresentationRole will return true if the role is
> presentation.
> 

Yes. you're right. Done.

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:236: return
isHTMLTableSectionElement(*parentNode);
On 2015/04/01 15:27:55, dmazzoni wrote:
> Aren't the table section elements optional in html? I usually write:
> 
>   <table>
>     <tr>
>       <td>Cell</td>
>     </tr>
>   </table>
> 
> Does this work correctly or do we also need to check if the parent of a table
> row is a table element?

AFIK, table section is automatically generated from
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

So, if we capture render tree with the example above, we can see the result
below.

#READY
Content-Type: text/plain
layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
layer at (0,0) size 800x600
  LayoutBlockFlow {HTML} at (0,0) size 800x600
    LayoutBlockFlow {BODY} at (8,8) size 784x584
      LayoutTable {TABLE} at (0,0) size 30x26
        LayoutTableSection {TBODY} at (0,0) size 30x26
          LayoutTableRow {TR} at (0,2) size 30x22
            LayoutTableCell {TD} at (2,2) size 26x22 [r=0 c=0 rs=1 cs=1]
              LayoutText {#text} at (1,1) size 24x19
                text run at (1,1) width 24: "Cell"
#EOF

If I'm wrong, please correct me.
Thanks,

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:251: if (roleValue() == NoneRole
|| roleValue() == PresentationalRole)
On 2015/04/01 15:27:55, dmazzoni wrote:
> This can be isPresentation()

Done.

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:381: bool isPresentation() const {
return roleValue() == NoneRole || roleValue() == PresentationalRole; }
On 2015/04/01 15:27:55, dmazzoni wrote:
> Name this isPresentational() to be consistent with:
> 
>     virtual bool ariaRoleHasPresentationalChildren() const { return false; }
>     virtual bool isPresentationalChildOfAriaRole() const { return false; }
> 
> Maybe throughout the change, use "is presentational" to mean something whose
> role is "presentation" or "none".
> 

Done.

[dmazzoni, 2015-04-02 06:22:18]

Thanks for your work on this change. It ended up being tricky but it's worth it
to get it right.

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1039873002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:236: return
isHTMLTableSectionElement(*parentNode);
On 2015/04/02 05:51:28, je_julie wrote:
> On 2015/04/01 15:27:55, dmazzoni wrote:
> > Aren't the table section elements optional in html? I usually write:
> > 
> >   <table>
> >     <tr>
> >       <td>Cell</td>
> >     </tr>
> >   </table>
> > 
> > Does this work correctly or do we also need to check if the parent of a
table
> > row is a table element?
> 
> AFIK, table section is automatically generated from
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
> 
> So, if we capture render tree with the example above, we can see the result
> below.
> 
> #READY
> Content-Type: text/plain
> layer at (0,0) size 800x600
>   LayoutView at (0,0) size 800x600
> layer at (0,0) size 800x600
>   LayoutBlockFlow {HTML} at (0,0) size 800x600
>     LayoutBlockFlow {BODY} at (8,8) size 784x584
>       LayoutTable {TABLE} at (0,0) size 30x26
>         LayoutTableSection {TBODY} at (0,0) size 30x26
>           LayoutTableRow {TR} at (0,2) size 30x22
>             LayoutTableCell {TD} at (2,2) size 26x22 [r=0 c=0 rs=1 cs=1]
>               LayoutText {#text} at (1,1) size 24x19
>                 text run at (1,1) width 24: "Cell"
> #EOF
> 
> If I'm wrong, please correct me.
> Thanks,

You're right that the layout tree for a table is automatically generated and
it's normalized like that no matter what the HTML input.

However, your code isn't walking the layout tree, it's walking the DOM tree.
Usually, the DOM tree is basically whatever was parsed directly from the HTML. I
don't think it adds any other elements.

So, I think the parent of a <tr> might be several possible elements.

Probably the best thing to do would be to add some separate tests for these
specific cases, test;

<table role="presentation"><tr>
<table role="presentation"><tbody><tr>

...and see what the parent of the <tr> element is in each case, and what should
happen.

Some tips for debugging: to print out a node, use:
fprintf(stderr, "node name is: %s", node->nodeName().utf8().data());

To print a layoutObject:
fprintf(stderr, "layoutObject is: %s", layoutObject->debugName().utf8().data());

https://codereview.chromium.org/1039873002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXListBoxOption.cpp (right):

https://codereview.chromium.org/1039873002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXListBoxOption.cpp:62: // Generally, the
presentation role is handled in computeAccessibilityIsIgnored().
Let's delete the first three lines of this comment. The rest, starting with
http://www.w3.org/TR/wai-aria/complete#presentation, explains it perfectly.

https://codereview.chromium.org/1039873002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXListBoxOption.h (right):

https://codereview.chromium.org/1039873002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXListBoxOption.h:67: bool
isParentPresentationRole() const;
How about isParentPresentational()

(since "presentational" is how we're describing role="presentation" OR
role="none")

[je_julie(Not used), 2015-04-02 13:38:08]

Hi Dominic,
I think that code is getting clear through the review and I appreciate your time
and comment.

I added the case as you mentioned like
<table role="presentation">
    <tr>
       <th>Presentation th</th>
    </tr>
</table>
in presentation-owned-elements.html.
We have already table section case there.

In order to make sure,
I dump DOM tree and LayoutTree as well using showTree in node.h with
<table>
  <tr>
    <td>Cell</td>
  </tr>
</table>

<table>
  <tbody>
  <tr>
    <td>Cell</td>
  </tr>
  </tbody>
</table>

The result is below.
*#document      0x25a2fc204bd0
        HTML    0x25a2fc2141a8
                HEAD    0x25a2fc214230
                BODY    0x25a2fc2142b8
                        TABLE   0x25a2fc220010
                                #text   0x25a2fc230010 "\n  "
                                TBODY   0x25a2fc214340
                                        TR      0x25a2fc2143c8
                                                #text   0x25a2fc230080 "\n    "
                                                TD      0x25a2fc214450
                                                        #text   0x25a2fc2300f0
"Cell"
                                                #text   0x25a2fc230160 "\n  "
                                        #text   0x25a2fc2301d0 "\n"
                        #text   0x25a2fc230240 "\n\n"
                        TABLE   0x25a2fc2200b0
                                #text   0x25a2fc2302b0 "\n  "
                                TBODY   0x25a2fc2144d8
                                        #text   0x25a2fc230320 "\n  "
                                        TR      0x25a2fc214560
                                                #text   0x25a2fc230390 "\n    "
                                                TD      0x25a2fc2145e8
                                                        #text   0x25a2fc230400
"Cell"
                                                #text   0x25a2fc230470 "\n  "
                                        #text   0x25a2fc2304e0 "\n  "
                                #text   0x25a2fc230550 "\n"
Content-Type: text/plain
layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
layer at (0,0) size 800x600
  LayoutBlockFlow {HTML} at (0,0) size 800x600
    LayoutBlockFlow {BODY} at (8,8) size 784x584
      LayoutTable {TABLE} at (0,0) size 30x26
        LayoutTableSection {TBODY} at (0,0) size 30x26
          LayoutTableRow {TR} at (0,2) size 30x22
            LayoutTableCell {TD} at (2,2) size 26x22 [r=0 c=0 rs=1 cs=1]
              LayoutText {#text} at (1,1) size 24x19
                text run at (1,1) width 24: "Cell"
      LayoutTable {TABLE} at (0,26) size 30x26
        LayoutTableSection {TBODY} at (0,0) size 30x26
          LayoutTableRow {TR} at (0,2) size 30x22
            LayoutTableCell {TD} at (2,2) size 26x22 [r=0 c=0 rs=1 cs=1]
              LayoutText {#text} at (1,1) size 24x19
                text run at (1,1) width 24: "Cell"
PTAL.

https://codereview.chromium.org/1039873002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXListBoxOption.cpp (right):

https://codereview.chromium.org/1039873002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXListBoxOption.cpp:62: // Generally, the
presentation role is handled in computeAccessibilityIsIgnored().
On 2015/04/02 06:22:18, dmazzoni wrote:
> Let's delete the first three lines of this comment. The rest, starting with
> http://www.w3.org/TR/wai-aria/complete#presentation, explains it perfectly.

Done.

https://codereview.chromium.org/1039873002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXListBoxOption.h (right):

https://codereview.chromium.org/1039873002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXListBoxOption.h:67: bool
isParentPresentationRole() const;
On 2015/04/02 06:22:18, dmazzoni wrote:
> How about isParentPresentational()
> 
> (since "presentational" is how we're describing role="presentation" OR
> role="none")

Done.

[dmazzoni, 2015-04-02 13:53:20]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-04-02 13:53:20]

lgtm

[commit-bot: I haz the power, 2015-04-02 13:53:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1039873002/120001

[commit-bot: I haz the power, 2015-04-02 15:07:09]

Committed patchset #5 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193042