Bug 96168

Summary: WebKit exposes incorrect bounds for embedded SVG in HTML
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, dglazkov, dmazzoni, eric, fmalita, krit, pdr, schenney, simon.fraser, thorton, webkit.review.bot, zimmermann, zourtney
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch for testing
webkit-ews: commit-queue-
patch
krit: review-
patch
eric: review-
patch
webkit.review.bot: commit-queue-
patch
eric: review+, webkit.review.bot: commit-queue-
patch for landing none

Description chris fleizach 2012-09-07 17:43:40 PDT
WebKit exposes incorrect bounds for embedded SVG in HTML
Comment 1 chris fleizach 2012-09-07 17:43:56 PDT
rdar://12225992
Comment 2 chris fleizach 2012-09-07 17:44:38 PDT
It looks like the problem is that the focusRing methods we rely on to return a bounding box are all in local coordinates for SVG elements.
Comment 3 chris fleizach 2012-09-07 22:50:59 PDT
Created attachment 162943 [details]
patch for testing
Comment 4 Early Warning System Bot 2012-09-07 23:06:38 PDT
Comment on attachment 162943 [details]
patch for testing

Attachment 162943 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13785842
Comment 5 Early Warning System Bot 2012-09-07 23:06:50 PDT
Comment on attachment 162943 [details]
patch for testing

Attachment 162943 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13795168
Comment 6 Build Bot 2012-09-07 23:09:07 PDT
Comment on attachment 162943 [details]
patch for testing

Attachment 162943 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13788505
Comment 7 Build Bot 2012-09-07 23:15:09 PDT
Comment on attachment 162943 [details]
patch for testing

Attachment 162943 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13798115
Comment 8 chris fleizach 2012-09-07 23:21:39 PDT
Created attachment 162946 [details]
patch
Comment 9 Alexey Proskuryakov 2012-09-08 19:37:09 PDT
The new patch doesn't apply to ToT any more, so EWS is not building it. Could you please upload a patch made against an updated tree?
Comment 10 Dirk Schulze 2012-09-08 22:09:51 PDT
Comment on attachment 162946 [details]
patch 

Not all elements inherit from RenderSVGModelObject, so I doubt that this extra check will help you a lot (SVGImage or SVG*Text* as example). Something that we usually do is checking the elements type (node()->isSVGElement()). You need to be carful with inline SVG elements so:

<body>
<svg>
...
</svg>
</body>

These elements are treated like HTML elements, but still return true for isSVGElement().

Can you add examples with <text> and <image> as well please?
Comment 11 chris fleizach 2012-09-10 17:37:52 PDT
(In reply to comment #10)
> (From update of attachment 162946 [details])
> Not all elements inherit from RenderSVGModelObject, so I doubt that this extra check will help you a lot (SVGImage or SVG*Text* as example). Something that we usually do is checking the elements type (node()->isSVGElement()). You need to be carful with inline SVG elements so:
> 
> <body>
> <svg>
> ...
> </svg>
> </body>
> 
> These elements are treated like HTML elements, but still return true for isSVGElement().
> 
> Can you add examples with <text> and <image> as well please?

Hi Dirk,

Thanks for your feedback. I just tested text and images for SVG

Text works correctly already. It looks like it doesn't do the same "draw the focus ring itself and store in local coordinates" thing

Images are not accessible at all for some reason. I filed
https://bugs.webkit.org/show_bug.cgi?id=96341

to figure that one out. I will update the test to include text in it, but do you think this approach is OK

It looks like if i just do the node() check, then <text> will actually be incorrect.
Comment 12 chris fleizach 2012-09-20 11:07:02 PDT
*** Bug 66095 has been marked as a duplicate of this bug. ***
Comment 13 chris fleizach 2012-09-20 11:09:42 PDT
Created attachment 164950 [details]
patch
Comment 14 Eric Seidel (no email) 2012-09-20 11:18:45 PDT
Comment on attachment 164950 [details]
patch 

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:817
>      else
>          obj->absoluteFocusRingQuads(quads);

Are you suggesting absoluteFocusRingQuads gives a non-absolute answer for SVG?  That seems like a bug if true...
Comment 15 chris fleizach 2012-09-20 11:20:40 PDT
Comment on attachment 164950 [details]
patch 

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

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:817
>>          obj->absoluteFocusRingQuads(quads);
> 
> Are you suggesting absoluteFocusRingQuads gives a non-absolute answer for SVG?  That seems like a bug if true...

Yep...


RenderSVGShape.cpp
// This method is called from inside paintOutline() since we call paintOutline()
// while transformed to our coord system, return local coords
void RenderSVGShape::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint&)
Comment 16 Eric Seidel (no email) 2012-09-20 11:25:08 PDT
Comment on attachment 164950 [details]
patch 

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

>>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:817
>>>          obj->absoluteFocusRingQuads(quads);
>> 
>> Are you suggesting absoluteFocusRingQuads gives a non-absolute answer for SVG?  That seems like a bug if true...
> 
> Yep...
> 
> 
> RenderSVGShape.cpp
> // This method is called from inside paintOutline() since we call paintOutline()
> // while transformed to our coord system, return local coords
> void RenderSVGShape::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint&)

But that's addFocusRingRects, not absoluteFocusRingQuads.  I guess SVG needs to override absoluteFocusRingQuads to transform them to absolute if absoluteFocusRingQuads expects addFocusRingRects to be absolute?

Because SVG elements can have arbitrary transforms between elements (and we don't have a layer tree to help), everything is done in local coordinates and then only turned to absolute when necessary.  I'm sure RenderLayer functions similarly, (even though our old HTML renderers operate in an odd containing-block-relative world). :)
Comment 17 Eric Seidel (no email) 2012-09-20 11:26:27 PDT
Comment on attachment 164950 [details]
patch 

I agree this seems like a potential bug.  This seems like the wrong fix to me.
Comment 18 chris fleizach 2012-09-20 11:27:58 PDT
(In reply to comment #16)
> (From update of attachment 164950 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164950&action=review
> 
> >>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:817
> >>>          obj->absoluteFocusRingQuads(quads);
> >> 
> >> Are you suggesting absoluteFocusRingQuads gives a non-absolute answer for SVG?  That seems like a bug if true...
> > 
> > Yep...
> > 
> > 
> > RenderSVGShape.cpp
> > // This method is called from inside paintOutline() since we call paintOutline()
> > // while transformed to our coord system, return local coords
> > void RenderSVGShape::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint&)
> 
> But that's addFocusRingRects, not absoluteFocusRingQuads.  I guess SVG needs to override absoluteFocusRingQuads to transform them to absolute if absoluteFocusRingQuads expects addFocusRingRects to be absolute?
> 
> Because SVG elements can have arbitrary transforms between elements (and we don't have a layer tree to help), everything is done in local coordinates and then only turned to absolute when necessary.  I'm sure RenderLayer functions similarly, (even though our old HTML renderers operate in an odd containing-block-relative world). :)

In absoluteFocusRingQuads, it calls addFocusRingRects...

void RenderObject::absoluteFocusRingQuads(Vector<FloatQuad>& quads)
{
    ....
    addFocusRingRects(rects, flooredLayoutPoint(absolutePoint));


It sounds like this patch should have an RenderSVGShape override of absoluteFocusRingQuads and absoluteFocusRingRects?
Comment 19 Stephen Chenney 2012-09-20 11:50:26 PDT
Comment on attachment 164950 [details]
patch 

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

>>>>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:817
>>>>>          obj->absoluteFocusRingQuads(quads);
>>>> 
>>>> Are you suggesting absoluteFocusRingQuads gives a non-absolute answer for SVG?  That seems like a bug if true...
>>> 
>>> Yep...
>>> 
>>> 
>>> RenderSVGShape.cpp
>>> // This method is called from inside paintOutline() since we call paintOutline()
>>> // while transformed to our coord system, return local coords
>>> void RenderSVGShape::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint&)
>> 
>> But that's addFocusRingRects, not absoluteFocusRingQuads.  I guess SVG needs to override absoluteFocusRingQuads to transform them to absolute if absoluteFocusRingQuads expects addFocusRingRects to be absolute?
>> 
>> Because SVG elements can have arbitrary transforms between elements (and we don't have a layer tree to help), everything is done in local coordinates and then only turned to absolute when necessary.  I'm sure RenderLayer functions similarly, (even though our old HTML renderers operate in an odd containing-block-relative world). :)
> 
> In absoluteFocusRingQuads, it calls addFocusRingRects...
> 
> void RenderObject::absoluteFocusRingQuads(Vector<FloatQuad>& quads)
> {
>     ....
>     addFocusRingRects(rects, flooredLayoutPoint(absolutePoint));
> 
> 
> It sounds like this patch should have an RenderSVGShape override of absoluteFocusRingQuads and absoluteFocusRingRects?

That would be better than adding the code you have here, yes. Let us know if you need help with that.

> LayoutTests/accessibility/svg-bounds.html:16
> +  <image x="20" y="20" width="300" height="80" aria-label="Test Image" xlink:href="resources/cake.ong" />

I presume you mean cake.png? Does this mean the issues you were having with images are not real issues?

> LayoutTests/platform/mac/accessibility/svg-bounds-expected.txt:1
> +Test

Do you expect this test to be platform specific? It doesn't seem so, and hence the results should go in LayoutTests/accessibility/svg-bounds-expected.txt
Comment 20 chris fleizach 2012-09-20 11:51:48 PDT
(In reply to comment #19)
> (From update of attachment 164950 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164950&action=review
> 
> >>>>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:817
> >>>>>          obj->absoluteFocusRingQuads(quads);
> >>>> 
> >>>> Are you suggesting absoluteFocusRingQuads gives a non-absolute answer for SVG?  That seems like a bug if true...
> >>> 
> >>> Yep...
> >>> 
> >>> 
> >>> RenderSVGShape.cpp
> >>> // This method is called from inside paintOutline() since we call paintOutline()
> >>> // while transformed to our coord system, return local coords
> >>> void RenderSVGShape::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint&)
> >> 
> >> But that's addFocusRingRects, not absoluteFocusRingQuads.  I guess SVG needs to override absoluteFocusRingQuads to transform them to absolute if absoluteFocusRingQuads expects addFocusRingRects to be absolute?
> >> 
> >> Because SVG elements can have arbitrary transforms between elements (and we don't have a layer tree to help), everything is done in local coordinates and then only turned to absolute when necessary.  I'm sure RenderLayer functions similarly, (even though our old HTML renderers operate in an odd containing-block-relative world). :)
> > 
> > In absoluteFocusRingQuads, it calls addFocusRingRects...
> > 
> > void RenderObject::absoluteFocusRingQuads(Vector<FloatQuad>& quads)
> > {
> >     ....
> >     addFocusRingRects(rects, flooredLayoutPoint(absolutePoint));
> > 
> > 
> > It sounds like this patch should have an RenderSVGShape override of absoluteFocusRingQuads and absoluteFocusRingRects?
> 
> That would be better than adding the code you have here, yes. Let us know if you need help with that.
> 
> > LayoutTests/accessibility/svg-bounds.html:16
> > +  <image x="20" y="20" width="300" height="80" aria-label="Test Image" xlink:href="resources/cake.ong" />
> 
> I presume you mean cake.png? Does this mean the issues you were having with images are not real issues?

Yes that should be cake.png but in reality it doesn't matter. An image still exists in the tree with the correct width and height.

> 
> > LayoutTests/platform/mac/accessibility/svg-bounds-expected.txt:1
> > +Test
> 
> Do you expect this test to be platform specific? It doesn't seem so, and hence the results should go in LayoutTests/accessibility/svg-bounds-expected.txt

The results will be platform specific, specifically the way that role's and labels are outputted are different
Comment 21 chris fleizach 2012-09-21 00:11:40 PDT
Created attachment 165060 [details]
patch
Comment 22 chris fleizach 2012-09-21 00:12:37 PDT
Comment on attachment 165060 [details]
patch 

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

> Source/WebCore/rendering/RenderObject.h:727
> +    virtual void absoluteFocusRingQuads(Vector<FloatQuad>&);

it looks like accessibility code is the only client of this method, so probably not a problem to make it virtual
Comment 23 WebKit Review Bot 2012-09-21 03:23:08 PDT
Comment on attachment 165060 [details]
patch 

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

New failing tests:
accessibility/svg-bounds.html
Comment 24 chris fleizach 2012-09-21 09:08:34 PDT
Created attachment 165144 [details]
patch 

Change the layout test to rely less on shouldBe so it can be platform independent
Comment 25 Eric Seidel (no email) 2012-09-21 09:40:43 PDT
Comment on attachment 165144 [details]
patch 

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

> Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:169
> +void RenderSVGModelObject::absoluteFocusRingQuads(Vector<FloatQuad>& quads)

FYI. Almost all SVG objects are SVGModelObjects, except RenderSVGBlock (for <text>, <tspan> etc.) and RenderForeignObject.  If you worry about those bounds, you may need to test them as well in your test case to be sure.
Comment 26 Eric Seidel (no email) 2012-09-21 09:42:41 PDT
(In reply to comment #22)
> (From update of attachment 165060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165060&action=review
> 
> > Source/WebCore/rendering/RenderObject.h:727
> > +    virtual void absoluteFocusRingQuads(Vector<FloatQuad>&);
> 
> it looks like accessibility code is the only client of this method, so probably not a problem to make it virtual

If that's true, we might consider renaming it to involve AX?  I know SVG has some focus-ring bugs:
http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/svg/custom/focus-ring-expected.png
Comment 27 WebKit Review Bot 2012-09-21 10:14:29 PDT
Comment on attachment 165144 [details]
patch 

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

New failing tests:
accessibility/svg-bounds.html
Comment 28 chris fleizach 2012-09-21 10:54:54 PDT
Comment on attachment 165144 [details]
patch 

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

>> Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:169
>> +void RenderSVGModelObject::absoluteFocusRingQuads(Vector<FloatQuad>& quads)
> 
> FYI. Almost all SVG objects are SVGModelObjects, except RenderSVGBlock (for <text>, <tspan> etc.) and RenderForeignObject.  If you worry about those bounds, you may need to test them as well in your test case to be sure.

I did add a test for <text> in this layout test, which seemed to already be working correctly, so I think we're OK on that front
Comment 29 WebKit Review Bot 2012-09-21 11:13:16 PDT
Comment on attachment 165144 [details]
patch 

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

New failing tests:
accessibility/svg-bounds.html
Comment 30 Dominic Mazzoni 2012-09-21 12:38:04 PDT
Go ahead and skip this test on Chromium for now. Looks like there are two issues:

1. Chromium is ignoring different objects, so all of the childAtIndex calculations are not working. Switching to the new accessibleElementById should help a lot. In the meantime, would it work to make each svg element focusable and focus them in order to get their accessible object? If so, the test will be a little less brittle...

2. I haven't implemented clickPointX and clickPointY on Chromium. Shouldn't be hard, but I just haven't had a chance. I'll get around to fixing it as soon as I have a chance.

If you can do #1 without much trouble before committing, that'd be great - but either way I'll take care of implementing clickPoint[X,Y] and fixing it later.
Comment 31 chris fleizach 2012-09-21 13:42:35 PDT
(In reply to comment #30)
> Go ahead and skip this test on Chromium for now. Looks like there are two issues:
> 
> 1. Chromium is ignoring different objects, so all of the childAtIndex calculations are not working. Switching to the new accessibleElementById should help a lot. In the meantime, would it work to make each svg element focusable and focus them in order to get their accessible object? If so, the test will be a little less brittle...
> 
> 2. I haven't implemented clickPointX and clickPointY on Chromium. Shouldn't be hard, but I just haven't had a chance. I'll get around to fixing it as soon as I have a chance.
> 
> If you can do #1 without much trouble before committing, that'd be great - but either way I'll take care of implementing clickPoint[X,Y] and fixing it later.

Thanks. I think I'll wait until we have our new method since we'll want to move a lot of tests over I imagine
Comment 32 chris fleizach 2012-09-21 13:57:28 PDT
Created attachment 165185 [details]
patch for landing
Comment 33 chris fleizach 2012-09-21 14:27:19 PDT
http://trac.webkit.org/changeset/129254