Bug 123568 - AX: Children Nodes for Canvas objects are not equal to Render Objects
Summary: AX: Children Nodes for Canvas objects are not equal to Render Objects
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-10-31 07:02 PDT by Artur Moryc
Modified: 2014-02-20 02:20 PST (History)
11 users (show)

See Also:


Attachments
Proposed solution for children nodes for the Canvas objects (1.89 KB, patch)
2013-11-14 06:17 PST, Artur Moryc
no flags Details | Formatted Diff | Diff
New solution for text nodes for Canvas objects (1.98 KB, patch)
2013-11-15 03:30 PST, Artur Moryc
no flags Details | Formatted Diff | Diff
Removing canvas-accessibilitynodeobject.html from TestExpectations for efl-wk1 efl-wk2 and gtk (3.38 KB, patch)
2013-12-02 05:43 PST, Artur Moryc
no flags Details | Formatted Diff | Diff
Removing canvas-accessibilitynodeobject.html from TestExpectations for efl-wk1 efl-wk2 and gtk (4.68 KB, patch)
2013-12-02 06:13 PST, Artur Moryc
no flags Details | Formatted Diff | Diff
New solution for text nodes for Canvas objects (4.76 KB, patch)
2013-12-02 07:26 PST, Artur Moryc
no flags Details | Formatted Diff | Diff
New solution for text nodes for Canvas objects (4.76 KB, patch)
2013-12-02 07:59 PST, Artur Moryc
no flags Details | Formatted Diff | Diff
Updating solution for children nodes of the Canvas objects (5.27 KB, patch)
2013-12-10 00:32 PST, Artur Moryc
cfleizach: review-
Details | Formatted Diff | Diff
Updating solution for children nodes of the Canvas objects (5.21 KB, patch)
2014-02-17 08:42 PST, Artur Moryc
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Artur Moryc 2013-10-31 07:02:50 PDT
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?
Comment 1 Artur Moryc 2013-11-06 03:17:22 PST
any comments from reviewers?
Comment 2 Artur Moryc 2013-11-06 04:47:22 PST
any comments from reviewers?
Comment 3 Artur Moryc 2013-11-14 06:17:07 PST
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.
Comment 4 Mario Sanchez Prada 2013-11-14 06:50:32 PST
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?
Comment 5 Artur Moryc 2013-11-15 03:30:50 PST
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.
Comment 6 chris fleizach 2013-11-15 05:49:01 PST
Comment on attachment 217039 [details]
New solution for text nodes for Canvas objects

I think we should add a layout test for this
Comment 7 Artur Moryc 2013-11-15 06:22:03 PST
(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?
Comment 8 chris fleizach 2013-11-15 10:26:19 PST
(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
Comment 9 Artur Moryc 2013-11-16 04:37:07 PST
(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.
Comment 10 Artur Moryc 2013-12-02 05:43:09 PST
Created attachment 218163 [details]
Removing canvas-accessibilitynodeobject.html from TestExpectations for efl-wk1 efl-wk2 and gtk 

The canvas-accessibilitynodeobject.htm test passes now.
Comment 11 Artur Moryc 2013-12-02 06:13:00 PST
Created attachment 218166 [details]
Removing canvas-accessibilitynodeobject.html from TestExpectations for efl-wk1 efl-wk2 and gtk

the canvas-accessibilitynodeobject.html passes.
Comment 12 Artur Moryc 2013-12-02 07:26:30 PST
Created attachment 218172 [details]
New solution for text nodes for Canvas objects

The canvas-accessibilitynodeobject.html test is not failing any more.
Comment 13 Artur Moryc 2013-12-02 07:59:35 PST
Created attachment 218173 [details]
New solution for text nodes for Canvas objects

The canvas-accessibilitynodeobject.html test is not failing anymore.
Comment 14 Artur Moryc 2013-12-06 04:16:27 PST
Any comments from reviewers?
Comment 15 chris fleizach 2013-12-06 08:02:20 PST
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)
Comment 16 Artur Moryc 2013-12-10 00:32:30 PST
Created attachment 218839 [details]
Updating solution for children nodes of the Canvas objects
Comment 17 Artur Moryc 2014-02-05 07:37:39 PST
Any comments from reviewers?
Comment 18 chris fleizach 2014-02-05 08:57:15 PST
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
Comment 19 chris fleizach 2014-02-07 09:22:20 PST
Comment on attachment 218839 [details]
Updating solution for children nodes of the Canvas objects

r- for nitpicking spelling error. Please update patch when possible
Comment 20 Radar WebKit Bug Importer 2014-02-07 09:22:58 PST
<rdar://problem/16012354>
Comment 21 Artur Moryc 2014-02-17 08:42:54 PST
Created attachment 224376 [details]
Updating solution for children nodes of the Canvas objects

Thanks for previous comments!
Comment 22 WebKit Commit Bot 2014-02-20 02:20:16 PST
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>
Comment 23 WebKit Commit Bot 2014-02-20 02:20:21 PST
All reviewed patches have been landed.  Closing bug.