Bug 47636 – AX: GTK: ARIA role is not respected on <p> <label> <div> and <form>

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

In webkit_accessible_get_role, if the node is a <p> <label> <div> and <form>, GTK is automatically returning some sort of role.

However, that completely ignores whether ARIA has set a role, which would be returned from 

axObject->roleValue()...

GTK needs to respect the roles it gets from WebCore and then make determinations afterwards whether it should change things.

This is causing the new test added in https://bugs.webkit.org/show_bug.cgi?id=47564 to generate a result that has failures

[chris fleizach]

Please assign to whomever is working on AX these days. Thanks

[Mario Sanchez Prada]

Adding myself to CC

[Mario Sanchez Prada]

Blocking metabug blocking ORCA support in RIA

[Mario Sanchez Prada]

Created attachment 87292 [details]
Patch proposal + Layout test

Attaching patch proposal + Layout test.

Note: the patch is basically about adding an extra condition to the 'if' statement.

-    if (axObject->isAccessibilityRenderObject()) {
-        Node* node = static_cast<AccessibilityRenderObject*>(axObject)->renderer()->node();
-        if (node) {
+    if (coreObject->isAccessibilityRenderObject()) {
+        Node* node = static_cast<AccessibilityRenderObject*>(coreObject)->renderer()->node();
+        if (node && coreObject->ariaRoleAttribute() == UnknownRole) {

Then I also made the most of this patch to rename the axObject variable (normally used to refer instances of AtkObject) to coreObject, which is the normal variable name used for instances of AccessibilityObject. I know it messes up a little bit the patch but in the long term it makes IMHO the code cleaner and, after all, the patch is not that big and complex to consider it unreviewable, I think :-)

[chris fleizach]

Comment on attachment 87292 [details]
Patch proposal + Layout test

You should probably enable the other test that led to the creation of this bug (and verify that test passes now)

[chris fleizach]

Comment on attachment 87292 [details]
Patch proposal + Layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=87292&action=review

> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:494
> +        if (node && coreObject->ariaRoleAttribute() == UnknownRole) {

I think you should add ParagraphRole, LabelRole and so on to WebCore. Then you could just use roleValue() and you won't have to query the node of the renderer or check that it's a render object (which we should be trying to avoid). 
Those roles would be mapped to NSAccessibilityGroupRole on mac

[Mario Sanchez Prada]

(In reply to comment #5)
> (From update of attachment 87292 [details])
> You should probably enable the other test that led to the creation of this bug (and verify that test passes now)

Guess you mean platform/gtk/accessibility/aria-table-hierarchy.html, right?

If that's the case I'm much afraid I can't skip it yest since, even when this patch improves the result (tables are seen as tables, not sections), there's still a bug in the GTK port which prevents it to see the rows and cols, and thus the cells, in those tables. And that should be IMHO addressed through a different bug I'll probably file later on.

[Mario Sanchez Prada]

(In reply to comment #6)
> (From update of attachment 87292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87292&action=review
> 
> > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:494
> > +        if (node && coreObject->ariaRoleAttribute() == UnknownRole) {
> 
> I think you should add ParagraphRole, LabelRole and so on to WebCore. Then you could just use roleValue() and you won't have to query the node of the renderer or check that it's a render object (which we should be trying to avoid). 
> Those roles would be mapped to NSAccessibilityGroupRole on mac

I like this, implementing a new patch right now that way...

[Mario Sanchez Prada]

Created attachment 87371 [details]
Patch proposal + Layout test

(In reply to comment #8)
> [...] 
> I like this, implementing a new patch right now that way...

Here you have the new one.

About the bug preventing from unskipping the other layout test, if it's ok to you, I'd prefer to file it after fixing this stuff, so I can be sure about what the root problem actually is.

[chris fleizach]

Comment on attachment 87371 [details]
Patch proposal + Layout test

can you also add in the mappings for the mac wrapper. it should be clear

[Mario Sanchez Prada]

(In reply to comment #10)
> (From update of attachment 87371 [details])
> can you also add in the mappings for the mac wrapper. it should be clear

Sure, I just thought it would already work that way without doing anything, like some kind of fallback.

But no problem in adding explicit lines in the mac wrapper of course (jsut hope to do it right)

[Mario Sanchez Prada]

Created attachment 87375 [details]
Patch proposal + Layout test

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 87371 [details] [details])
> > can you also add in the mappings for the mac wrapper. it should be clear
> 
> Sure, I just thought it would already work that way without doing anything, like some kind of fallback.
> 
> But no problem in adding explicit lines in the mac wrapper of course (jsut hope to do it right)

Done

[chris fleizach]

Comment on attachment 87375 [details]
Patch proposal + Layout test

Looks good. Thanks

[Mario Sanchez Prada]

(In reply to comment #13)
> (From update of attachment 87375 [details])
> Looks good. Thanks

Thanks to you for the responsiveness and quick reviews!

Now I'm in a hurry, but first thing I'll do tomorrow morning will be to check the problem around the aria-table-hierarchy.html test and file a bug accordingly.

Now closing this, as it seems webkit-patch failed to do it so...

Committed r82295: <http://trac.webkit.org/changeset/82295>

[Mario Sanchez Prada]

Reopening as it seems the current patch was twice wrong :-(:

 1. It made Chromium bots to fail compiling
 2. It made Leopard tests to fail

About (1), I tried to quickly write a patch to fix it when I thought that was the only problem, but it seems I messed up in one line and that I basically replaced one compilation problem with another one :/.

The patch I committed to fix that was committed in review 82300 [1], and the wrong line in there is obviously this one:

  COMPILE_ASSERT_MATCHING_ENUM(WebAccessibilityRoleDiv, LabelRole);

Most likely, if I hadn't made that mistake Chromium bots would be compiling fine, so it's worth bearing this in mind when writing the next pach.

Anyway, that was not enough since later on we found out about the problem (2), so we finally decided to roll out both revisions 82295 and 82300 through bug 57380, as I have no more time today to sit down, think and write a patch that doesn't make those leopard tests to fail [2].

Thus, tomorrow will be a new day and I will get back to this stuff. In the meanwhile, let's reopen this...

[1]http://trac.webkit.org/changeset/82300
[2]http://build.webkit.org/results/Leopard%20Intel%20Release%20%28Tests%29/r82295%20%2830326%29/results.html

[Mario Sanchez Prada]

(In reply to comment #15)
> [...]
> Anyway, that was not enough since later on we found out about the problem (2), 
> so we finally decided to roll out both revisions 82295 and 82300 through bug 
> 57380, as I have no more time today to sit down, think and write a patch that 
> doesn't make those leopard tests to fail [2].
> 
> [...] 
> [2]http://build.webkit.org/results/Leopard%20Intel%20Release%20%28Tests%29/r82295%20%2830326%29/results.html

Btw Chris, if you could take a look into those errors and provide some feedback on why Mac is failing that way, that would be awesome, as I'm not sure whether I'll be able to cook a patch for that on my own easily, since I suspect the reason behind this problem could be that just doing that mapping is not enough in Mac... we could be messing up with the roles somehow, I'm afraid.

[chris fleizach]

(In reply to comment #16)
> (In reply to comment #15)
> > [...]
> > Anyway, that was not enough since later on we found out about the problem (2), 
> > so we finally decided to roll out both revisions 82295 and 82300 through bug 
> > 57380, as I have no more time today to sit down, think and write a patch that 
> > doesn't make those leopard tests to fail [2].
> > 
> > [...] 
> > [2]http://build.webkit.org/results/Leopard%20Intel%20Release%20%28Tests%29/r82295%20%2830326%29/results.html
> 
> Btw Chris, if you could take a look into those errors and provide some feedback on why Mac is failing that way, that would be awesome, as I'm not sure whether I'll be able to cook a patch for that on my own easily, since I suspect the reason behind this problem could be that just doing that mapping is not enough in Mac... we could be messing up with the roles somehow, I'm afraid.

This is strange that it would only affect leopard. And it's strange that it happens. It indicates that some element's role changed away from NSAccessibilityGroup, which would remove the the title ui element attribute. we might have to check in skip lists and then file another bug to fix

[Mario Sanchez Prada]

(In reply to comment #17)
> [...]
> This is strange that it would only affect leopard. And it's strange that it 
> happens. 

Well, actually I have double checked now and it seems it's also failing in Snow Leopard as well:

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r82295%20%2827423%29/accessibility


> It indicates that some element's role changed away from 
> NSAccessibilityGroup, which would remove the the title ui element attribute. we 
> might have to check in skip lists and then file another bug to fix

By just reading the code in AccessibilityObjectWrapper.mm (and please take the following with a grain of salt), I have the feeling the problem with those tests is that they use some of the four elements we defined new roles for (e.g. <p>) and, even if the following code gets executed...

  - (NSArray*)accessibilityAttributeNames
  {
    [...]
    if (groupAttrs == nil) {
        tempArray = [[NSMutableArray alloc] initWithArray:attributes];
        [tempArray addObject:NSAccessibilityTitleUIElementAttribute];
        groupAttrs = [[NSArray alloc] initWithArray:tempArray];
        [tempArray release];
    }
    [...]
  }

...the value of that groupAttrs is being ignored because the accessible objects associated to the conflictive element are  returning false when calling to isGroup() (as the do not have roleValue() == GroupRole), hence the following code is not executed:

    [...]
    else if (m_object->isGroup() || m_object->isListItem())
        objectAttributes = groupAttrs;

    [...]

...and thus at the end the objectAttributes variable will hold just the content of the original attributes variable, which contains everything in the expected output but the AXTitleUIElement.

I basically see two solutions from my ignorant perspective:

  1. Add more OR conditions to the "if (m_object->isGroup() || m_object->isListItem())" statement, so it gets executed as well for ParagraphRole, DivRole, FormRole and LabelRole.

  2. Make the new code in AccessibilityRenderObject::determineAccessibilityRole() GTK-only, leaving it as it was before for the rest of the platforms.

Among these two options, I think (2) is the best one since it just changes the behaviour for GTK and the other platforms would keep working as they do right now, so I'll work on a new patch that way, but feel free to suggest me to do it in another way if you think it makes sense.

Thanks and excuse my verbosity :-)

[Mario Sanchez Prada]

Created attachment 87535 [details]
Patch proposal + Layout test

(In reply to comment #18)
> [...]
> I basically see two solutions from my ignorant perspective:
> 
>   1. Add more OR conditions to the "if (m_object->isGroup() || 
> m_object->isListItem())" statement, so it gets executed as well for 
> ParagraphRole, DivRole, FormRole and LabelRole.
> 
>   2. Make the new code in 
> AccessibilityRenderObject::determineAccessibilityRole() GTK-only, leaving it as 
> it was before for the rest of the platforms.
> 
> Among these two options, I think (2) is the best one since it just changes the 
> behaviour for GTK and the other platforms would keep working as they do right 
> now, so I'll work on a new patch that way, but feel free to suggest me to do it 
> in another way if you think it makes sense.

Attaching new patch addressing the problem with Mac in the previously mentioned way: now only GTK is actually affected, and the rest of the ports should remain unaltered.

Still, as the new roles are not GTK-specific, I maintained the changes in Mac wrapper (explicit mapping to Group role) and Chromium (duplicating the web core roles in those assertions).

I hope this time it's the right one. Will wait anyway for the EWS to run, to gain peace of mind :-)

[chris fleizach]

Comment on attachment 87535 [details]
Patch proposal + Layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=87535&action=review

Your changelogs have duplicate entries in them.

Can you file a Mac bug that raises this issue and assign it to me. I think the correct behavior is for all these elements to be group elements.

r=me with minor nits.

> LayoutTests/ChangeLog:11
> +

this comment is duplicated in the this changelog

[chris fleizach]

> I basically see two solutions from my ignorant perspective:
> 
>   1. Add more OR conditions to the "if (m_object->isGroup() || m_object->isListItem())" statement, so it gets executed as well for ParagraphRole, DivRole, FormRole and LabelRole.
> 
>   2. Make the new code in AccessibilityRenderObject::determineAccessibilityRole() GTK-only, leaving it as it was before for the rest of the platforms.
> 

isGroup() appears to be a platform dependent method, that's living in WebCore. I think that should be amended to reflect the new reality so that isGroup()  perhaps calls into platform specific code to determine if something is a group.


> Among these two options, I think (2) is the best one since it just changes the behaviour for GTK and the other platforms would keep working as they do right now, so I'll work on a new patch that way, but feel free to suggest me to do it in another way if you think it makes sense.
> 
> Thanks and excuse my verbosity :-)

[Mario Sanchez Prada]

Answering the two comments in a row...

(In reply to comment #20)
> [...]
> Can you file a Mac bug that raises this issue and assign it to me. I think 
> the correct behavior is for all these elements to be group elements.

If I raise a bug for this I think it wouldn't be Mac only, since GTK code would also benefit of that change, at least because that way AccessibilityObject::allowsTextRanges() wouldn't need any change as it needs now, since the already present isGroup() call would work for all these 4 elements as well (as I thin it would be OK to consider those 4 elements as Group elements as well in GTK).

I'll file that new bug and will comment this specific thing in the description.

> r=me with minor nits.

Ok.

> > LayoutTests/ChangeLog:11
> > +
> 
> this comment is duplicated in the this changelog

Oops, fixing it now.

(In reply to comment #21)
> [...]
> isGroup() appears to be a platform dependent method, that's living in WebCore.
> I think that should be amended to reflect the > new reality so that isGroup()
> perhaps calls into platform specific code to determine if something is a group.

I like this. The default implementation could just still be return role == GroupRole, though, despite of Mac and GTK adding the new 4 roles that list in platform specific code.

But, as you said, that's better another bug.

[Mario Sanchez Prada]

(In reply to comment #14)
> [...]
> Now I'm in a hurry, but first thing I'll do tomorrow morning will be to check 
> the problem around the aria-table-hierarchy.html test and file a bug 
> accordingly.

Not the first thing I did at the end, nor the last one either:
https://bugs.webkit.org/show_bug.cgi?id=57463

[Mario Sanchez Prada]

Committed r82459: <http://trac.webkit.org/changeset/82459>