Bug 217737 – Twitter Photo gallery incorrectly rendered after opening another modal

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Charlie Croom]

Created attachment 411385 [details]
Layout capture after bug; highlighted area is container of background-image

On twitter.com, we encountered a strange rendering bug when working on our photo gallery modal which causes the layout to incorrectly render when coming back to the gallery. 

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.)
- Go to a Tweet with a photo gallery: https://twitter.com/CharlieCroom/status/968880712286793728
- Open the gallery
- At the bottom is a reply icon, click it to open the composer
- Click the x in the top left of the composer to click it

Expected:
- Gallery renders normally

Actual:
- Gallery renders all messed up (see attachment)

To work around this in production, we've added `borderRadius: '0.01px'` to the dialog element. (It should have aria-modal="true")

[Simon Fraser (smfr)]

(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?

[Charlie Croom]

Current staging URL: https://mobile.twitter.com/?tts_token=Hy2Ncdql0yMKW0Hs

[Charlie Croom]

Yes I did lol. Forgot to wait for it to stage.

[Simon Fraser (smfr)]

I can reproduce.

[Radar WebKit Bug Importer]

<rdar://problem/70314822>

[Simon Fraser (smfr)]

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?

[Charlie Croom]

(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

[Amy Simmons]

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.

[Simon Fraser (smfr)]

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.

[Amy Simmons]

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!

[Simon Fraser (smfr)]

This is happening because there's a path through RenderLayerBacking::updateChildClippingStrategy() that fails too clean up the rounded-rect clipping layers.

[Simon Fraser (smfr)]

Created attachment 412124 [details]
Patch

[Simon Fraser (smfr)]

(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.

[zalan]

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

[Simon Fraser (smfr)]

https://trac.webkit.org/r268898