Bug 113666 – SVG outline property is broken and inefficient

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

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!

[Florin Malita]

(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.

[Philip Rogers]

(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.

[Florin Malita]

(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.

[Florin Malita]

Created attachment 195996 [details]
Patch

[Dirk Schulze]

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?

[Florin Malita]

(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! :)

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Philip Rogers]

(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.

[Dirk Schulze]

(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.

[Florin Malita]

(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?

[Florin Malita]

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.

[Dirk Schulze]

Created attachment 231312 [details]
Patch from dino - rebaselined 

Patch

[Dirk Schulze]

Ha, auto completion. s/Patch from dino - rebaselined/Patch/

[Dean Jackson]

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.

[Dirk Schulze]

Created attachment 231317 [details]
Patch for landing

[WebKit Commit Bot]

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

[Dirk Schulze]

Created attachment 231319 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 231319 [details]
Patch for landing

Clearing flags on attachment: 231319

Committed r168645: <http://trac.webkit.org/changeset/168645>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.