REGRESSION (r123767): platform/gtk/accessibility/object-with-title.html failing on GTK

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zan Dobersek]

platform/gtk/accessibility/object-with-title.html started failing on GTK after r123767.
http://trac.webkit.org/changeset/123767/trunk

--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/platform/gtk/accessibility/object-with-title-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/platform/gtk/accessibility/object-with-title-actual.txt 
@@ -8,8 +8,8 @@
 
 
 PASS child.role is 'AXRole: list'
-PASS child.role is 'AXRole: section'
-PASS child.role is 'AXRole: list'
+FAIL child.role should be AXRole: section. Was AXRole: list.
+FAIL child.role should be AXRole: list. Was AXRole: list item.
 PASS successfullyParsed is true
 
 TEST COMPLETE

Perhaps this is actually a progression and the test case should be updated?

[Mario Sanchez Prada]

Created attachment 154981 [details]
Patch proposal

(In reply to comment #0)
> platform/gtk/accessibility/object-with-title.html started failing on GTK after r123767.
> http://trac.webkit.org/changeset/123767/trunk
> [...]
> Perhaps this is actually a progression and the test case should be updated?

Doesn't seem so. It seems the problem is that the <div> holding the second list is not being exposed (when it should be) because helpText() is not returning the title attribute, just because it happens to be the same than the accessibility description (that changed in r123767).

This seems to be a problem for the GTK port because we expect to expose any object with an accessibility description available, regardless of the value of helpText(), which GTK doesn't use (it seems only the mac uses it).

So, in order to fix this I propose the current patch, which only affects the GTK port and that just works on the following simple assumption: if an object has an accessible description available, it should be exposed. Considering that, according ARIA specs, the 'title' attribute is the last resort when calculating the alternative text for an element, I think it's a pretty reasonable assumption to make.

[Mario Sanchez Prada]

Putting Chris on CC, as I'd be asking for review over the attached patch.

[chris fleizach]

Comment on attachment 154981 [details]
Patch proposal

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

> Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:77
> +    if (!accessibilityDescription().isEmpty())

i might change this to 

if (!accessibilityDescription().isEmpty() || !title.isEmpty())

as the "accessible" name of an object is split between these two concepts, which are modeled on the Mac API where title is what you get visually on the screen if available, and axDescription is what you get when the description is not really visible (think alt tag on image)

[Mario Sanchez Prada]

Created attachment 155132 [details]
Patch proposal

(In reply to comment #3)
> (From update of attachment 154981 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154981&action=review
> 
> > Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:77
> > +    if (!accessibilityDescription().isEmpty())
> 
> i might change this to 
> 
> if (!accessibilityDescription().isEmpty() || !title.isEmpty())
> 
> as the "accessible" name of an object is split between these two concepts, which are modeled on the Mac API
> where title is what you get visually on the screen if available, and axDescription is what you get when the
> description is not really visible (think alt tag on image)

Good point.

Actually, there was another issue with that solution (which was causing some tests to fail) and it's that calling to title() / accessibilityDescription() from this function will cause infinite recursion since accessibilityIsIgnored() will be called again at some point after calling those functions.

So, and considering that helpText() is something that seems to be used only on the Mac, I made a change directly in AccessibilityRenderObject::accessibilityIsIgnored(), so we decide not to ignore an object if helpText is not empty when on the Mac, and when (title || accessibilityDescription) is not empty in other platforms.

This seems to fix the issue indeed, while not causing any regression, at least in the GTK port.

[chris fleizach]

Comment on attachment 155132 [details]
Patch proposal

i feel like these checks make sense for all platforms. 
what we're talking about here are things like <div> or <span> not showing up in the tree when they have some sort of accessible name. 
I think it makes sense that if something has helpText(), title or axDescription() then it should NOT be ignored at this point. the only things to verify is that helpText(), title and axDescription do not call accessibilityIsIgnored() at any point

[Mario Sanchez Prada]

(In reply to comment #5)
> (From update of attachment 155132 [details])
> i feel like these checks make sense for all platforms. 
> what we're talking about here are things like <div> or <span> not showing up in 
> the tree when they have some sort of accessible name. 
> I think it makes sense that if something has helpText(), title or axDescription()
> then it should NOT be ignored at this point.

So, you are proposing to just add the new checks right after the helpText() check, not using #if guards at all, right?

> the only things to verify is that helpText(), title and axDescription do not
> call accessibilityIsIgnored() at any point

I think that's not an issue here. The problem happened when calling from accessibilityPlatformIncludesObject().

[chris fleizach]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 155132 [details] [details])
> > i feel like these checks make sense for all platforms. 
> > what we're talking about here are things like <div> or <span> not showing up in 
> > the tree when they have some sort of accessible name. 
> > I think it makes sense that if something has helpText(), title or axDescription()
> > then it should NOT be ignored at this point.
> 
> So, you are proposing to just add the new checks right after the helpText() check, not using #if guards at all, right?
>
correct

[Mario Sanchez Prada]

Created attachment 155170 [details]
Patch proposal

Ok. Attaching a new patch now then

[chris fleizach]

Comment on attachment 155170 [details]
Patch proposal

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2001
> +    if (!helpText().isEmpty() || !title().isEmpty() || !accessibilityDescription().isEmpty())

we should update the comments as well to reflect why we're doing this

[Mario Sanchez Prada]

(In reply to comment #9)
> (From update of attachment 155170 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155170&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2001
> > +    if (!helpText().isEmpty() || !title().isEmpty() || !accessibilityDescription().isEmpty())
> 
> we should update the comments as well to reflect why we're doing this

Right. I'll do it when landing.

Thanks

[Mario Sanchez Prada]

Committed r124003: <http://trac.webkit.org/changeset/124003>