Check mute toggle when autoplay is enable and show notice when attempting to autoplay unmuted video #6547

dhaval-parekh · 2021-08-18T13:55:44Z

Summary

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

codecov · 2021-08-23T08:50:33Z

Codecov Report

Merging #6547 (753d87e) into develop (d3a85fa) will increase coverage by 0.12%.
The diff coverage is 33.33%.

❗ Current head 753d87e differs from pull request most recent head 33c0dc1. Consider uploading reports for the commit 33c0dc1 to get more accurate results

@@              Coverage Diff              @@
##             develop    #6547      +/-   ##
=============================================
+ Coverage      75.48%   75.60%   +0.12%     
+ Complexity      6024     6023       -1     
=============================================
  Files            190      239      +49     
  Lines          18126    18909     +783     
=============================================
+ Hits           13683    14297     +614     
- Misses          4443     4612     +169

Flag	Coverage Δ
javascript	`78.41% <0.00%> (?)`
php	`75.48% <100.00%> (ø)`
unit	`75.48% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
assets/src/block-editor/helpers/index.js	`37.93% <0.00%> (ø)`
includes/sanitizers/class-amp-video-sanitizer.php	`99.18% <100.00%> (+0.02%)`	⬆️
src/Infrastructure/ServiceBasedPlugin.php	`87.23% <0.00%> (-0.21%)`	⬇️
...rc/block-validation/components/amp-toggle/index.js	`100.00% <0.00%> (ø)`
...g-wizard/components/navigation-context-provider.js	`85.71% <0.00%> (ø)`
...lidation/components/invalid-block-outline/index.js	`0.00% <0.00%> (ø)`
...izard/pages/template-mode/get-selection-details.js	`82.35% <0.00%> (ø)`
...s/amp-validation-status/revalidate-notification.js	`100.00% <0.00%> (ø)`
...lidation/components/amp-validation-status/index.js	`0.00% <0.00%> (ø)`
assets/src/block-editor/store/selectors.js	`100.00% <0.00%> (ø)`
... and 42 more

github-actions · 2021-08-23T08:53:21Z

Plugin builds for ad01cdd are ready 🛎️!

Download development build
Download production build

delawski

Thanks for your work, @dhaval-parekh.

I left some feedback on the JS part.

From the functional point of view, everything seems to be working well.

assets/src/block-editor/helpers/index.js

pierlon

I'm noticing some inconsistencies with the notice in the panel body. Initially it renders above the video settings panel, but if I go to post settings and back to the block settings, it now renders below the video settings panel:

inconsistency.mov

pierlon · 2021-08-26T07:35:46Z

includes/sanitizers/class-amp-video-sanitizer.php

@@ -305,6 +308,10 @@ private function filter_attributes( $attributes ) {
 			}
 		}

+		if ( isset( $out['autoplay'] ) ) {


A comment would be useful here to provide context. Also, we would only need to remove the muted attribute when it is set, so $out['muted'] could be added as a condition here as well.

Suggested change

if ( isset( $out['autoplay'] ) ) {

// The amp-video will forcibly be muted whenever it is set to autoplay, so omit the

// `muted` attribute if it exists.

if ( isset( $out['autoplay'], $out['muted'] ) ) {

tests/php/test-amp-video-sanitizer.php

pierlon · 2021-08-27T08:35:41Z

I'm noticing some inconsistencies with the notice in the panel body. Initially it renders above the video settings panel, but if I go to post settings and back to the block settings, it now renders below the video settings panel

So this has been a bug in Gutenberg for quite some time now: WordPress/gutenberg#15641. @dhaval-parekh @delawski any other workarounds in mind? Maybe we could show a post notice or an alert().

delawski · 2021-08-27T12:43:46Z

So this has been a bug in Gutenberg for quite some time now: WordPress/gutenberg#15641. @dhaval-parekh @delawski any other workarounds in mind? Maybe we could show a post notice or an alert().

I've proposed a rather crude workaround in #6573 but maybe it's the way to go for now. Give it a try locally.

…utoplay-2 Add muted message via direct DOM manipulation

…ute-when-autoplay * origin/develop: (35 commits) Remove redundant `object-fit=contain` from img elements Add test coverage for class appending Remove redundant object-fit=contain from video Set `amp-wp-unknown-size` class for videos without dimensions Update amp-toolbox-php to 0.7.0 Update tests to account for comments being stripped out Use `VisuallyHidden` for other screen reader texts Center chevron in toggle button Update test snapshots Use `VisuallyHidden` component to render hidden text Remove forced-disabling of SSR when Bento is enabled Update amp-toolbox-php to 7e63e5d Update test Update `translators` text Show a warning notice for featured image errors Add WebP and SVG as accepted image formats for a featured image Override `cleanupIDs` params instead redefining entire plugins list Update `svgo` config - prevent deprecation notices Test TikTok embed in Custom HTML block to fix missing coverage Update skip message for TikTok oEmbed conversion ...

assets/src/block-editor/helpers/index.js

westonruter · 2021-08-30T17:48:03Z

assets/src/block-editor/helpers/index.js


 		let inspectorControls;

 		if ( 'core/gallery' === name ) {
 			inspectorControls = setUpGalleryInspectorControls( props );
 		} else if ( 'core/image' === name ) {
 			inspectorControls = setUpImageInspectorControls( props );
-		} else if ( MEDIA_BLOCKS.includes( name ) || 0 === name.indexOf( 'core-embed/' ) ) {
+		} else if ( 'core/video' === name ) {
+			inspectorControls = isSelected ? VideoInspectorControls( props ) : null;


Suggested change

inspectorControls = isSelected ? VideoInspectorControls( props ) : null;

inspectorControls = isSelected ? VideoInspectorControls( props ) : setUpInspectorControls( props );

assets/src/block-editor/helpers/index.js

pierlon · 2021-08-30T17:59:52Z

Still noticing the message being removed whenever the <PanelBody> is being re-rendered:

notice-disappear.mov

Co-authored-by: Weston Ruter <[email protected]>

westonruter · 2021-08-30T18:14:36Z

What I'll do is cherry-pick the PHP changes into a new PR which we'll include in 2.1.4 and then we can finish this up for 2.2.

westonruter · 2021-08-30T18:22:05Z

See #6581.

…disable-mute-when-autoplay * 'develop' of github.com:ampproject/amp-wp: Fix video_with_autoplay test after #6576 Add test case for video with autoplay and muted on Update unit test case Remove mute attr from noscript video tag, and while autoplay is enable Place import in correct position Remove unnecessary constant Show a warning notice when there are validation errors for the featured image Update comments Simplify PrePublishPanel because we already applied filter to PostFeaturedImage Fix tests Keep featured image check in pre-publish panel after selected an image Add PostFeaturedImage to PrePublishPanel

delawski · 2021-08-31T16:03:36Z

Still noticing the message being removed whenever the <PanelBody> is being re-rendered

I've looked into that issue and added E2E tests to cover this particular scenario (2ca8cc7).

There doesn't seem to be an easy fix for that. The "Video settings" panel is rendered by the Video block's Edit function.

In our app, we're using the editor.BlockEdit filter and so we're tapping into the React tree before the Video block Edit function is called. For this reason, we don't "know" when the "Video settings" panel gets re-rendered (and that's what happens when you expand/collapse a panel).

In our current implementation, we're reacting to the entire Video block sidebar changes. It works well in case a block is added, selected, or re-selected. However, we're completely unaware of any changes happening deeper in the app tree.

What's more, since the panel's expanded/collapsed state is stored in the component's local state and not in the global data store, we won't be able to make use of the subscribe() function (from @wordpress/data).

At this point, the only potential workaround I see is setting up a MutationObserver to imperatively alter the DOM tree rendered by React.

westonruter · 2021-09-01T19:51:51Z

Should we just opt to contribute the notice improvement to Gutenberg core instead then?

delawski · 2021-09-01T20:59:49Z

Should we just opt to contribute the notice improvement to Gutenberg core instead then?

I'm not sure. From what I experienced so far, Gutenberg maintainers are not very keen on opening and extending the existing core APIs and blocks.

I gave a MutationObserver a try in ad01cdd and it seems to be working pretty well. I've also covered a case when multiple core/video blocks are selected (in this case Gutenberg picks the attributes of the first block).

Probably the last thing we would need is to do a stress test to see if the observer doesn't negatively affect the editor performance.

dhaval-parekh self-assigned this Aug 23, 2021

dhaval-parekh marked this pull request as ready for review August 23, 2021 08:48

dhaval-parekh requested a review from pierlon as a code owner August 23, 2021 08:48

westonruter requested a review from delawski August 23, 2021 19:50

delawski requested changes Aug 24, 2021

View reviewed changes

assets/src/block-editor/helpers/index.js Outdated Show resolved Hide resolved

assets/src/block-editor/helpers/index.js Outdated Show resolved Hide resolved

dhaval-parekh added 5 commits August 24, 2021 19:11

Remove mute attr from noscript video tag, and while autoplay is enable

d9d9bc8

Automatically mute the video while autoplay enable

609c35e

Add notice when autoplay enable in video component

f9b162d

Update unit test case

772a8af

Introduce new function to setup inspector controller for video

33c0dc1

dhaval-parekh force-pushed the bug/4635-disable-mute-when-autoplay branch from 753d87e to 33c0dc1 Compare August 24, 2021 14:48

dhaval-parekh requested a review from delawski August 24, 2021 14:53

westonruter added this to the v2.1.4 milestone Aug 25, 2021

delawski approved these changes Aug 25, 2021

View reviewed changes

pierlon suggested changes Aug 26, 2021

View reviewed changes

dhaval-parekh added 2 commits August 26, 2021 17:43

Add test case for video with autoplay and muted on

fe7959b

Fix notice apperance condition

8791efa

Add muted message via direct DOM manipulation

066b1de

delawski mentioned this pull request Aug 27, 2021

Add muted message via direct DOM manipulation #6573

Merged

2 tasks

delawski and others added 3 commits August 30, 2021 17:13

Add E2E test to ensure muted message is displayed in correct place

3755725

Update muted notice text

9c80afd

Merge pull request #6573 from ampproject/bug/4635-disable-mute-when-a…

602eb7e

…utoplay-2 Add muted message via direct DOM manipulation

delawski requested review from pierlon and westonruter August 30, 2021 15:19

westonruter added 2 commits August 30, 2021 08:40

Fix video_with_autoplay test after #6576

c329290

westonruter reviewed Aug 30, 2021

View reviewed changes

assets/src/block-editor/helpers/index.js Outdated Show resolved Hide resolved

westonruter reviewed Aug 30, 2021

View reviewed changes

assets/src/block-editor/helpers/index.js Outdated Show resolved Hide resolved

Apply setUpInspectorControls to MEDIA_BLOCKS

bbda4bd

Co-authored-by: Weston Ruter <[email protected]>

westonruter changed the title ~~Check mute toggle when autoplay is enable~~ Check mute toggle when autoplay is enable and show notice when attempting to autoplay unmuted video Aug 30, 2021

westonruter modified the milestones: v2.1.4, v2.2 Aug 30, 2021

westonruter mentioned this pull request Aug 30, 2021

Remove muted attribute from amp-video and its video fallback when autoplay is present #6581

Merged

westonruter and others added 2 commits August 30, 2021 12:48

Add test case for when panel is toggled

2ca8cc7

Use MutationObserver for toggling "Muted" message in Video block

ad01cdd

delawski requested a review from westonruter September 1, 2021 20:59

westonruter added the P2 Low priority label Sep 2, 2021

westonruter modified the milestones: v2.2, v2.3 Sep 4, 2021

westonruter modified the milestones: v2.3, v2.4 Dec 23, 2021

westonruter removed this from the v2.4 milestone Jul 28, 2022

westonruter closed this Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check mute toggle when autoplay is enable and show notice when attempting to autoplay unmuted video #6547

Check mute toggle when autoplay is enable and show notice when attempting to autoplay unmuted video #6547

dhaval-parekh commented Aug 18, 2021 •

edited

Loading

codecov bot commented Aug 23, 2021 •

edited

Loading

github-actions bot commented Aug 23, 2021 •

edited

Loading

delawski left a comment

pierlon left a comment •

edited

Loading

pierlon Aug 26, 2021

pierlon commented Aug 27, 2021

delawski commented Aug 27, 2021

westonruter Aug 30, 2021

pierlon commented Aug 30, 2021

westonruter commented Aug 30, 2021

westonruter commented Aug 30, 2021

delawski commented Aug 31, 2021

westonruter commented Sep 1, 2021

delawski commented Sep 1, 2021

-		if ( isset( $out['autoplay'] ) ) {
+		// The amp-video will forcibly be muted whenever it is set to autoplay, so omit the
+		// `muted` attribute if it exists.
+		if ( isset( $out['autoplay'], $out['muted'] ) ) {

	inspectorControls = isSelected ? VideoInspectorControls( props ) : null;
	inspectorControls = isSelected ? VideoInspectorControls( props ) : setUpInspectorControls( props );

Check mute toggle when autoplay is enable and show notice when attempting to autoplay unmuted video #6547

Check mute toggle when autoplay is enable and show notice when attempting to autoplay unmuted video #6547

Conversation

dhaval-parekh commented Aug 18, 2021 • edited Loading

Summary

Checklist

codecov bot commented Aug 23, 2021 • edited Loading

Codecov Report

github-actions bot commented Aug 23, 2021 • edited Loading

delawski left a comment

Choose a reason for hiding this comment

pierlon left a comment • edited Loading

Choose a reason for hiding this comment

pierlon Aug 26, 2021

Choose a reason for hiding this comment

pierlon commented Aug 27, 2021

delawski commented Aug 27, 2021

westonruter Aug 30, 2021

Choose a reason for hiding this comment

pierlon commented Aug 30, 2021

westonruter commented Aug 30, 2021

westonruter commented Aug 30, 2021

delawski commented Aug 31, 2021

westonruter commented Sep 1, 2021

delawski commented Sep 1, 2021

dhaval-parekh commented Aug 18, 2021 •

edited

Loading

codecov bot commented Aug 23, 2021 •

edited

Loading

github-actions bot commented Aug 23, 2021 •

edited

Loading

pierlon left a comment •

edited

Loading