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?
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.
Putting Chris on CC, as I'd be asking for review over the attached patch.
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)
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.
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
(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().
(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
Created attachment 155170 [details] Patch proposal Ok. Attaching a new patch now then
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
(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
Committed r124003: <http://trac.webkit.org/changeset/124003>