Bug 96168

Summary:

WebKit exposes incorrect bounds for embedded SVG in HTML

Product:

WebKit

Reporter:

chris fleizach <cfleizach>

Component:

Accessibility

Assignee:

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:

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

chris fleizach

Reported 2012-09-07 17:43:40 PDT

WebKit exposes incorrect bounds for embedded SVG in HTML

Attachments
patch for testing (6.31 KB, patch) 2012-09-07 22:50 PDT, chris fleizach	webkit-ews: commit-queue-	Details Formatted Diff Diff
patch (7.70 KB, patch) 2012-09-07 23:21 PDT, chris fleizach	krit: review-	Details Formatted Diff Diff
patch (8.55 KB, patch) 2012-09-20 11:09 PDT, chris fleizach	eric: review-	Details Formatted Diff Diff
patch (8.78 KB, patch) 2012-09-21 00:11 PDT, chris fleizach	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
patch (8.51 KB, patch) 2012-09-21 09:08 PDT, chris fleizach	eric: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
patch for landing (9.15 KB, patch) 2012-09-21 13:57 PDT, chris fleizach	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

chris fleizach

Comment 1 2012-09-07 17:43:56 PDT

rdar://12225992

chris fleizach

Comment 2 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.

chris fleizach

Comment 3 2012-09-07 22:50:59 PDT

Created attachment 162943 [details] patch for testing

Early Warning System Bot

Comment 4 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

Early Warning System Bot

Comment 5 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

Build Bot

Comment 6 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

Build Bot

Comment 7 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

chris fleizach

Comment 8 2012-09-07 23:21:39 PDT

Created attachment 162946 [details] patch

Alexey Proskuryakov

Comment 9 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?

Dirk Schulze

Comment 10 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?

chris fleizach

Comment 11 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.

chris fleizach

Comment 12 2012-09-20 11:07:02 PDT

*** Bug 66095 has been marked as a duplicate of this bug. ***

chris fleizach

Comment 13 2012-09-20 11:09:42 PDT

Created attachment 164950 [details] patch

Eric Seidel (no email)

Comment 14 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...

chris fleizach

Comment 15 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&)

Eric Seidel (no email)

Comment 16 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). :)

Eric Seidel (no email)

Comment 17 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.

chris fleizach

Comment 18 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?

Stephen Chenney

Comment 19 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

chris fleizach

Comment 20 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

chris fleizach

Comment 21 2012-09-21 00:11:40 PDT

Created attachment 165060 [details] patch

chris fleizach

Comment 22 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

WebKit Review Bot

Comment 23 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

chris fleizach

Comment 24 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

Eric Seidel (no email)

Comment 25 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.

Eric Seidel (no email)

Comment 26 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

WebKit Review Bot

Comment 27 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

chris fleizach

Comment 28 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

WebKit Review Bot

Comment 29 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

Dominic Mazzoni

Comment 30 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.

chris fleizach

Comment 31 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

chris fleizach

Comment 32 2012-09-21 13:57:28 PDT

Created attachment 165185 [details] patch for landing

chris fleizach

Comment 33 2012-09-21 14:27:19 PDT

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

Note You need to log in before you can comment on or make changes to this bug.