Bug 123568 – AX: Children Nodes for Canvas objects are not equal to Render Objects

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Artur Moryc]

There is a difference in children nodes taken into account for RenderObject and  for NodeObject types. 
Text nodes are eliminated in the AccessibilityRenderObject::computeAccessibilityIsIgnored() method but in 
AccessibilityNodeObject::computeAccessibilityIsIgnored() are not considered and therefore are always added to the m_children vector. 

In Mac behaviour is similar but the test canvas-accessibilitynodeobject.html passes since the text nodes are not focusable which result in 
having equal number of children nodes for the RenderObject and NodeObject types. But for efl/gtk there was a bug created
https://bugs.webkit.org/show_bug.cgi?id=27048 according to which the text nodes are required to be focusable.

The question is how to adjust the canvas-accessibilitynodeobject.html test to the needs of efl/gtk ports?

The first idea could be to eliminate the text node in AccessibilityNodeObject::computeAccessibilityIsIgnored() respectively as for RenderObjects.
Which result in that these objects have the same number of children nodes and the test passes without causing any regression so far. 
Although it is wondering whether this approach is fully correct?

[Artur Moryc]

any comments from reviewers?

[Artur Moryc]

any comments from reviewers?

[Artur Moryc]

Created attachment 216926 [details]
Proposed solution for children nodes for the Canvas objects

The proposed solution concerns the test canvas-accessibilitynodeobject.html which at this moment doesn't pass for efl/gtk.

[Mario Sanchez Prada]

Comment on attachment 216926 [details]
Proposed solution for children nodes for the Canvas objects

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:416
> +#if  PLATFORM(EFL) ||  PLATFORM(GTK)

extra space after ||

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:419
> +#if  PLATFORM(EFL) ||  PLATFORM(GTK)
> +    if (m_node->isTextNode())
> +        return true;
> +#endif

Hmm... I don't think this is the best approach to skip text nodes from this point. It seems to me like it will be fine for this very specific case but it might fail for some others that are also platform specific.

What about trying to do something similar to what it's done in AccessibilityRenderObject::computeAccessibilityIsIgnored(), so we check first whether the defaultObjectInclusion() function (which also checks platform specific stuff) to see if we can know in advance whether we should be ignoring things?

I mean, something like this:

bool AccessibilityNodeObject::computeAccessibilityIsIgnored() const
{
#ifndef NDEBUG
    // Double-check that an AccessibilityObject is never accessed before
    // it's been initialized.
    ASSERT(m_initialized);
#endif

    AccessibilityObjectInclusion decision = defaultObjectInclusion();
    if (decision == IncludeObject)
        return false;
    if (decision == IgnoreObject)
        return true;
    
    // If this element is within a parent that cannot have children, it should not be exposed.
    if (isDescendantOfBarrenParent())
        return true;

    return m_role == UnknownRole;
}

Would that work OK?

[Artur Moryc]

Created attachment 217039 [details]
New solution for text nodes for Canvas objects

Thanks for solution improvement. It works and it doesn't cause regression on efl accessibility and for Mac as well.

[chris fleizach]

Comment on attachment 217039 [details]
New solution for text nodes for Canvas objects

I think we should add a layout test for this

[Artur Moryc]

(In reply to comment #6)
> (From update of attachment 217039 [details])
> I think we should add a layout test for this

There is already a test - canvas-accessibilitynodeobject.html that verifies if canvas object nodes are equal to the render object. After the fix the test passes. Shell I come up with a new layout test?

[chris fleizach]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 217039 [details] [details])
> > I think we should add a layout test for this
> 
> There is already a test - canvas-accessibilitynodeobject.html that verifies if canvas object nodes are equal to the render object. After the fix the test passes. Shell I come up with a new layout test?

Shouldn't this patch update the TestExpectations then? I would imagine the test is marked as a failure right now right

[Artur Moryc]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 217039 [details] [details] [details])
> > > I think we should add a layout test for this
> > 
> > There is already a test - canvas-accessibilitynodeobject.html that verifies if canvas object nodes are equal to the render object. After the fix the test passes. Shell I come up with a new layout test?
> 
> Shouldn't this patch update the TestExpectations then? I would imagine the test is marked as a failure right now right

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 217039 [details] [details] [details])
> > > I think we should add a layout test for this
> > 
> > There is already a test - canvas-accessibilitynodeobject.html that verifies if canvas object nodes are equal to the render object. After the fix the test passes. Shell I come up with a new layout test?
> 
> Shouldn't this patch update the TestExpectations then? I would imagine the test is marked as a failure right now right

Yes the TestExpectations file will be updated.

[Artur Moryc]

Created attachment 218163 [details]
Removing canvas-accessibilitynodeobject.html from TestExpectations for efl-wk1 efl-wk2 and gtk 

The canvas-accessibilitynodeobject.htm test passes now.

[Artur Moryc]

Created attachment 218166 [details]
Removing canvas-accessibilitynodeobject.html from TestExpectations for efl-wk1 efl-wk2 and gtk

the canvas-accessibilitynodeobject.html passes.

[Artur Moryc]

Created attachment 218172 [details]
New solution for text nodes for Canvas objects

The canvas-accessibilitynodeobject.html test is not failing any more.

[Artur Moryc]

Created attachment 218173 [details]
New solution for text nodes for Canvas objects

The canvas-accessibilitynodeobject.html test is not failing anymore.

[Artur Moryc]

Any comments from reviewers?

[chris fleizach]

Comment on attachment 218173 [details]
New solution for text nodes for Canvas objects

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

otherwise looks ok

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

indentation seems wrong for a number of these lines

> Source/WebCore/ChangeLog:9
> +        and  for NodeObject types. Text nodes are eliminated in the

extraneous space between and and for

> Source/WebCore/ChangeLog:12
> +

Can your comment explain the manifestation of what this bug means a little better. It's not clear to me right now why we need to fix this (I sort of know, but a casual observer wouldn't)

[Artur Moryc]

Created attachment 218839 [details]
Updating solution for children nodes of the Canvas objects

[Artur Moryc]

Any comments from reviewers?

[chris fleizach]

Comment on attachment 218839 [details]
Updating solution for children nodes of the Canvas objects

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

except for spelling error should be ready to go

> Source/WebCore/ChangeLog:13
> +        approach has been applyed to the NodeObject to eliminate text nodes.

applyed -> applied

[chris fleizach]

Comment on attachment 218839 [details]
Updating solution for children nodes of the Canvas objects

r- for nitpicking spelling error. Please update patch when possible

[Radar WebKit Bug Importer]

<rdar://problem/16012354>

[Artur Moryc]

Created attachment 224376 [details]
Updating solution for children nodes of the Canvas objects

Thanks for previous comments!

[WebKit Commit Bot]

Comment on attachment 224376 [details]
Updating solution for children nodes of the Canvas objects

Clearing flags on attachment: 224376

Committed r164423: <http://trac.webkit.org/changeset/164423>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.