Allow embed sandbox to make content non-interactive for unknown embeds

[notnownikki]

Description

Fixes: #7872

For embeds that work but we don't have a specific block for (i.e. they're not known to Gutenberg) we want to make the content of the frame non-interactive so that whatever links are supplied in the embed's preview don't get clicked and go to different pages inside the iframe. See #7872 for an example of where this can happen.

Why doesn't this use the Disabled component in the embed block? Because we want the iframe to be enabled do you can tab through it as normal, but the content inside should be disabled, and the overlay seems to be an easy way of doing that that has no side effects.

How has this been tested?

Follow the exampled in #7872 and make sure you can't click away from the embedded preview.

Types of changes

Bug fix

[aduth]

For what reason should we have different treatment of content for known vs. unknown embeds. What is it about a Tweet preview that we should want it to be interactable, but not for one like Giphy? The Giphy case seems like its a misbehaving embed moreso than an issue of how we approach embeds generally. What stops links in known embeds from doing the same, other than the embed provider knowingly correcting this behavior?

[notnownikki]

I was looking at this again and thought that perhaps all embed previews should not be interactive. It makes them much easier to select if you can click anywhere on the preview and not have a video start playing, or some text get focus. Do we really want any of them to be interactive in the editor? They are just previews, after all. Perhaps interactive defaults to false?

[gziolo]

Moving to 4.2 so we could focus on bigger UI tasks in 4.1.

[aduth]

It makes them much easier to select if you can click anywhere on the preview and not have a video start playing, or some text get focus.

Previously: #483

Do we really want any of them to be interactive in the editor? They are just previews, after all. Perhaps interactive defaults to false?

Personally, and especially if we did have an overlay effect for the initial select (#483), I'd be perfectly okay with the embed being interactable, as it matches the expectation on the front-end. Even if that means living with #7872, if that's the same behavior which occurs on the front-end (needs testing).

[youknowriad]

Seems blocked somehow. I'm moving out for 4.2 for now.

[notnownikki]

If we can live with #7872 then let's close this. Otherwise I'll fix up the conflicts and get this unblocked.

[notnownikki]

Actually, looking at #11885 we probably want to be able to opt blocks in to this. I'll start unblocking it.

[aduth]

I'm not sure I see a benefit to assigning this to a variable if it's not in-fact a single source of truth (i.e. duplicated in the style assignment).

[notnownikki]

Me neither, but the linter doesn't allow a string literal as id :)

[aduth]

Out of curiosity, is there any reason we couldn't use Disabled for this?

https://github.com/WordPress/gutenberg/tree/master/packages/components/src/disabled

[notnownikki]

Disabled changes the tab order I think, I wanted to keep it consistent

…

On Mon, 19 Nov 2018, 19:16 Andrew Duthie ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/components/src/sandbox/index.js <#10512 (comment)>: > `; - + const overlayId = 'wp-sandbox-overlay'; I'm not sure I see a benefit to assigning this to a variable if it's not in-fact a single source of truth (i.e. duplicated in the style assignment). ------------------------------ In packages/block-library/src/embed/embed-preview.js <#10512 (comment)>: > @@ -45,6 +45,7 @@ const EmbedPreview = ( props ) => { scripts={ scripts } title={ iframeTitle } type={ sandboxClassnames } + interactive={ interactive } Out of curiosity, is there any reason we couldn't use Disabled for this? https://github.com/WordPress/gutenberg/tree/master/packages/components/src/disabled — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10512 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADKSsgxVs8iWiFNW6-qf_CkSWbm20R6Pks5uwwOEgaJpZM4XXg39> .

[aduth]

I'm still unsure of this one.

I am fine to live with #7872 as-is if that's the same behavior one can expect from the front-end embed.

#11885 sounds like a regression for a behavior I thought we'd applied long ago, which is that the first click on an embed should select it, and only then should the embed's content be interactable. I don't think a Vimeo block should be non-interactable, only that it should also not be difficult to select the block without also accidentally causing interactions (play, pause). This is not at all specific to Vimeo. YouTube blocks suffer the same, as would pretty much any other interactable embed block.

[notnownikki]

Circling back round to this one again, as #1993 (comment) reminded me how much this trips me up too.

I'll rework it so that the preview iframe is disabled while the block doesn't have focus, so that initial click shouldn't trigger anything, but will select the block. That might get rid of 90% of the issues here.

[notnownikki]

The approach with Disabled causes bugs here, if we disable only when the block is not selected.

1. An optional Disabled wrapping element causes the preview to be re-rendered when the block gets selected. This results in significant UI jumps.

2. Clicking on a disabled preview stops mouse events bubbling, and so Gutenberg goes not realise that the mouse button has been released, only that it has been pressed. So when you move your mouse, you start multi-selecting blocks.

I'll try the same approach, but with the overlay instead and see how it goes.

[notnownikki]

Tried with the overlay method and it has the same problem as using Disabled - you end up multi selecting because the mouseup event doesn't get processed and Gutenberg things you're dragging. I'm not sure why this is yet.

I think the solution might be to have an overlay that stays there, and allows click events to propagate depending on isSelected. That will avoid re-rendering the preview iframe and overlay, if it works.

[notnownikki]

I think the solution might be to have an overlay that stays there, and allows click events to propagate depending on isSelected.

And that doesn't work either, because by the time the click event gets to the overlay, the block is selected.

I think it's going to have to be some state in the EmbedPreview component that keeps track of if the block has just been selected, and doesn't let that click though, but lets through subsequent clicks.

[notnownikki]

Just posting my findings again...

So if you click on an element that is there when the block is not selected, but absent when it is selected (in the case of Disabled or an overlay div that serve to stop the initial click making it through to the embedded content) then Gutenberg doesn't pick up on the mouseup event and you're selecting multiple blocks when you move the mouse around. Having a Disabled component disappear also resulted in a re-render of the preview, which made UI element jump about the screen while it rendered again. That is also the case if an overlay div was turned on and off inside the sandbox component, because the change in props resulted in a render.

The way to work around all of this is to have an overlay that does not go away at all, but instead we could change the size, or zindex, or something elseto make it appear to disappear. But we can't changeclassName` because that's a prop change and we would re-render. So, we'd have to get a ref and change the style in the node itself. That'll let the mouse events propagate correctly, and avoid unnecessary renders.

[notnownikki]

Closing this in favor of #12981 which solves it with an overlay and doesn't have the re-rendering or multi-selection issues.