documentElement should not always get a renderer
Created attachment 176383 [details] Patch
Comment on attachment 176383 [details] Patch Attachment 176383 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15026260 New failing tests: svg/as-image/img-zoom-svg-stylesheet.html
Comment on attachment 176383 [details] Patch Attachment 176383 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15013354
Created attachment 176423 [details] Patch
It seems SVG really does require a renderer even when the root is display: none (though this appears to be a bug): https://bugs.webkit.org/show_bug.cgi?id=103493 Changed my patch to reflect this.
Comment on attachment 176423 [details] Patch Attachment 176423 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15016333
Created attachment 176428 [details] Patch
Comment on attachment 176428 [details] Patch Attachment 176428 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15010447 New failing tests: svg/text/text-property-with-display-none.html svg/custom/svg-fonts-without-missing-glyph.xhtml svg/custom/svg-fonts-segmented.xhtml svg/custom/svg-fonts-fallback.xhtml
Comment on attachment 176428 [details] Patch Attachment 176428 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15014438 New failing tests: svg/text/text-property-with-display-none.html svg/custom/svg-fonts-without-missing-glyph.xhtml svg/custom/svg-fonts-segmented.xhtml svg/custom/svg-fonts-fallback.xhtml
SVG needs renderers for content that the SVG spec defines as display:none. Examples include filters, masks, patterns, anything in defs elements, etc. Mozilla have apparently also had issue with this: https://bugzilla.mozilla.org/show_bug.cgi?id=376027
Comment on attachment 176428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176428&action=review > Source/WebCore/svg/SVGSVGElement.cpp:478 > + if (isOutermostSVGSVGElement()) Even non-outermost <svg> elements need renderers, so this method should return true always, I think. See if that fixes the failing tests (which I do not believe to be flakes).
(In reply to comment #11) > (From update of attachment 176428 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176428&action=review > > > Source/WebCore/svg/SVGSVGElement.cpp:478 > > + if (isOutermostSVGSVGElement()) > > Even non-outermost <svg> elements need renderers, so this method should return true always, I think. See if that fixes the failing tests (which I do not believe to be flakes). That's not the issue as we always delegated to StyledElement::rendererIsNeeded, the problem is that the outermost <svg> element should *not* always get a renderer, it's just the documentElement SVG element that needs one. Otherwise things like this don't work: <html> <!-- The SVG should not be shown, but it *is* the outermost SVGSVGElement --> <svg style="display: none"> </svg> </html>
Created attachment 176505 [details] Patch
Comment on attachment 176505 [details] Patch Test?
(In reply to comment #14) > (From update of attachment 176505 [details]) > Test? There's no behavior change.
Comment on attachment 176505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176505&action=review Does this change the behavior of <mathml style="display:none"> documents? I think this would make us not create a renderer for them, which is a bug fix. > Source/WebCore/svg/SVGSVGElement.cpp:477 > + // they should instead depend on the RenderView. Add a link to bug 103493 here. Easier to find that having to dig through commit history.
(In reply to comment #10) > SVG needs renderers for content that the SVG spec defines as display:none. Examples include filters, masks, patterns, anything in defs elements, etc. > > Mozilla have apparently also had issue with this: https://bugzilla.mozilla.org/show_bug.cgi?id=376027 So this wasn't always the case. :) <defs> and <filter> didn't used to have renderers. Before these got renderers we had a special path were we resolved style and held it off of the StyledElement (this path still exists for some elements I believe). This had bugs, and eventually adding a RenderSVGHiddenContanier class made those code-paths cleaner.
(In reply to comment #16) > (From update of attachment 176505 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176505&action=review > > Does this change the behavior of <mathml style="display:none"> documents? I think this would make us not create a renderer for them, which is a bug fix. Yeah that's a good point, I'll add a test. > > > Source/WebCore/svg/SVGSVGElement.cpp:477 > > + // they should instead depend on the RenderView. > > Add a link to bug 103493 here. Easier to find that having to dig through commit history. Okay.
(In reply to comment #18) > (In reply to comment #16) > > (From update of attachment 176505 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=176505&action=review > > > > Does this change the behavior of <mathml style="display:none"> documents? I think this would make us not create a renderer for them, which is a bug fix. > > Yeah that's a good point, I'll add a test. It turns out this doesn't fix any bugs in MathML because MathML forces the nodes into flex-box so doing: <math style="display: none"> does nothing. getComputedStyle(node).display is always -webkit-inline-flex. It would seem that mathml needs fixing here because right now it's impossible to make a mathml block display: none so you'd have to wrap it in a <div> to hide it. :/
Created attachment 176842 [details] Patch
Comment on attachment 176842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176842&action=review > LayoutTests/fast/css/display-none-documentElement.html:11 > + /* > + non HTMLElement documentElement ignores display: none > + https://bugs.webkit.org/show_bug.cgi?id=103681 > + */ I'm confused by this comment. Bug 103681 points to this bug saying that this bug fixes it. Normally we don't include links to the fixed bug in the test content. Am I misunderstanding?
Comment on attachment 176842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176842&action=review >> LayoutTests/fast/css/display-none-documentElement.html:11 >> + */ > > I'm confused by this comment. Bug 103681 points to this bug saying that this bug fixes it. Normally we don't include links to the fixed bug in the test content. Am I misunderstanding? This test case is just super weird to understand so I filed a bug explaining it. I can just put the explanation here instead?
Comment on attachment 176842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176842&action=review Looks good. Please just fix the comment and close bug 103681. >>> LayoutTests/fast/css/display-none-documentElement.html:11 >>> + */ >> >> I'm confused by this comment. Bug 103681 points to this bug saying that this bug fixes it. Normally we don't include links to the fixed bug in the test content. Am I misunderstanding? > > This test case is just super weird to understand so I filed a bug explaining it. I can just put the explanation here instead? Yeah. I think a simple comment here like "// Test display:none on a non-HTMLElement that is the documentElement" is sufficient.
Created attachment 177043 [details] Patch for landing
Created attachment 177073 [details] Patch for landing
Comment on attachment 177073 [details] Patch for landing Clearing flags on attachment: 177073 Committed r136331: <http://trac.webkit.org/changeset/136331>
All reviewed patches have been landed. Closing bug.