Bug 123885

Summary: AX: [ATK] <span> elements exposed through ATK when not needed
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, eflews.bot, gtk-ews, gyuyoung.kim, jcraig, jdiggs, k.czech, rego+ews, samuel_white, webkit-bug-importer, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch proposal
none
Patch proposal
eflews.bot: commit-queue-
Patch proposal cfleizach: review+

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>