Bug 41386 - REGRESSION(58212): html foreignObjects with positions other than static not hidden correctly when parent has display:none
Summary: REGRESSION(58212): html foreignObjects with positions other than static not h...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL: http://maps.afpc.tamu.edu/webkit-fo-h...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-29 18:56 PDT by Rob
Modified: 2012-02-22 06:49 PST (History)
6 users (show)

See Also:


Attachments
test case (531 bytes, image/svg+xml)
2010-06-29 18:56 PDT, Rob
no flags Details
Patch v1 (29.32 KB, patch)
2012-02-17 02:50 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (29.49 KB, patch)
2012-02-22 05:56 PST, Nikolas Zimmermann
zherczeg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob 2010-06-29 18:56:42 PDT
Created attachment 60077 [details]
test case

summary basically says it all, the test case should show a green square with no text. I get FAILED.
Comment 1 Eric Seidel (no email) 2011-12-09 14:34:30 PST
Yup.  Definitely a bug. :(
Comment 2 Eric Seidel (no email) 2011-12-09 14:35:06 PST
Here is a link to the accused regressing revision: http://trac.webkit.org/changeset/58212
Comment 3 Nikolas Zimmermann 2012-02-17 01:23:52 PST
Regression confirmed. The issue is that we create renderers for SVGGElement, that have display: none set. This is needed for constructs like: <g display="none"><linearGradient/> ... If we wouldn't create a renderer for <g>, the <linearGradient/> renderer wouldn't be constructed. For display: none the <g> gets a RenderSVGHiddenContainer renderer, which is like a regular container, but overrides paint() to do nothing.

The problem here is that the <body> in the <fO> still creates inline boxes, which gets renderered, by RenderBlock::paint (trough the <body> renderer). All non-inline objects (direct block children of the <fO>) are treated correctly. Looking into a fix.
Comment 4 Nikolas Zimmermann 2012-02-17 01:53:06 PST
(In reply to comment #3)
> Regression confirmed. The issue is that we create renderers for SVGGElement, that have display: none set. This is needed for constructs like: <g display="none"><linearGradient/> ... If we wouldn't create a renderer for <g>, the <linearGradient/> renderer wouldn't be constructed. For display: none the <g> gets a RenderSVGHiddenContainer renderer, which is like a regular container, but overrides paint() to do nothing.
> 
> The problem here is that the <body> in the <fO> still creates inline boxes, which gets renderered, by RenderBlock::paint (trough the <body> renderer). All non-inline objects (direct block children of the <fO>) are treated correctly. Looking into a fix.

I forgot to mention that the problem doesn't happen with non-relative positioned content in the <body>.
Here's a dump of your test case:
layer at (0,0) size 800x600
  RenderSVGRoot {svg} at (0,0) size 100x90
    RenderSVGHiddenContainer {g} at (0,0) size 0x0
      RenderSVGForeignObject {foreignObject} at (0,0) size 0x0
        RenderBlock {html} at (0,0) size 0x100
    RenderSVGRect {rect} at (0,0) size 100x90 [fill={[type=SOLID] [color=#008000]}] [x=0.00] [y=0.00] [width=100.00] [height=90.00]
layer at (0,0) size 100x100
  RenderBody {body} at (0,0) size 100x100 [color=#FF0000]
    RenderText {#text} at (0,0) size 76x36
      text run at (0,0) width 76: "FAILED IF"
      text run at (0,18) width 62: "VISIBLE"

Removing the position: relative yields:
layer at (0,0) size 800x600
  RenderSVGRoot {svg} at (0,0) size 100x90
    RenderSVGHiddenContainer {g} at (0,0) size 0x0
      RenderSVGForeignObject {foreignObject} at (0,0) size 0x0
        RenderBlock {html} at (0,0) size 0x100
          RenderBody {body} at (0,0) size 100x100 [color=#FF0000]
            RenderText {#text} at (0,0) size 76x36
              text run at (0,0) width 76: "FAILED IF"
              text run at (0,18) width 62: "VISIBLE"
    RenderSVGRect {rect} at (0,0) size 100x90 [fill={[type=SOLID] [color=#008000]}] [x=0.00] [y=0.00] [width=100.00] [height=90.00]

The key problem here is that due position: relative, a new RenderLayer is created for the <body>. Now when the full document paints we have two separated layers, that get painted! The second layer doesn't know that its contained in a hidden SVG subtree, and thus gets painted.

HTML doesn't suffer from problems like this, because it never creates renderers for children, of a display: none object. Anyhow, the bug is fully understood and can be fixed now.
Comment 5 Nikolas Zimmermann 2012-02-17 01:56:20 PST
(In reply to comment #2)
> Here is a link to the accused regressing revision: http://trac.webkit.org/changeset/58212
<quote>
svg/SVGGElement.cpp: If style()->display() is NONE, create a RenderSVGHiddenContainer, removing hacks in SVGStopElement for pservers-grad-19-b.svg (yes, SVG is crazy.)
....
</quote>
Completely correct bisect, this caused the regression with <fO>.
Comment 6 Nikolas Zimmermann 2012-02-17 02:50:27 PST
Created attachment 127564 [details]
Patch v1
Comment 7 WebKit Review Bot 2012-02-17 07:09:56 PST
Comment on attachment 127564 [details]
Patch v1

Attachment 127564 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11542368

New failing tests:
svg/foreignObject/fO-parent-display-changes.svg
Comment 8 Nikolas Zimmermann 2012-02-22 03:08:33 PST
Anyone?
Comment 9 Nikolas Zimmermann 2012-02-22 05:56:35 PST
Created attachment 128190 [details]
Patch v2
Comment 10 Zoltan Herczeg 2012-02-22 06:38:53 PST
Comment on attachment 128190 [details]
Patch v2

r=me
Comment 11 Nikolas Zimmermann 2012-02-22 06:49:37 PST
Committed r108494: <http://trac.webkit.org/changeset/108494>