Bug 103475 - documentElement should not always get a renderer
Summary: documentElement should not always get a renderer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks: 102975 103681
  Show dependency treegraph
 
Reported: 2012-11-27 18:17 PST by Elliott Sprehn
Modified: 2012-12-02 03:39 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.23 KB, patch)
2012-11-27 18:43 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (4.26 KB, patch)
2012-11-28 01:15 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (4.33 KB, patch)
2012-11-28 01:37 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (4.35 KB, patch)
2012-11-28 10:02 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (6.59 KB, patch)
2012-11-29 16:03 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (30.33 KB, patch)
2012-11-30 16:16 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (6.51 KB, patch)
2012-11-30 18:04 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-11-27 18:17:29 PST
documentElement should not always get a renderer
Comment 1 Elliott Sprehn 2012-11-27 18:43:38 PST
Created attachment 176383 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-27 20:29:41 PST
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 3 Build Bot 2012-11-27 22:33:37 PST
Comment on attachment 176383 [details]
Patch

Attachment 176383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15013354
Comment 4 Elliott Sprehn 2012-11-28 01:15:53 PST
Created attachment 176423 [details]
Patch
Comment 5 Elliott Sprehn 2012-11-28 01:17:35 PST
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 6 Build Bot 2012-11-28 01:32:08 PST
Comment on attachment 176423 [details]
Patch

Attachment 176423 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15016333
Comment 7 Elliott Sprehn 2012-11-28 01:37:12 PST
Created attachment 176428 [details]
Patch
Comment 8 WebKit Review Bot 2012-11-28 02:12:49 PST
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 9 Build Bot 2012-11-28 03:15:48 PST
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
Comment 10 Stephen Chenney 2012-11-28 08:22:26 PST
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 11 Stephen Chenney 2012-11-28 08:49:35 PST
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).
Comment 12 Elliott Sprehn 2012-11-28 09:59:33 PST
(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>
Comment 13 Elliott Sprehn 2012-11-28 10:02:12 PST
Created attachment 176505 [details]
Patch
Comment 14 Ojan Vafai 2012-11-28 12:03:11 PST
Comment on attachment 176505 [details]
Patch

Test?
Comment 15 Elliott Sprehn 2012-11-28 12:13:58 PST
(In reply to comment #14)
> (From update of attachment 176505 [details])
> Test?

There's no behavior change.
Comment 16 Ojan Vafai 2012-11-28 12:20:53 PST
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.
Comment 17 Eric Seidel (no email) 2012-11-28 13:27:14 PST
(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.
Comment 18 Elliott Sprehn 2012-11-28 13:44:05 PST
(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.
Comment 19 Elliott Sprehn 2012-11-29 14:02:01 PST
(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. :/
Comment 20 Elliott Sprehn 2012-11-29 16:03:05 PST
Created attachment 176842 [details]
Patch
Comment 21 Ojan Vafai 2012-11-30 15:07:12 PST
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 22 Elliott Sprehn 2012-11-30 15:19:20 PST
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 23 Ojan Vafai 2012-11-30 16:04:57 PST
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.
Comment 24 Elliott Sprehn 2012-11-30 16:16:03 PST
Created attachment 177043 [details]
Patch for landing
Comment 25 Elliott Sprehn 2012-11-30 18:04:57 PST
Created attachment 177073 [details]
Patch for landing
Comment 26 WebKit Review Bot 2012-12-02 03:38:54 PST
Comment on attachment 177073 [details]
Patch for landing

Clearing flags on attachment: 177073

Committed r136331: <http://trac.webkit.org/changeset/136331>
Comment 27 WebKit Review Bot 2012-12-02 03:39:00 PST
All reviewed patches have been landed.  Closing bug.