Bug 45561

Hi Justin, The SVG spec states, http://www.w3.org/TR/SVG/painting.html, section 11.5 "Controlling visibility" "A value of display: none indicates that the given element and its children shall not be rendered directly (i.e., those elements are not present in the rendering tree). Any value other than none or inherit indicates that the given element shall be rendered by the SVG user agent." That means we should treat all display values the same, as it can either mean it's rendered or not. It should not be possible to alter "display: block" for <text> & <foreignObject> in our implementation. Cheers, Niko

Created attachment 92133 [details] Patch

Comment on attachment 92133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92133&action=review This doesn't seem like the correct fix. Consider the following test case: data:text/html,<div style="display:table-caption"><svg style="display:inherit"><text display="inherit">This test PASSED if we don't crash.</text></svg><div> We also need some test cases that use the CSS value "inherit". > LayoutTests/svg/custom/display-table-caption-expected.txt:2 > +PASS > +PASS I suggest we add some more English to this such that a person can understand what these PASSes mean; that is, neither sub test caused a crash. > LayoutTests/svg/custom/display-table-caption.svg:6 > + <text display="table-caption" y="10">PASS</text> Can we put some text/comment in this test case to explain that this test PASSED if it didn't crash? We need some English sentences in this test to explain its purpose and how to interpret the results. > LayoutTests/svg/custom/display-table-caption.svg:7 > + <foreignObject display="table-caption" y="10"><xhtml:div>PASS</xhtml:div></foreignObject> I'm not very familiar with SVG and this looks like another test case for the display property. If so, I suggest separating this into its own layout test file since it seems more straightforward to diagnose a regression/crash from a minimal test case as opposed to a test case that has many sub tests.

Created attachment 92154 [details] Backtrace Backtrace generated with Safari Version 5.0.5 (6533.21.1, r85595) using Justin Schuh's URL: 'data:text/html,<svg><text display="table-caption">'.

Comment on attachment 92133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92133&action=review > Source/WebCore/ChangeLog:8 > + In SVG for the CSS display property only accept the values block, inline, inherit and none. Thats wrong, we definitely support all values for 'display', like Niko wrote in the quote, other values than 'none' and 'inherit' indicate that the element should get displayed. And we shouldn't do anything different than displaying the element.

Hi Dirk, (In reply to comment #5) > (From update of attachment 92133 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92133&action=review > > > Source/WebCore/ChangeLog:8 > > + In SVG for the CSS display property only accept the values block, inline, inherit and none. > > Thats wrong, we definitely support all values for 'display', like Niko wrote in the quote, other values than 'none' and 'inherit' indicate that the element should get displayed. And we shouldn't do anything different than displaying the element. You are right, I should have pointed out that it is about computed value that gets restricted. Will reword in my next patch. Cheers, Rob.

Created attachment 92254 [details] Patch

Comment on attachment 92254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92254&action=review > Source/WebCore/css/CSSStyleSelector.cpp:3608 > + EDisplay did = LIST_ITEM; How about naming this local variable “display” instead of “did”? I think the use of LIST_ITEM here as “a value where we definitely won’t do setDisplay” is oblique and confusing. > Source/WebCore/css/CSSStyleSelector.cpp:3617 > + if (isInitial) > + did = RenderStyle::initialDisplay(); > + else if (isInherit) > + did = m_parentStyle->display(); > + else if (primitiveValue) > + did = *primitiveValue; > + if ((did >= INLINE && did <= BLOCK) || did == NONE) > + m_style->setDisplay(did); > + return; It would be much better if we could refactor this so it could share code with the normal case. I see the difficulty in doing that since the normal case code has return statements in it for inherit and initial. One way to do it would be to create a stack-based object that would store the old display type and then when it goes out of scope, restores the display type if the new type is not legal for SVG. The code would look like this: case CSSPropertyDisplay: { SVGDisplayPropertyGuard guard(m_style); HANDLE_INHERIT_AND_INITIAL_AND_PRIMITIVE(display, Display) ... } And: class SVGDisplayPropertyGuard { WTF_MAKE_NONCOPYABLE(SVGDisplayPropertyGuard); public: SVGDisplayPropertyGuard(Element*, RenderStyle*); ~SVGDisplayPropertyGuard(); private: #if ENABLE(SVG) RenderStyle* m_style; EDisplay m_originalDisplayPropertyValue; #endif } #if !ENABLE(SVG) inline SVGDisplayPropertyGuard::SVGDisplayPropertyGuard(Element*, RenderStyle*) { } inline SVGDisplayPropertyGuard::~SVGDisplayPropertyGuard() { } #else static inline bool isAcceptableForSVGElement(EDisplay displayPropertyValue) { // FIXME: Is there a better way to write this that will make sure we keep this up to date as we add display property values? return displayPropertyValue >= INLINE && displayPropertyValue <= BLOCK || displayPropertyValue == NONE; } inline SVGDisplayPropertyGuard::SVGDisplayPropertyGuard(Element* element, RenderStyle* style) { if (!(element && element->isSVGElement())) { m_style = 0; return; } m_style = style; m_originalDisplayPropertyValue = style->display(); ASSERT(isAcceptableForSVGElement(m_originalDisplayPropertyValue)); } inline SVGDisplayPropertyGuard::~SVGDisplayPropertyGuard() { if (!m_style) return; if (isAcceptableForSVGElement(m_style->display())) return; m_style->setDisplay(m_originalDisplayPropertyValue); } #endif What do you think of my version?

Hi Darin, > What do you think of my version? I like it because there are less changes in the original code section. I'll try that variant, see whether the tests still work and if so will put up the new patch. Cheers, Rob.

Created attachment 92281 [details] Patch

Comment on attachment 92281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92281&action=review > Source/WebCore/css/CSSStyleSelector.cpp:3535 > + WTF_MAKE_NONCOPYABLE(SVGDisplayPropertyGuard); Wrong indentation there.

http://trac.webkit.org/changeset/85788 might have broken Leopard Intel Debug (Tests) The following tests are not passing: svg/text/text-block-child-crash.xhtml

> svg/text/text-block-child-crash.xhtml Chromium is also having a crash on the same file. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=svg%2Ftext%2Ftext-block-child-crash.xhtml message: ASSERTION FAILED: !m_layoutAttributes.isEmpty() third_party/WebKit/Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp(54) : WebCore::SVGTextLayoutEngine::SVGTextLayoutEngine(WTF::Vector<WebCore::SVGTextLayoutAttributes, 0u>&)

It asserted on all platform, so it was rolled out by http://trac.webkit.org/changeset/85834

Created attachment 92582 [details] Patch

Fixed the problem with the crashing text-block-child-crash.xhtml test now. The problem was that the display restricting was applied to pseudo-element styles like :before, where it should not be restricted. Cheers, Rob.

Comment on attachment 92582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92582&action=review Looks good, r=me. > Source/WebCore/css/CSSStyleSelector.cpp:3542 > + WTF_MAKE_NONCOPYABLE(SVGDisplayPropertyGuard); Too much indented, 4 spaces are enough. > Source/WebCore/css/CSSStyleSelector.cpp:3561 > +#if !ENABLE(SVG) > +inline SVGDisplayPropertyGuard::SVGDisplayPropertyGuard(Element*, RenderStyle*) > +{ > +} > + > +inline SVGDisplayPropertyGuard::~SVGDisplayPropertyGuard() > +{ > +} > +#else I'd write it the other way round. #if ENABLE(SVG) ... #else .. #endif. Feel free to leave it as-is though. > Source/WebCore/css/CSSStyleSelector.cpp:3584 > + if (!m_style) > + return; > + if (isAcceptableForSVGElement(m_style->display())) > + return; You could turn this into one line.

Committed r86771: <http://trac.webkit.org/changeset/86771>

Revision r86771 cherry-picked into qtwebkit-2.2 with commit 51381f8 <http://gitorious.org/webkit/qtwebkit/commit/51381f8>