103475 – documentElement should not always get a renderer

RESOLVED FIXED 103475

documentElement should not always get a renderer

https://bugs.webkit.org/show_bug.cgi?id=103475

Summary documentElement should not always get a renderer

Elliott Sprehn

Reported 2012-11-27 18:17:29 PST

documentElement should not always get a renderer

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Elliott Sprehn

Comment 1 2012-11-27 18:43:38 PST

Created attachment 176383 [details] Patch

WebKit Review Bot

Comment 2 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

Build Bot

Comment 3 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

Elliott Sprehn

Comment 4 2012-11-28 01:15:53 PST

Created attachment 176423 [details] Patch

Elliott Sprehn

Comment 5 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.

Build Bot

Comment 6 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

Elliott Sprehn

Comment 7 2012-11-28 01:37:12 PST

Created attachment 176428 [details] Patch

WebKit Review Bot

Comment 8 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

Build Bot

Comment 9 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

Stephen Chenney

Comment 10 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

Stephen Chenney

Comment 11 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).

Elliott Sprehn

Comment 12 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>  <svg style="display: none"> </svg> </html>

Elliott Sprehn

Comment 13 2012-11-28 10:02:12 PST

Created attachment 176505 [details] Patch

Ojan Vafai

Comment 14 2012-11-28 12:03:11 PST

Comment on attachment 176505 [details] Patch Test?

Elliott Sprehn

Comment 15 2012-11-28 12:13:58 PST

(In reply to comment #14) > (From update of attachment 176505 [details]) > Test? There's no behavior change.

Ojan Vafai

Comment 16 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.

Eric Seidel (no email)

Comment 17 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.

Elliott Sprehn

Comment 18 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.

Elliott Sprehn

Comment 19 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. :/

Elliott Sprehn

Comment 20 2012-11-29 16:03:05 PST

Created attachment 176842 [details] Patch

Ojan Vafai

Comment 21 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?

Elliott Sprehn

Comment 22 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?

Ojan Vafai

Comment 23 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.

Elliott Sprehn

Comment 24 2012-11-30 16:16:03 PST

Created attachment 177043 [details] Patch for landing

Elliott Sprehn

Comment 25 2012-11-30 18:04:57 PST

Created attachment 177073 [details] Patch for landing

WebKit Review Bot

Comment 26 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>

WebKit Review Bot

Comment 27 2012-12-02 03:39:00 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Elliott Sprehn

Reported

2012-11-27 18:17 PST

Modified

2012-12-02 03:39 PST History

CC List

9 users Show

URL

Keywords

Depends on

Blocks

102975 103681

Dependencies

tree graph