Bug 131623

Summary:

Dynamic background color changes do not update until a layout is forced

Product:

WebKit

Reporter:

Philip Rogers <pdr>

Component:

SVG

Assignee:

Antoine Quint <graouts>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, buildbot, commit-queue, graouts, krit, rniwa, sabouhallawa, webkit-bug-importer, zimmermann

Priority:

Keywords:

BlinkMergeCandidate, InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ews104 for mac-mavericks-wk2	none
Archive of layout-test-results from ews101 for mac-mavericks	none
Patch	none
Archive of layout-test-results from ews105 for mac-mavericks-wk2	none
Archive of layout-test-results from ews101 for mac-mavericks	none
Patch	none
Patch for landing	none

Philip Rogers

Reported 2014-04-14 11:01:26 PDT

1. Set svg.style.backgroundColor = "blue" 2. Notice the svg element does not change 3. Resize the window and notice the background updates Testcase below: <!DOCTYPE HTML> <html> <head> <script> function test() { var svg = document.getElementById('svg'); setTimeout(function() { svg.style.backgroundColor = "green"; }, 100); } </script> </head> <body onload="test()"> This test passes if there is a green rectangle below:<br/> <svg id="svg" width="100px" height="100px"> </svg> </body> </html> This has been fixed in Blink. We just need to merge the patch from crbug.com/354305.

Attachments
Patch (19.78 KB, patch) 2015-10-09 04:15 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (791.13 KB, application/zip) 2015-10-09 04:55 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews101 for mac-mavericks (710.61 KB, application/zip) 2015-10-09 05:04 PDT, Build Bot	no flags	Details
Patch (21.71 KB, patch) 2015-10-09 06:07 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (698.70 KB, application/zip) 2015-10-09 06:45 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews101 for mac-mavericks (639.18 KB, application/zip) 2015-10-09 06:55 PDT, Build Bot	no flags	Details
Patch (21.37 KB, patch) 2015-10-09 08:19 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch for landing (20.33 KB, patch) 2015-10-09 12:06 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2015-10-06 08:44:48 PDT

<rdar://problem/22991057>

Antoine Quint

Comment 2 2015-10-09 04:15:17 PDT

Created attachment 262758 [details] Patch

Build Bot

Comment 3 2015-10-09 04:55:04 PDT

Comment on attachment 262758 [details] Patch Attachment 262758 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/260872 New failing tests: svg/overflow/overflow-on-outermost-svg-element-in-xhtml-visible.xhtml svg/animations/animate-viewport-overflow.html fast/repaint/moving-shadow-on-container.html svg/animations/animate-viewport-overflow-2.html

Build Bot

Comment 4 2015-10-09 04:55:07 PDT

Created attachment 262760 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Build Bot

Comment 5 2015-10-09 05:04:15 PDT

Comment on attachment 262758 [details] Patch Attachment 262758 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/260892 New failing tests: svg/overflow/overflow-on-outermost-svg-element-in-xhtml-visible.xhtml svg/animations/animate-viewport-overflow.html fast/repaint/moving-shadow-on-container.html svg/animations/animate-viewport-overflow-2.html

Build Bot

Comment 6 2015-10-09 05:04:20 PDT

Created attachment 262761 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5

Antoine Quint

Comment 7 2015-10-09 06:07:02 PDT

Created attachment 262765 [details] Patch

Build Bot

Comment 8 2015-10-09 06:45:38 PDT

Comment on attachment 262765 [details] Patch Attachment 262765 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/261138 New failing tests: svg/overflow/overflow-on-outermost-svg-element-in-xhtml-visible.xhtml svg/animations/animate-viewport-overflow.html svg/animations/animate-viewport-overflow-2.html

Build Bot

Comment 9 2015-10-09 06:45:41 PDT

Created attachment 262767 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Build Bot

Comment 10 2015-10-09 06:55:25 PDT

Comment on attachment 262765 [details] Patch Attachment 262765 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/261168 New failing tests: svg/overflow/overflow-on-outermost-svg-element-in-xhtml-visible.xhtml svg/animations/animate-viewport-overflow.html svg/animations/animate-viewport-overflow-2.html

Build Bot

Comment 11 2015-10-09 06:55:30 PDT

Created attachment 262768 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5

Antoine Quint

Comment 12 2015-10-09 07:00:54 PDT

The newly failing tests are all using an `overflow: visible` style, so this is most likely where the issue is lying. Can a reviewer spot what in the changes made to RenderSVGRoot.cpp would cause repaints to areas outside of the overflowing area not to be performed?

Antoine Quint

Comment 13 2015-10-09 08:17:56 PDT

(In reply to comment #12) > The newly failing tests are all using an `overflow: visible` style, so this > is most likely where the issue is lying. Can a reviewer spot what in the > changes made to RenderSVGRoot.cpp would cause repaints to areas outside of > the overflowing area not to be performed? Never mind that, the issue was pretty clear, overflow handling was already performed in RenderSVGRoot::layout() if shouldApplyViewportClip() was false and we were now resetting the overflow in all cases. Removing the new code for that purpose should fix things right up.

Antoine Quint

Comment 14 2015-10-09 08:19:19 PDT

Created attachment 262772 [details] Patch

Darin Adler

Comment 15 2015-10-09 09:34:37 PDT

Comment on attachment 262772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262772&action=review > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:309 > if (diff == StyleDifferenceLayout) > setNeedsBoundariesUpdate(); > + > + if (diff == StyleDifferenceRepaint) { > + // Box decorations may have appeared/disappeared - recompute status. > + m_hasBoxDecorations = hasBoxDecorationStyle(); > + } > RenderReplaced::styleDidChange(diff, oldStyle); > SVGResourcesCache::clientStyleChanged(*this, diff, style()); If adding a blank line before this new if block, then I suggest also adding a blank line after it. Both or neither. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:355 > + // Return early for any cases where we don't actually paint. All the comments in this function are too “on the nose”, stating what the code below does and we don’t find those comments as valuable as comments saying things that the code does *not* say. They can be improved by focusing more on *why* the code is needed, and minimize stating what the code does. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:361 > + FloatRect contentRepaintRect = repaintRectInLocalCoordinates(); > + contentRepaintRect = m_localToBorderBoxTransform.mapRect(contentRepaintRect); Might read better as a single line. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:372 > + // The selectionRect can project outside of the overflowRect, so take their union > + // for repainting to avoid selection painting glitches. > + LayoutRect decoratedRepaintRect = unionRect(localSelectionRect(false), visualOverflowRect()); > + repaintRect.unite(decoratedRepaintRect); Why not two calls to repaintRect.unite instead? > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:377 > + LayoutRect rect = enclosingIntRect(repaintRect); > + return RenderReplaced::computeRectForRepaint(rect, repaintContainer); Would read better without a local variable and on a single line.

Antoine Quint

Comment 16 2015-10-09 12:06:50 PDT

Created attachment 262785 [details] Patch for landing

Antoine Quint

Comment 17 2015-10-09 12:07:34 PDT

Addressing the comments in the landing commit, thanks for the review Darin.

WebKit Commit Bot

Comment 18 2015-10-09 12:59:46 PDT

Comment on attachment 262785 [details] Patch for landing Clearing flags on attachment: 262785 Committed r190816: <http://trac.webkit.org/changeset/190816>

WebKit Commit Bot

Comment 19 2015-10-09 12:59:51 PDT

All reviewed patches have been landed. Closing bug.

Brent Fulgham

Comment 20 2015-10-15 09:55:45 PDT

The equivalent Windows test expectation was not updated. Committed r191116: <http://trac.webkit.org/changeset/191116>

Antoine Quint

Comment 21 2015-10-16 02:18:51 PDT

Thanks Brent!

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