Created attachment 195909 [details] Outline testcase WebKit is the only rendering engine that supports outlines on SVG elements: <rect x="0" y="0" width="100" height="300" style="outline: 1px solid #000"/> Furthermore, our implementation is broken as we paint outlines in a separate paintphase so they always render on top of other siblings (see attached: outlineTest.html). Note: Presto does this for HTML divs as well, whereas WebKit, Gecko, and Trident all render HTML div outlines below a later sibling's background. Do we even want to support outlines? In supporting it we are making two passes over the render tree which includes a matrix multiply at each node!
(In reply to comment #0) > Created an attachment (id=195909) [details] > Outline testcase > > WebKit is the only rendering engine that supports outlines on SVG elements: > <rect x="0" y="0" width="100" height="300" style="outline: 1px solid #000"/> > > Furthermore, our implementation is broken as we paint outlines in a separate paintphase so they always render on top of other siblings (see attached: outlineTest.html). Strange as this may sound, I believe we are technically in compliance: 1) the spec seems to intentionally avoid describing the behavior in the presence of overlapping (http://www.w3.org/TR/CSS2/ui.html#propdef-outline): "This specification does not define how multiple overlapping outlines are drawn, or how outlines are drawn for boxes that are partially obscured behind other elements." 2) the stacking context guidelines *encourage* implementations to draw the outline in a separate, last paint phase(http://www.w3.org/TR/CSS21/zindex.html): "10. Finally, implementations that do not draw outlines in steps above must draw outlines from this stacking context at this stage. (It is recommended to draw outlines in this step and not in the steps above.)" 3) it follows that since SVG does not create any new stacking contexts (except for FOs), its outlines will indeed be drawn at the very end. Now whether or not this behavior actually makes sense is a different discussion :) > Note: Presto does this for HTML divs as well, whereas WebKit, Gecko, and Trident all render HTML div outlines below a later sibling's background. I think that's actually a side effect of using separate layers and compositing for positioned elements more than an actual intent. RenderLayer::paintLayerContents() performs the expected recursive paint phases (paintOutlineForFragments() being the last) so there's no specific effort to correctly interleave outlines. > Do we even want to support outlines? In supporting it we are making two passes over the render tree which includes a matrix multiply at each node! It seems like a useful feature and it should be trivial to simply draw outlines during the foreground phase, no? If we all agree that it's the desired SVG behavior, that is.
(In reply to comment #1) Awesome analysis! You're convincing me we should keep outline after all. > It seems like a useful feature and it should be trivial to simply draw outlines during the foreground phase, no? If we all agree that it's the desired SVG behavior, that is. This would be a great change but difficult to do cleanly. I think we would need to include the outline in the object bounding box so occlusion detection works properly.
(In reply to comment #2) > > It seems like a useful feature and it should be trivial to simply draw outlines during the foreground phase, no? If we all agree that it's the desired SVG behavior, that is. > > This would be a great change but difficult to do cleanly. I think we would need to include the outline in the object bounding box so occlusion detection works properly. It may actually work, since clippedOverflowRectForRepaint does include the outline box (see SVGRenderSupport::computeFloatRectForRepaint). Also, it couldn't be any worse than what we have today, since we're not doing anything special for PaintPhaseOutline :) Quick patch for coming for a test drive.
Created attachment 195996 [details] Patch
outline is a poor feature and bad implemented for CSS even on HTML (a lot of problems with other effects in combination). Why do we want to implement a poorly specified property in SVG?
(In reply to comment #5) > outline is a poor feature and bad implemented for CSS even on HTML (a lot of problems with other effects in combination). > > Why do we want to implement a poorly specified property in SVG? I don't feel strongly either way: on one hand being able to quickly visually mark an element seems useful for debugging, but on the other hand I agree that the specification is lacking and our current implementation doesn't make much sense. If removing SVG outline support is actually on the table, then damn the torpedoes! :)
Comment on attachment 195996 [details] Patch Attachment 195996 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17400009 New failing tests: svg/custom/focus-ring.svg
Created attachment 196001 [details] Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
(In reply to comment #5) > outline is a poor feature and bad implemented for CSS even on HTML (a lot of problems with other effects in combination). > > Why do we want to implement a poorly specified property in SVG? Our focus ring support is also handled with the outline code, so we probably don't want to rip it all out right before tabIndex comes to SVG. Because focus rings share all the outline bugs/quirks it might be worth fixing this code up, even though nobody uses outlines.
(In reply to comment #9) > (In reply to comment #5) > > outline is a poor feature and bad implemented for CSS even on HTML (a lot of problems with other effects in combination). > > > > Why do we want to implement a poorly specified property in SVG? > > Our focus ring support is also handled with the outline code, so we probably don't want to rip it all out right before tabIndex comes to SVG. Because focus rings share all the outline bugs/quirks it might be worth fixing this code up, even though nobody uses outlines. Oh, I totally agree. But start with HTML first and use what we have there IMO.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #5) > > > outline is a poor feature and bad implemented for CSS even on HTML (a lot of problems with other effects in combination). > > > > > > Why do we want to implement a poorly specified property in SVG? > > > > Our focus ring support is also handled with the outline code, so we probably don't want to rip it all out right before tabIndex comes to SVG. Because focus rings share all the outline bugs/quirks it might be worth fixing this code up, even though nobody uses outlines. > > Oh, I totally agree. But start with HTML first and use what we have there IMO. Isn't that exactly what we're using today (a dedicated paint phase) and the reason why we're having this discussion (HTML paint phases + stacking contexts don't quite make sense for SVG)? Do you agree that foreground and outline painting should be atomic for SVG?
Created attachment 196018 [details] Patch Fixed SVGText outlines and some clipping issues. Uploading for reference only - it is still unclear whether this is a good direction.
Created attachment 231312 [details] Patch from dino - rebaselined Patch
Ha, auto completion. s/Patch from dino - rebaselined/Patch/
Comment on attachment 231312 [details] Patch from dino - rebaselined View in context: https://bugs.webkit.org/attachment.cgi?id=231312&action=review Lies. This patch is not from me. > LayoutTests/ChangeLog:8 > + Patch by Erik Dahlsroem backported from Blink. Typo in name. > Source/WebCore/ChangeLog:8 > + Patch by Erik Dahlsroem ackported from Blink. Ditto. Except two typos in this one. > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:339 > + // finally, paint the outline if any Nit: period at end of sentence.
Created attachment 231317 [details] Patch for landing
Comment on attachment 231317 [details] Patch for landing Rejecting attachment 231317 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 231317, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/6162278365790208
Created attachment 231319 [details] Patch for landing
Comment on attachment 231319 [details] Patch for landing Clearing flags on attachment: 231319 Committed r168645: <http://trac.webkit.org/changeset/168645>
All reviewed patches have been landed. Closing bug.