Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require an initial click on embed previews for interactivity #12981

Merged

Conversation

notnownikki
Copy link
Member

@notnownikki notnownikki commented Dec 18, 2018

Description

This fixes the block selection UX for video embeds. Previously
the click on the video to select the block would start the
video playing. Now that previews require a click to become
interactive, the initial click selects the block without
side effects.

How has this been tested?

Embed a youtube video. Click outside of the block. Click the video. The block should be selected, and the video should not start to play.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@notnownikki notnownikki added the [Block] Embed Affects the Embed Block label Dec 18, 2018
@notnownikki notnownikki added this to the 4.8 milestone Dec 18, 2018
@notnownikki notnownikki self-assigned this Dec 18, 2018
@notnownikki notnownikki requested a review from a team December 18, 2018 16:14
@notnownikki
Copy link
Member Author

This fixes more issues too, such as if you embed a twitter timeline, you can get caught scrolling the timeline as you use the mousewheel to scroll down the page. But with this, you scroll as expected.

@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018
@karmatosed karmatosed added the Needs Design Feedback Needs general design feedback. label Dec 19, 2018
.wp-block-embed-interactive-overlay {
position: absolute;
width: 100%;
height: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when using top, right, bottom, left, there's no need for width and height.

</figure>
);
{ ! interactive && <div
role="dialog"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this div is not an ARIA dialog, doesn't have a particular semantic meaning nor can be focused so this role needs to be removed

@afercia
Copy link
Contributor

afercia commented Dec 19, 2018

Left a couple minor comments. More importantly, seems to me while this approach helps with selecting the block, it also introduces a potentially confusing interaction model,

Especially users who use input devices other than mouse / touch or assistive technologies might be confused. Also, I'm not sure mouse / touch users would understand why a first click doesn't "play" and a second click does. I'd tend to think we'd need a different approach.

We should always take into consideration all devices and design for all users. Keyboard users, for example, can still get to the player controls: in the screenshot below, I've used a rgba red background on the overlay to make it clearly visible:

screenshot 2018-12-19 at 17 21 19

As you can see, my mouse is on the paragraph above the embed. With the keyboard, I'm still able to tab through all the player controls and activate them (see the "watch later" control). Many users make a mixed use of mouse and keyboard: most likely, they would be confused by the different interaction with the overlay.

If we really want to make the player "inert" to allow a better block selection mechanism, then I'd tend to think the player should be "inert" for all users until the block is selected. Offering a different experience to different users seems to me not so ideal.

A while ago, there has been an experiment to introduce a new selection and navigation mechanism with the blocks: see "navigation and edit mode" tried in #5709 and recently revived with the proposal in #11581. I'd be in favor of pairing the experience for all users: navigation / edit mode would help users to have the same experience, regardless of the device they use.

@notnownikki
Copy link
Member Author

If we really want to make the player "inert" to allow a better block selection mechanism, then I'd tend to think the player should be "inert" for all users until the block is selected. Offering a different experience to different users seems to me not so ideal.

That's not possible, unfortunately. If we have an overlay making previews inert based on isSelected, then mouse clicks get through to the preview when selecting the block because as soon as the mouse goes down, the block becomes selected and the overlay is switched off, and the click goes through to the underlying preview. It's a really horrible problem, but IMO having a slightly inconsistent experience for combined keyboard/mouse users is better than having videos that play when you try to select a block with a touch or click.

@jasmussen
Copy link
Contributor

I'm about to head out for some family things, but I have ideas to share that could get a version of this in. We also have some past mockups that I think we can dig up. I think there's a path forward, but I'll share more tomorrow.

@afercia
Copy link
Contributor

afercia commented Dec 20, 2018

That's not possible, unfortunately

It is possible, at least for keyboard users. I've pointed to #5709 / #11581 have you had a look at those?

Screen reader users would need something more, as with a screen reader it's always possible to use arrows navigation to get the content.

having a slightly inconsistent experience for combined keyboard/mouse users is better than ...

I disagree 🙂We're aiming to build accessible software. The first step is providing the same content and experience to all users in a device-independent way, as this is the real nature of the web.

@jasmussen
Copy link
Contributor

Thank you for fearlessly taking a stab at this. I strongly agree this is something we need to fix.

Regarding the specific way we fix this, I have reopened #483 which has a bunch of mockups, and was supposedly fixed in #5730. Do we know why that fix regressed? CC: @aduth

In any case, to address the mouse/keyboard situation, can the mockup on that old ticket inspire an approach that would be the same for both?

click blocker

I imagine it could work like this:

  1. You load up an existing post with a video embed. No block is selected. You see the video embed clearly — it is not dimmed in any way.
  2. You select the video embed. Whether through keyboard navigation or a click. You see the dimmed version above, and the video does not start by being selected.

Is the above possible? Does it address the keyboard/mouse issues?

If we need for you to be able to interact with the contents of the embed inside the editor, we could add a a button on top of the dimmed/disabled embed: "Interact with content" (or a better label). Clicking this button would make the embed not disabled again, until you deselect the block. It could look like this:

dimmer-mockup

Can we use tabIndex to make the embed container un-interactable by keyboard until it has been "unlocked" with the button press?

@notnownikki
Copy link
Member Author

Can we use tabIndex to make the embed container un-interactable by keyboard until it has been "unlocked" with the button press?

This is possible as long as we change the tabindex directly on the DOM node instead of changing the tabindex prop. If we change the prop, the iframe is re-rendered, and the preview is regenerated and the UI jumps about. So yeah, it's possible, the code just needs to have plenty of comments explaining why it's not being done in the obvious way :)

If we need for you to be able to interact with the contents of the embed inside the editor, we could add a a button on top of the dimmed/disabled embed: "Interact with content" (or a better label).

As I understand it, it would need to be done this way with an interactive element for screen readers to work properly. One of the issues brought up in the core editor chat was that the static div used in the current code would not work with screen readers.

@jasmussen
Copy link
Contributor

As I understand it, it would need to be done this way with an interactive element for screen readers to work properly. One of the issues brought up in the core editor chat was that the static div used in the current code would not work with screen readers.

But a static div with a button inside, clearly labeled, perhaps even with a verbose description, that would presumably be fine, right?

I think we need to lean into the design principle that a selected block can show additional controls and interface. So I think this shouldn't be a problem.

@notnownikki
Copy link
Member Author

But a static div with a button inside, clearly labeled, perhaps even with a verbose description, that would presumably be fine, right?

Yes, absolutely. The problem with this PR is it's only a static div, no elements that a screenreader would recognise. So the second mockup would be great!

However, if we got the FocusableIframe fixed, would we want this anyway?

@jasmussen
Copy link
Contributor

However, if we got the FocusableIframe fixed, would we want this anyway?

Ideally yes, even if not as part of this PR (and by that I mean I think it's okay to ship a small fix and then a bigger fix later). But I have a feeling this PR might need to go into 5.1 as opposed to 5.0.3 which might not happen depending on things.

But yes, because the way it is right now, any YouTube videos appear "interactable" even if they aren't (i.e. they require two actions). It's probably better to embrace that we are intercepting the actions and making it more visually clear what's happening.

The balance in all these cases is finding a way that isn't creating "heavy UI", so as to consider all aspects of accessibility, including use cases 1 and 6 in this poster. But because this "action interceptor UI" is only visible when the block is selected, it is not additional UI that weighs down the whole thing, and it's clear how to "get it away" (with the button action).

@afercia
Copy link
Contributor

afercia commented Dec 20, 2018

Can we use tabIndex

Short version: no 🙂

Long version: assistive technologies do allow to navigate to elements with a tabindex -1 so the embed content would still be interactive for these users. As accessibility team we should try to explain this better to all the designers involved, as it seems to be a recurring question.

Looking at the history, I see in in #517 (comment) etc. there is also some discussion about the different embed types. Should this apply to only videos or to all the embed types?

I see the overlay approach was already implemented and then removed in #816. Seems to me the main concerns were about scoping the CSS #816 (comment) and problems with a "resizing iframe" #816 (comment)

Later, a solution that used the blur event was implemented in #4011 to my understanding that's no longer possible after #4872.

Lastly, the issue was fixed again in #5730 as Joen mentioned. Checked in Gutenberg 2.9.0 it worked. Worth noting the video couldn't be played at all, not even after multiple clicks. Stopped working in Gutenberg 3.0.0 so maybe this part is a regression?

Granted, even in 2.9.0 keyboard and assistive technologies users were still able to tab to the video player so even then the implemented solution was mouse-only.

Trying some lateral thinking: in #483 Joen mentioned "Abstract live preview". Do we really need to display a fully functional video? Is it possible to display the video poster image instead? Do all video embed providers have a poster image in their response? This would also make the page way lighter. A button #483 (comment) could be used to load the actual video + player.

@jasmussen
Copy link
Contributor

Do we really need to display a fully functional video?

So long as these embeds are super basic (and they really are just the oembeds at this point), I don't personally think they should be fully functional. Though I would really hate for us to go into the abstracted live preview territory, even if I did think so in the past. With the improvements made to editor styles, the editor itself is increasingly a preview of the final content, and discrepancies in that preview are therefore likely to cause user concerns: "Am I doing this right? It looks wrong".

This used to be solved, and I know this is solved in the classic editor. I think Ella is AFK, but perhaps @azaozz can provide insights?

For what it's worth, if we can make the entire embed completely inert, for mouse and keyboard users alike — totally un-interactable — I think that would be fine, so long as the visual of the embed is the same. And if we did this, yes, it would be nice if we could do this for only videos, so non video embeds were still interactible.

@notnownikki
Copy link
Member Author

Short version: no 

😭

Later, a solution that used the blur event was implemented in #4011 to my understanding that's no longer possible after #4872.

😭 Ok, then we should remove the FocusableIframe from the embed block altogether. That explains why the debug code I tried didn't work, if the focus events are no longer supported.

Trying some lateral thinking: in #483 Joen mentioned "Abstract live preview". Do we really need to display a fully functional video? Is it possible to display the video poster image instead?

🔔 🔔 🔔 🔔 I think we might have a winner here 😁 Allowing an email to show the poster with a button which displays an interactive full preview when clicked sounds like it sovles all of this.

@jasmussen :

I don't personally think they should be fully functional.

Sorry, but they really should be for the next round of embed enhancements that change things like YouTube start time, player theme, branding, Twitter media options, etc.... It would be a poor UX if we had to get the user to preview their post to check all the options were set right and looked right, when the current implementation of that branch (admittedly, only on my local machine right now...) updates the preview live.

@notnownikki
Copy link
Member Author

This used to be solved, and I know this is solved in the classic editor.

Looks like it used the same approach as we had before with the FocusableIframe, which we can't use now because of the API change.

@afercia
Copy link
Contributor

afercia commented Dec 20, 2018

I know this is solved in the classic editor

Yep I think it's a TinyMCE thing. However, even in the Classic Editor users are able to get to the player when using a screen reader... it's just a bit more difficult.

screenshot 2018-12-20 at 10 26 34

For what it's worth, if we can make the entire embed completely inert, for mouse and keyboard users alike — totally un-interactable — I think that would be fine, so long as the visual of the embed is the same.

Yeah I'd love to see some exploration in that direction.

Ok, then we should remove the FocusableIframe from the embed

Not sure #4011 / #4872 have something to do with that 🙂 there must be something else that makes the FocusableIframe not work as intended.

@jasmussen
Copy link
Contributor

Sorry, but they really should be for the next round of embed enhancements that change things like YouTube start time, player theme, branding, Twitter media options, etc.... It would be a poor UX if we had to get the user to preview their post to check all the options were set right and looked right, when the current implementation of that branch (admittedly, only on my local machine right now...) updates the preview live.

I agree, but if step 1 was to make the embeds completely inert, step 2 could then be to introduce the dimmer + button from the mockup to make it not inert, would that work?

@notnownikki
Copy link
Member Author

I agree, but if step 1 was to make the embeds completely inert, step 2 could then be to introduce the dimmer + button from the mockup to make it not inert, would that work?

If you do step 1 by itself, you get rid of embed previews completely for things like Twitter that have no thumbnail. Please don't do that 😄

@jasmussen
Copy link
Contributor

If you do step 1 by itself, you get rid of embed previews completely for things like Twitter that have no thumbnail. Please don't do that 😄

Well, I'm not referring to the abstracted preview that Andrea suggested, I'm not in love with that idea. I'm suggesting we still have the embed exactly as it is in this very branch, but it just utterly inert, you can click it to select the block, but nothing inside the iframe is interactible. I'm not sure it's possible, clearly we can't redirect the tabindex to let you not select it, but I know that for clicks we can use pointer-events: none; — perhaps we need to use JS to let you not tab into it?

@paaljoachim
Copy link
Contributor

paaljoachim commented Feb 7, 2019

What about just a smaller hit area to begin playing the video?
As today it covers most of the embed.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final question to consider:

Do we really need any of the focus/click handling or interactive state, or would it have been enough to just show the overlay when the isSelected prop is false? In some brief testing, this seems to me to have the exact same effect as what's proposed here.

In other words, I wonder if it would have been enough for the changes to embed-preview.js to amount to:

diff --git a/packages/block-library/src/embed/embed-preview.js b/packages/block-library/src/embed/embed-preview.js
index efc11b7a5..08e29dff4 100644
--- a/packages/block-library/src/embed/embed-preview.js
+++ b/packages/block-library/src/embed/embed-preview.js
@@ -47,6 +47,9 @@ const EmbedPreview = ( props ) => {
 				title={ iframeTitle }
 				type={ sandboxClassnames }
 			/>
+			{ ! isSelected && (
+				<div className="block-library-embed__interactive-overlay" />
+			) }
 		</div>
 	);

@notnownikki
Copy link
Member Author

The reason for the state was because isSelected becomes true on mouse down, and because the mouse down event goes to the overlay but the mouse up doesnt, the block state gets confused and we end up entering multi selection mode with the mouse up. That may have changed, or be browser specific? But that was the reason for needing the state at time of implementation. Try putting an embed block in the middles of a load of paragraphs, clicking it, and then moving the mouse around. If it's not multi selecting, then whatever was causing that has gone away and I'll look at it further when im back from afk on Monday 🙂

@aduth
Copy link
Member

aduth commented Feb 7, 2019

Ah, yes! And my memory must be failing me, since I had just remarked the other day on the specific comment describing this behavior 😅 I confirmed that a multi-selection wrongly occurs with my naive alternative implementation, so feel free to proceed.

@notnownikki notnownikki merged commit ae4067f into master Feb 11, 2019
@notnownikki
Copy link
Member Author

Thanks to everyone for the help on this one!

@notnownikki notnownikki deleted the update/embed-interactive-content-requires-initial-click branch February 11, 2019 12:02
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
This fixes the block selection UX for video embeds. Previously
the click on the video to select the block would start the
video playing. Now that previous require a click to become
interactive, the initial click selects the block without
side effects.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
This fixes the block selection UX for video embeds. Previously
the click on the video to select the block would start the
video playing. Now that previous require a click to become
interactive, the initial click selects the block without
side effects.
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.