Summary: | REGRESSION(r272201): Safari showed red distortion on webview after using Web Inspector, returning from another app | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||||||||||||
Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | dino, ews-watchlist, graouts, hi, joepeck, pangle, simon.fraser, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=238067 | ||||||||||||||||||||||
Attachments: |
|
Description
Antoine Quint
2021-10-07 06:11:04 PDT
Created attachment 440496 [details]
Patch for EWS
Created attachment 440498 [details]
Patch for EWS
Created attachment 440499 [details]
Patch for EWS
Created attachment 440538 [details]
Layout Test and Plumbing v1.0
Right now the test still fails, even with the patch applied – If I add a breakpoint inside RepaintIndicatorLayerClient::notifyAnimationEnded in WebInspectorClient.cpp, I don’t hit the breakpoint until I do something like open a new tab, which hides the page (which solves the user facing issue since at that point we will clean everything up) - I’d expect to get the notifications 0.25s after the animation starts, though. My test checks if there are currently repaint rects, but I’m expecting the repaint rects to have been cleared out once their animation finishes. I think once notifyAnimationEnded fires when the animation actually ends, it should work. Created attachment 443198 [details]
Patch
Comment on attachment 443198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443198&action=review r=me > Source/WebCore/inspector/InspectorOverlay.h:209 > + unsigned paintRectCount() const { return m_paintRects.size(); } NIT: please move this next to `setShowPaintRects` and `showPaintRect since it's more related to those than grid overlays :) > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3090 > + animation.m_beginTime = *animation.m_beginTime + animationGroupBeginTime; Would `*animation.m_beginTime += animationGroupBeginTime;` also work? > LayoutTests/inspector/page/setShowPaintRects.html:14 > + let hasPaintRects = !!await InspectorTest.evaluateInPage(`window.internals.inspectorPaintRectCount()`); > + InspectorTest.expectEqual(hasPaintRects, expected, `Should ${expected ? "" : "not "}have paint rects displayed.`); NIT: I personally try to avoid using operators in conjunction with `await` as I find it not super easy to read. I think renaming this to `paintRectsCount` and then using it as `!!paintRectsCount` would be clearer. > LayoutTests/inspector/page/setShowPaintRects.html:23 > + PageAgent.setShowPaintRects(true); NIT: you could/should also `await` this just in case there's an error > LayoutTests/inspector/page/setShowPaintRects.html:26 > + InspectorTest.evaluateInPage(`document.getElementById("test").style.backgroundColor = "papayawhip"`); ditto (:23) > LayoutTests/inspector/page/setShowPaintRects.html:28 > + setTimeout(() => { Is there no better way to do this than just having a `setTimeout`? Perhaps we could/should have a `setInterval` loop in the page that constantly checks and `TestPage.dispatchEventToFrontend` when it notices a change so that we're at least way less likely to miss anything? > LayoutTests/inspector/page/setShowPaintRects.html:45 > + PageAgent.setShowPaintRects(false); ditto :(23) > LayoutTests/inspector/page/setShowPaintRects.html:48 > + InspectorTest.evaluateInPage(`document.getElementById("test").style.width = "rebeccapurple"`); ditto (:23) (In reply to Devin Rousso from comment #9) > Comment on attachment 443198 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443198&action=review > > r=me > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3090 > > + animation.m_beginTime = *animation.m_beginTime + animationGroupBeginTime; > > Would `*animation.m_beginTime += animationGroupBeginTime;` also work? I wasn't sure that it would and didn't bother trying… but it does work! I'm uploading a new patch with this change just to make sure there's no funny stuff. I'll let Patrick Angle answer your other comments as this is the part he wrote. Created attachment 443288 [details]
Patch
Created attachment 443289 [details]
Patch
Comment on attachment 443289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443289&action=review > LayoutTests/inspector/page/setShowPaintRects.html:6 > +<script src="resources/search-script.js"></script> Needed? (Maybe it is!) > LayoutTests/inspector/page/setShowPaintRects.html:48 > + InspectorTest.evaluateInPage(`document.getElementById("test").style.width = "rebeccapurple"`); Err, should this attempt to change the backgroundColor and not width to `rebeccapurple`? > LayoutTests/inspector/page/setShowPaintRects.html:69 > + width: 100px; > + height: 100px; Odd spacing. Created attachment 443626 [details]
Address test-related feedback
Created attachment 443713 [details]
Patch for landing
Committed r285529 (244046@main): <https://commits.webkit.org/244046@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443713 [details]. |