Bug 103475

Summary:

documentElement should not always get a renderer

Product:

WebKit

Reporter:

Elliott Sprehn <esprehn>

Component:

New Bugs

Assignee:

Elliott Sprehn <esprehn>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, dglazkov, d-r, eric, fmalita, ojan, pdr, schenney, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

102975, 103681

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none
Patch for landing	none

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.