Bug 123885 - AX: [ATK] <span> elements exposed through ATK when not needed
Summary: AX: [ATK] <span> elements exposed through ATK when not needed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-11-06 03:10 PST by Mario Sanchez Prada
Modified: 2013-11-08 02:53 PST (History)
15 users (show)

See Also:


Attachments
Patch proposal (15.53 KB, patch)
2013-11-06 07:55 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (13.28 KB, patch)
2013-11-07 06:41 PST, Mario Sanchez Prada
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch proposal (13.48 KB, patch)
2013-11-07 07:24 PST, Mario Sanchez Prada
cfleizach: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2013-11-06 03:10:13 PST
The patch for bug 121335 has introduced a change in how is determined when an accessibility object gets ignored that causes that all <span> elements show up in the accessibility tree even when they should not (e.g. not explicitly focusable due to tabindex, no ARIA rule mandating that, no meaningful accessible name...)

For that reason the following tests are failing now, as the StaticText objects inside those <span> elements are not getting properly folded into their parents anymore:

platform/gtk/accessibility/spans-paragraphs-and-divs.html
platform/gtk/accessibility/spans.html
Comment 1 Radar WebKit Bug Importer 2013-11-06 03:10:44 PST
<rdar://problem/15402535>
Comment 2 Mario Sanchez Prada 2013-11-06 07:55:52 PST
Created attachment 216177 [details]
Patch proposal
Comment 3 chris fleizach 2013-11-06 08:19:45 PST
Comment on attachment 216177 [details]
Patch proposal

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1925
> +

can all these checks move into AccessibilityObject?

> Source/WebCore/accessibility/AccessibilityObject.h:608
> +    virtual bool hasAccessibleName() const;

I think this should be called something like hasAccessibleAttributes() or hasRequiredInclusionAttributes() or hasAttributedRequiredForInclusion()

something to denote that this method should be used to determine whether to include it in the tree
Comment 4 Mario Sanchez Prada 2013-11-06 09:51:23 PST
Comment on attachment 216177 [details]
Patch proposal

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

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1925
>> +
> 
> can all these checks move into AccessibilityObject?

Hmm... yes and no: we can move the MathML part but not the ariaAccessibilityDescription() call since that's a non-virtual method from AccessibilityNodeObject.

>> Source/WebCore/accessibility/AccessibilityObject.h:608
>> +    virtual bool hasAccessibleName() const;
> 
> I think this should be called something like hasAccessibleAttributes() or hasRequiredInclusionAttributes() or hasAttributedRequiredForInclusion()
> 
> something to denote that this method should be used to determine whether to include it in the tree

Believe it or not, I struggled to find a name for this and in the end this is the least bad I found.

hasAttributedRequiredForInclusion() sounds good to me, though
Comment 5 chris fleizach 2013-11-06 11:02:51 PST
(In reply to comment #4)
> (From update of attachment 216177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216177&action=review
> 
> >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1925
> >> +
> > 
> > can all these checks move into AccessibilityObject?
> 
> Hmm... yes and no: we can move the MathML part but not the ariaAccessibilityDescription() call since that's a non-virtual method from AccessibilityNodeObject.
> 
> >> Source/WebCore/accessibility/AccessibilityObject.h:608
> >> +    virtual bool hasAccessibleName() const;
> > 
> > I think this should be called something like hasAccessibleAttributes() or hasRequiredInclusionAttributes() or hasAttributedRequiredForInclusion()
> > 
> > something to denote that this method should be used to determine whether to include it in the tree
> 
> Believe it or not, I struggled to find a name for this and in the end this is the least bad I found.
> 
> hasAttributedRequiredForInclusion() sounds good to me, though

should be: hasAttributesRequiredForInclusion()
Comment 6 Mario Sanchez Prada 2013-11-07 06:41:42 PST
Created attachment 216298 [details]
Patch proposal

(In reply to comment #5)
> [...]
> should be: hasAttributesRequiredForInclusion()

New patch attached renaming that function and moving the MathML bits up into AccessibilityObject
Comment 7 EFL EWS Bot 2013-11-07 06:47:27 PST
Comment on attachment 216298 [details]
Patch proposal

Attachment 216298 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/21248312
Comment 8 EFL EWS Bot 2013-11-07 06:48:11 PST
Comment on attachment 216298 [details]
Patch proposal

Attachment 216298 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/22738203
Comment 9 kov's GTK+ EWS bot 2013-11-07 06:51:59 PST
Comment on attachment 216298 [details]
Patch proposal

Attachment 216298 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/22718202
Comment 10 Mario Sanchez Prada 2013-11-07 07:24:01 PST
Created attachment 216299 [details]
Patch proposal
Comment 11 Mario Sanchez Prada 2013-11-08 02:53:45 PST
Committed r158914: <http://trac.webkit.org/changeset/158914>