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: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
chris fleizach
2012-09-07 17:43:40 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. Created attachment 162943 [details]
patch for testing
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 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 on attachment 162943 [details] patch for testing Attachment 162943 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13788505 Comment on attachment 162943 [details] patch for testing Attachment 162943 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13798115 Created attachment 162946 [details]
patch
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 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?
(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. *** Bug 66095 has been marked as a duplicate of this bug. *** Created attachment 164950 [details]
patch
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 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 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 on attachment 164950 [details]
patch
I agree this seems like a potential bug. This seems like the wrong fix to me.
(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 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 (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 Created attachment 165060 [details]
patch
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 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 Created attachment 165144 [details]
patch
Change the layout test to rely less on shouldBe so it can be platform independent
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. (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 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 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 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 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. (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 Created attachment 165185 [details]
patch for landing
|