Bug 131623 - Dynamic background color changes do not update until a layout is forced
Summary: Dynamic background color changes do not update until a layout is forced
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: BlinkMergeCandidate, InRadar
Depends on:
Blocks:
 
Reported: 2014-04-14 11:01 PDT by Philip Rogers
Modified: 2015-10-16 02:18 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 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.
Comment 1 Radar WebKit Bug Importer 2015-10-06 08:44:48 PDT
<rdar://problem/22991057>
Comment 2 Antoine Quint 2015-10-09 04:15:17 PDT
Created attachment 262758 [details]
Patch
Comment 3 Build Bot 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
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Antoine Quint 2015-10-09 06:07:02 PDT
Created attachment 262765 [details]
Patch
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Antoine Quint 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?
Comment 13 Antoine Quint 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.
Comment 14 Antoine Quint 2015-10-09 08:19:19 PDT
Created attachment 262772 [details]
Patch
Comment 15 Darin Adler 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.
Comment 16 Antoine Quint 2015-10-09 12:06:50 PDT
Created attachment 262785 [details]
Patch for landing
Comment 17 Antoine Quint 2015-10-09 12:07:34 PDT
Addressing the comments in the landing commit, thanks for the review Darin.
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2015-10-09 12:59:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Brent Fulgham 2015-10-15 09:55:45 PDT
The equivalent Windows test expectation was not updated.

Committed r191116: <http://trac.webkit.org/changeset/191116>
Comment 21 Antoine Quint 2015-10-16 02:18:51 PDT
Thanks Brent!