AX: Support embedded SVG objects in AX tree

[chris fleizach]

SVG is already accessible with VoiceOver if you view it directly, but you can't interact with it when it's embedded inside HTML content. That's because the elements are not exposed in any form.

[chris fleizach]

rdar://8984887

[chris fleizach]

Created attachment 165805 [details]
patch

[chris fleizach]

Created attachment 165812 [details]
patch

[Dominic Mazzoni]

Comment on attachment 165812 [details]
patch

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

Unofficial review.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:810
> +            rect.setX(rect.x() + parentBoundingBox.x());

Use rect.move() or rect.moveBy().

Break from the loop after finding a RenderSVGRoot, because I don't think there could be more than one in the parent chain, right?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:838
> +    offsetBoundingBoxForRemoteSVGElement(result);

Can you avoid calling this method if you don't have an SVG element to begin with? Or is there any way you could short-circuit this method's inner loop so that it doesn't need to walk up the entire parent chain for every non-SVG element?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2421
> +            return GroupRole;

How about adding a new role SVGRole and mapping it to the same as GroupRole on all platforms for now? I think it's possible that some platforms might want to do something with an SVG root, similar to canvas.

That may require that you specifically add SVGRole to a few checks in the code that currently handle GroupRole (maybe accessibilityIsIgnored?) but I'd rather it be explicit.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2756
> +    SVGSVGElement* rootElement = static_cast<SVGDocument*>(frame->document())->rootElement();

Test isSVGDocument on frame->document() just to be safe.

> Source/WebCore/accessibility/AccessibilityRenderSVGRoot.h:36
> +class AccessibilityRenderSVGRoot : public AccessibilityRenderObject {

Should this just be AccessibilitySVGRoot?

Several other Accessibility classes inherit from AccessibilityRenderObject, but they don't have Render in their names.

> LayoutTests/ChangeLog:8
> +        Test skipped on chromium until clickPoint() is implementing in DRT.

implementing -> implemented

[Build Bot]

Comment on attachment 165812 [details]
patch

Attachment 165812 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14030522

[Gyuyoung Kim]

Comment on attachment 165812 [details]
patch

Attachment 165812 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14019583

[chris fleizach]

Created attachment 165856 [details]
patch

made all changes dominic suggested

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 165856 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1
Source/WebCore/accessibility/AccessibilityAllInOne.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
Source/WebCore/accessibility/AXObjectCache.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominic Mazzoni]

Comment on attachment 165856 [details]
patch

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

Unofficial review looks good

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2171
> +    IntPoint offsetPoint(point.x() - parentRect.location().x(), point.y() - parentRect.location().y());

You should be able to do something more like:

IntPoint offsetPoint = point - parentRect.location();

[chris fleizach]

Created attachment 165885 [details]
patch

[chris fleizach]

Adding Eric to help comment about SVG usage

[Gyuyoung Kim]

Comment on attachment 165885 [details]
patch

Attachment 165885 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14037574

[Early Warning System Bot]

Comment on attachment 165885 [details]
patch

Attachment 165885 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14043286

[Early Warning System Bot]

Comment on attachment 165885 [details]
patch

Attachment 165885 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14027671

[WebKit Review Bot]

Comment on attachment 165885 [details]
patch

Attachment 165885 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14033575

[chris fleizach]

Created attachment 165887 [details]
patch

[Eric Seidel (no email)]

pdr and schenny (despite not yet being reviewers) are way more up-to-date on SVG issues than I am these days. :)  But I'll take a look regardless!

[chris fleizach]

Created attachment 165888 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 165887 [details]
patch

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

The patch seems OK... I'm not sure about the security (pointer-lifetimes, or x-orgin access) aspects.

This seems very last-mile to me for AX.  Is there some high-profile site driving this? My impression is that embedding SVGs in <img> tags is a relatively uncommon way to embed them.  But my impresion could be wrong.  As I mention below, this feels very akin to OCRing text out of bitmap images and exposing it to AX.  If that's on the eventual goal list, then this is definitely in-line with such. :)

> Source/WebCore/ChangeLog:10
> +        This patch allows an SVG image from another resource to be hooked into the AX
> +        hierarchy. This is done by creating an AX wrapper for the root SVG that holds onto
> +        its native parent image. The SVGChromeClient is then used to connect to this SVG resource.

is the AX hierarchy ever exposed to web pages?  If so, I would worry about possible x-origin exploits here?  Or does AX follow the same-origin policy (I kinda doubt it would, as it would mean you couldn't "see" into x-origin images.)

> Source/WebCore/accessibility/AccessibilitySVGRoot.cpp:38
> +    : AccessibilityRenderObject(renderer),
> +    m_parent(0)

I'm surprised the style-bot didn't catch the , on the "wrong" line.  I believe our style is to lead with the , or : so as to make each line "independent" of the others for easy removal/addition.

> Source/WebCore/accessibility/AccessibilitySVGRoot.cpp:50
> +    AccessibilitySVGRoot* obj = new AccessibilitySVGRoot(renderer);
> +    obj->init();
> +    return adoptRef(obj);

Could alternatively just put it into the RefPtr immediately and then .release() it.

> Source/WebCore/accessibility/AccessibilitySVGRoot.cpp:58
> +    // If a parent was set because this is a remote SVG resource, use that
> +    // but otherwise, we should rely on the standard render tree for the parent.
> +    if (m_parent)
> +        return m_parent;

I'm not sure what this does yet... but presumably it will be more clear by the end of the patch. :)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1472
> +        // If we have an empty chrome client (like SVG) then we should use the page
> +        // of the scroll view parent to help us get to the screen rect.
> +        if (parent && page && page->chrome()->client()->isEmptyChromeClient())
> +            page = parent->page();

I see.  So part of this patch is making navigation inside one of these resources behave reasonably?  (Which might otherwise be difficult since it has empty clients?)

Do we need any of this for iframes/seamless-iframes? or pdf-as-images?  I guess the iframe case "just works" because it has real clients/has a Document/Page and the pdf case we don't care about/is-safari-only?

> Source/WebCore/accessibility/AccessibilityRenderObject.h:254
> +    bool isSVGImage() const;

It feels a bit odd to even bother supporting SVGImages for AX, as they're "images" and thus not really accessible, or?    I guess we'd happily OCR text in bitmap images for AX if we could?  As this seems similar to what we're doing here.  If an author wants an interactive SVG they need to embed it directly (via a sandboxed iframe or directly-in-the-document if they don't care about security).

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2424
> +        if (isSVGImage())
> +            return SVGRootRole;

Is this the same role as when you embed <svg> directly in your html document?  (should it be?)

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2757
> +    FrameView* frameView = svgImage->frameView();
> +    if (!frameView)
> +        return 0;
> +    Frame* frame = frameView->frame();
> +    if (!frame)
> +        return 0;

Aren't there helpers to get to the RenderView directly from the FrameView?  The method you chose definitely works.  I just wonder if there si something more concise we should use.

> Source/WebCore/svg/graphics/SVGImageChromeClient.h:42
> +    : m_image(image)

I think our style says to indent this.

[Build Bot]

Comment on attachment 165887 [details]
patch

Attachment 165887 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14032606

[Peter Beverloo (cr-android ews)]

Comment on attachment 165887 [details]
patch

Attachment 165887 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14037590

[chris fleizach]

(In reply to comment #20)

Thanks for the feedback!

> (From update of attachment 165887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165887&action=review
> 
> The patch seems OK... I'm not sure about the security (pointer-lifetimes, or x-orgin access) aspects.
> 
> This seems very last-mile to me for AX.  Is there some high-profile site driving this? My impression is that embedding SVGs in <img> tags is a relatively uncommon way to embed them.  But my impresion could be wrong. 

This is a priority for us.

> As I mention below, this feels very akin to OCRing text out of bitmap images and exposing it to AX.  If that's on the eventual goal list, then this is definitely in-line with such. :)

SVG is different in that each individual component can be made accessible. So if you had a map of Africa for example, you could make each country and individual element and provide information about it. With bitmap graphics you'd have to go OCR routes to get text and it would be just be impossible for anything beyond that

> 
> > Source/WebCore/ChangeLog:10
> > +        This patch allows an SVG image from another resource to be hooked into the AX
> > +        hierarchy. This is done by creating an AX wrapper for the root SVG that holds onto
> > +        its native parent image. The SVGChromeClient is then used to connect to this SVG resource.
> 
> is the AX hierarchy ever exposed to web pages?  If so, I would worry about possible x-origin exploits here?  Or does AX follow the same-origin policy (I kinda doubt it would, as it would mean you couldn't "see" into x-origin images.)

The AX hierarchy is not exposed to web pages, it's only exposed to AX app clients, so I think we're ok. we're taking what's already stored in memory of the SVG document and turning it into AX nodes.

> 
> > Source/WebCore/accessibility/AccessibilitySVGRoot.cpp:38
> > +    : AccessibilityRenderObject(renderer),
> > +    m_parent(0)
> 
> I'm surprised the style-bot didn't catch the , on the "wrong" line.  I believe our style is to lead with the , or : so as to make each line "independent" of the others for easy removal/addition.
> 
> > Source/WebCore/accessibility/AccessibilitySVGRoot.cpp:50
> > +    AccessibilitySVGRoot* obj = new AccessibilitySVGRoot(renderer);
> > +    obj->init();
> > +    return adoptRef(obj);
> 
> Could alternatively just put it into the RefPtr immediately and then .release() it.
> 
> > Source/WebCore/accessibility/AccessibilitySVGRoot.cpp:58
> > +    // If a parent was set because this is a remote SVG resource, use that
> > +    // but otherwise, we should rely on the standard render tree for the parent.
> > +    if (m_parent)
> > +        return m_parent;
> 
> I'm not sure what this does yet... but presumably it will be more clear by the end of the patch. :)
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1472
> > +        // If we have an empty chrome client (like SVG) then we should use the page
> > +        // of the scroll view parent to help us get to the screen rect.
> > +        if (parent && page && page->chrome()->client()->isEmptyChromeClient())
> > +            page = parent->page();
> 
> I see.  So part of this patch is making navigation inside one of these resources behave reasonably?  (Which might otherwise be difficult since it has empty clients?)

Correct

> 
> Do we need any of this for iframes/seamless-iframes? or pdf-as-images?  I guess the iframe case "just works" because it has real clients/has a Document/Page and the pdf case we don't care about/is-safari-only?

iframes and pdfs should work already. both of those are handled in slightly different ways which are a tad easier to implement in that they have natural parent child connections already set up

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.h:254
> > +    bool isSVGImage() const;
> 
> It feels a bit odd to even bother supporting SVGImages for AX, as they're "images" and thus not really accessible, or?    I guess we'd happily OCR text in bitmap images for AX if we could?  As this seems similar to what we're doing here.  If an author wants an interactive SVG they need to embed it directly (via a sandboxed iframe or directly-in-the-document if they don't care about security).
> 

Hopefully my response above addresses your concern here.

> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2424
> > +        if (isSVGImage())
> > +            return SVGRootRole;
> 
> Is this the same role as when you embed <svg> directly in your html document?  (should it be?)
> 

Good question. Right now it's not, but I can make that change, I think it makes sense

> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2757
> > +    FrameView* frameView = svgImage->frameView();
> > +    if (!frameView)
> > +        return 0;
> > +    Frame* frame = frameView->frame();
> > +    if (!frame)
> > +        return 0;
> 
> Aren't there helpers to get to the RenderView directly from the FrameView?  The method you chose definitely works.  I just wonder if there si something more concise we should use.
> 

will look into it

> > Source/WebCore/svg/graphics/SVGImageChromeClient.h:42
> > +    : m_image(image)
> 
> I think our style says to indent this.

[Dirk Schulze]

Eric is right. If your concerns are accessibility related, definitely don't embed SVG with <img> but use inline or <object>. Just as a hint.

[Build Bot]

Comment on attachment 165888 [details]
patch

Attachment 165888 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14050128

New failing tests:
accessibility/svg-remote-element.html

[chris fleizach]

(In reply to comment #24)
> Eric is right. If your concerns are accessibility related, definitely don't embed SVG with <img> but use inline or <object>. Just as a hint.

Just to point out, I am not advocating anyone to follow this approach, I just need to make sure it's accessible if someone does it

[chris fleizach]

Created attachment 166107 [details]
patch addressing Eric's comments

[WebKit Review Bot]

Comment on attachment 166107 [details]
patch addressing Eric's comments

Attachment 166107 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14036995

[Peter Beverloo (cr-android ews)]

Comment on attachment 166107 [details]
patch addressing Eric's comments

Attachment 166107 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14059104

[Build Bot]

Comment on attachment 166107 [details]
patch addressing Eric's comments

Attachment 166107 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14043767

New failing tests:
accessibility/svg-bounds.html

[chris fleizach]

Created attachment 166614 [details]
patch

[Tim Horton]

Comment on attachment 166614 [details]
patch

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

> Source/WebCore/ChangeLog:67
> +        (WebCore::toSVGImageChromeClient):

This is the best place for per-function documentation, it’s a shame not to use it for a relatively big patch like this!

> Source/WebCore/accessibility/AccessibilityRenderObject.h:252
>      // This returns true if it's focusable but it's not content editable and it's not a control or ARIA control.

Did this comment at some point get detached from the line it refers to?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:839
> +    Document* doc = document();

Would prefer to spell out document, but there seem to be lots of cases of this abbreviation.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2744
> +    RenderImage* renderImage = toRenderImage(m_renderer);

Can we fold this into the next line?

> Source/WebCore/accessibility/AccessibilitySVGRoot.h:29
> +#ifndef AccessibilitySVGRoot_h

This file, and the rest of the Accessibility files (yay, consistency, I guess?), is in a weird order. The majority of WebCore classes go public/protected/private and put members below methods. Maybe there’s no real rule about this.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1229
> +        return String();

Does it really not want a title at all?

> Source/WebCore/svg/graphics/SVGImageChromeClient.h:72
> +

If you had per-function comments in your change log, it would be more clear what changed here :)

> LayoutTests/accessibility/svg-bounds.html:8
> -<svg>
> +<svg tabindex=0>

This change is not mentioned in your change log and I don’t understand it.

> LayoutTests/accessibility/resources/svg-face.svg:1
> +<svg xmlns:cc="http://web.resource.org/cc/" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns="http://www.w3.org/2000/svg" xmlns:svg="http://www.w3.org/2000/svg"

So many unnecessary namespaces?

> LayoutTests/ChangeLog:8
> +        Test skipped on chromium until clickPoint() is implemented in DRT.

Probably add one more line here about what your new test tests! Though that’s pretty clear, I guess.

[chris fleizach]

Thanks for the review. I'll make all suggested changes except fixing the weird public/protected/private ordering in AX. That would probably be best left for another patch

(In reply to comment #32)
> (From update of attachment 166614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166614&action=review
> 
> > Source/WebCore/ChangeLog:67
> > +        (WebCore::toSVGImageChromeClient):
> 
> This is the best place for per-function documentation, it’s a shame not to use it for a relatively big patch like this!
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.h:252
> >      // This returns true if it's focusable but it's not content editable and it's not a control or ARIA control.
> 
> Did this comment at some point get detached from the line it refers to?
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:839
> > +    Document* doc = document();
> 
> Would prefer to spell out document, but there seem to be lots of cases of this abbreviation.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2744
> > +    RenderImage* renderImage = toRenderImage(m_renderer);
> 
> Can we fold this into the next line?
> 
> > Source/WebCore/accessibility/AccessibilitySVGRoot.h:29
> > +#ifndef AccessibilitySVGRoot_h
> 
> This file, and the rest of the Accessibility files (yay, consistency, I guess?), is in a weird order. The majority of WebCore classes go public/protected/private and put members below methods. Maybe there’s no real rule about this.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1229
> > +        return String();
> 
> Does it really not want a title at all?
> 
> > Source/WebCore/svg/graphics/SVGImageChromeClient.h:72
> > +
> 
> If you had per-function comments in your change log, it would be more clear what changed here :)
> 
> > LayoutTests/accessibility/svg-bounds.html:8
> > -<svg>
> > +<svg tabindex=0>
> 
> This change is not mentioned in your change log and I don’t understand it.
> 
> > LayoutTests/accessibility/resources/svg-face.svg:1
> > +<svg xmlns:cc="http://web.resource.org/cc/" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns="http://www.w3.org/2000/svg" xmlns:svg="http://www.w3.org/2000/svg"
> 
> So many unnecessary namespaces?
> 
> > LayoutTests/ChangeLog:8
> > +        Test skipped on chromium until clickPoint() is implemented in DRT.
> 
> Probably add one more line here about what your new test tests! Though that’s pretty clear, I guess.

[chris fleizach]

http://trac.webkit.org/changeset/132915