Bug 113666 - SVG outline property is broken and inefficient
Summary: SVG outline property is broken and inefficient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-31 16:47 PDT by Philip Rogers
Modified: 2014-05-12 14:19 PDT (History)
16 users (show)

See Also:


Attachments
Outline testcase (774 bytes, text/html)
2013-03-31 16:47 PDT, Philip Rogers
no flags Details
Patch (8.91 KB, patch)
2013-04-01 11:01 PDT, Florin Malita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64 (796.13 KB, application/zip)
2013-04-01 12:12 PDT, WebKit Review Bot
no flags Details
Patch (12.65 KB, patch)
2013-04-01 13:41 PDT, Florin Malita
no flags Details | Formatted Diff | Diff
Patch from dino - rebaselined (24.30 KB, patch)
2014-05-12 12:06 PDT, Dirk Schulze
dino: review+
Details | Formatted Diff | Diff
Patch for landing (24.31 KB, patch)
2014-05-12 13:30 PDT, Dirk Schulze
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (24.30 KB, patch)
2014-05-12 13:40 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2013-03-31 16:47:56 PDT
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!
Comment 1 Florin Malita 2013-04-01 08:14:15 PDT
(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.
Comment 2 Philip Rogers 2013-04-01 09:22:05 PDT
(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.
Comment 3 Florin Malita 2013-04-01 11:00:42 PDT
(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.
Comment 4 Florin Malita 2013-04-01 11:01:07 PDT
Created attachment 195996 [details]
Patch
Comment 5 Dirk Schulze 2013-04-01 12:01:26 PDT
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?
Comment 6 Florin Malita 2013-04-01 12:10:57 PDT
(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 7 WebKit Review Bot 2013-04-01 12:12:02 PDT
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
Comment 8 WebKit Review Bot 2013-04-01 12:12:06 PDT
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
Comment 9 Philip Rogers 2013-04-01 12:15:19 PDT
(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.
Comment 10 Dirk Schulze 2013-04-01 13:22:37 PDT
(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.
Comment 11 Florin Malita 2013-04-01 13:29:20 PDT
(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?
Comment 12 Florin Malita 2013-04-01 13:41:31 PDT
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.
Comment 13 Dirk Schulze 2014-05-12 12:06:18 PDT
Created attachment 231312 [details]
Patch from dino - rebaselined 

Patch
Comment 14 Dirk Schulze 2014-05-12 12:12:23 PDT
Ha, auto completion. s/Patch from dino - rebaselined/Patch/
Comment 15 Dean Jackson 2014-05-12 13:14:51 PDT
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.
Comment 16 Dirk Schulze 2014-05-12 13:30:28 PDT
Created attachment 231317 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2014-05-12 13:32:31 PDT
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
Comment 18 Dirk Schulze 2014-05-12 13:40:01 PDT
Created attachment 231319 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2014-05-12 14:18:53 PDT
Comment on attachment 231319 [details]
Patch for landing

Clearing flags on attachment: 231319

Committed r168645: <http://trac.webkit.org/changeset/168645>
Comment 20 WebKit Commit Bot 2014-05-12 14:19:05 PDT
All reviewed patches have been landed.  Closing bug.