Summary: | Twitter Photo gallery incorrectly rendered after opening another modal | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Charlie Croom <charlie.croom> | ||||||
Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | asimmons, bfulgham, changseok, esprehn+autocc, ews-watchlist, fred.wang, glenn, Hironori.Fujii, jmuizelaar, kondapallykalyan, pdr, sergio, simon.fraser, webkit-bug-importer, zalan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Safari Technology Preview | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
URL: | https://mobile.twitter.com/?tts_token=Hy2Ncdql0yMKW0Hs | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=218111 | ||||||||
Attachments: |
|
Description
Charlie Croom
2020-10-14 16:25:29 PDT
(In reply to Charlie Croom from comment #0) > Reproduce: > - Open this special link which removes our production workaround: . (Because > this is an internal build, it's only available for 3 days at a time, but you > can reach out to me to get a new one.) Did you forget the link? Current staging URL: https://mobile.twitter.com/?tts_token=Hy2Ncdql0yMKW0Hs Yes I did lol. Forgot to wait for it to stage. I can reproduce. Could I get a new staging URL please?
> To work around this in production, we've added `borderRadius: '0.01px'` to the dialog element. (It should have aria-modal="true")
Which element is that? Is it a dialog for the reply, or for the gallery?
(In reply to Simon Fraser (smfr) from comment #6) > Could I get a new staging URL please? https://mobile.twitter.com/?tts_token=ksII522TMwUrIm6u > > To work around this in production, we've added `borderRadius: '0.01px'` to the dialog element. (It should have aria-modal="true") > > Which element is that? Is it a dialog for the reply, or for the gallery? This is in the dialog for the galleryn, it's currently .r-1pp0k63 for me in prod, but that may change...so you can also look for aria-labelledby="modal-header" which points to the element Hi, In case this additional info helps. These are some notes I took at the time we added the border radius of 0.01px. What we know about the bug: - Only happens in Safari - Only happens after exiting the Reply or RT with comment tweet actions, does not happen after exiting the Send via DM tweet action - Only happens on tweets with multiple images, not on single images - Only happens when pressing the 'X' to dismiss the tweet action, does not happen when pressing the mask to go back - After some playing around, you can no longer reproduce the issue, and need to close Safari and re-open in order to reproduce it again How I got to the borderRadius 0.01px solution: - I had been working on a series of enhancements to the media viewer, one of which was making it full screen (previously it was not a full screen modal) - I checked out several of my changes to see at what point the bug was introduced. It was introduced in the very first ticket, where I changed the width and height of the modal to make it full screen and removed the border radius - Previously, all modals had a border radius. But with the new full screen modal, we no longer needed a border radius. So when I implemented the full screen modal I chose to only add a border radius when the modal size was something other than 'full'. Eg the compose modal would have the border radius, but the full screen media modal would not. - I don't understand why adding the borderRadius of 0.01px back in fixes the bug, why it was only an issue for the Reply or RT with Comment tweet actions (not the Send via DM tweet action), or why it was only an issue when clicking the X to dismiss, rather than the mask. Thanks for the extra info, Amy, that's useful. I suspect it's a bug in our compositing code. I'll debug using the staging URL. Thanks Simon. Do you by chance have a rough idea of how long this might take to fix? Trying to figure out if it's worth putting in a temporary solution on our end that doesn't impact perf in the other browsers. Thanks! This is happening because there's a path through RenderLayerBacking::updateChildClippingStrategy() that fails too clean up the rounded-rect clipping layers. Created attachment 412124 [details]
Patch
(In reply to Amy Simmons from comment #10) > Thanks Simon. Do you by chance have a rough idea of how long this might take > to fix? Trying to figure out if it's worth putting in a temporary solution > on our end that doesn't impact perf in the other browsers. Thanks! I can't say anything about when this fix gets released. Your 0.01px radius workaround is fine (I don't believe it has significant perf impact). Using a different element for the reply box with radius, and the gallery would also work. Or you could force a display:none update, but that would have vastly higher perf impact. Comment on attachment 412124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412124&action=review nice cleanup on bool setMasksToBoundsRect() > Source/WebCore/rendering/RenderLayerBacking.cpp:1325 > + auto computeMasksToBoundsRect = [&]() { [&] should do > Source/WebCore/rendering/RenderLayerBacking.cpp:2219 > + auto needsClipMaskLayer = [&]() { Same here |