Bug 41386

Summary: REGRESSION(58212): html foreignObjects with positions other than static not hidden correctly when parent has display:none
Product: WebKit Reporter: Rob <rob+webkit>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, krit, webkit.review.bot, zherczeg, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.5   
URL: http://maps.afpc.tamu.edu/webkit-fo-html-reduction.svg
Attachments:
Description Flags
test case
none
Patch v1
none
Patch v2 zherczeg: review+

Rob
Reported 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.
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
DetailsFormatted DiffDiff
Patch v2 (29.49 KB, patch)
2012-02-22 05:56 PST, Nikolas Zimmermann 		zherczeg: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2011-12-09 14:34:30 PST
Yup. Definitely a bug. :(
Eric Seidel (no email)
Comment 2 2011-12-09 14:35:06 PST
Here is a link to the accused regressing revision: http://trac.webkit.org/changeset/58212
Nikolas Zimmermann
Comment 3 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.
Nikolas Zimmermann
Comment 4 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.
Nikolas Zimmermann
Comment 5 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>.
Nikolas Zimmermann
Comment 6 2012-02-17 02:50:27 PST
Created attachment 127564 [details] Patch v1
WebKit Review Bot
Comment 7 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
Nikolas Zimmermann
Comment 8 2012-02-22 03:08:33 PST
Anyone?
Nikolas Zimmermann
Comment 9 2012-02-22 05:56:35 PST
Created attachment 128190 [details] Patch v2
Zoltan Herczeg
Comment 10 2012-02-22 06:38:53 PST
Comment on attachment 128190 [details] Patch v2 r=me
Nikolas Zimmermann
Comment 11 2012-02-22 06:49:37 PST
Committed r108494: <http://trac.webkit.org/changeset/108494>
Note You need to log in before you can comment on or make changes to this bug.