Improve handling of Tumblr embeds #5926

pierlon · 2021-02-25T06:01:12Z

Summary

Changes are adapted from 21ae657.

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-02-25T06:06:45Z

Codecov Report

Merging #5926 (69e35cb) into develop (0ac7064) will increase coverage by 0.34%.
The diff coverage is 93.35%.

@@              Coverage Diff              @@
##             develop    #5926      +/-   ##
=============================================
+ Coverage      74.96%   75.31%   +0.34%     
- Complexity      5675     5703      +28     
=============================================
  Files            212      214       +2     
  Lines          17049    17242     +193     
=============================================
+ Hits           12781    12985     +204     
+ Misses          4268     4257      -11

Flag	Coverage Δ	Complexity Δ
javascript	`79.44% <96.49%> (+4.31%)`	`0.00 <0.00> (ø)`
php	`75.14% <91.44%> (+0.18%)`	`0.00 <49.00> (ø)`

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

Impacted Files	Coverage Δ	Complexity Δ
assets/src/block-editor/helpers/index.js	`39.28% <ø> (+11.42%)`	`0.00 <0.00> (ø)`
assets/src/block-validation/error/error-content.js	`93.54% <ø> (ø)`	`0.00 <0.00> (ø)`
...c/block-validation/error/get-error-source-title.js	`93.75% <ø> (ø)`	`0.00 <0.00> (ø)`
src/AmpWpPlugin.php	`100.00% <ø> (ø)`	`8.00 <0.00> (ø)`
includes/cli/class-amp-cli-validation-command.php	`18.51% <20.00%> (ø)`	`33.00 <0.00> (+1.00)`
...cludes/validation/class-amp-validation-manager.php	`81.52% <50.00%> (-0.84%)`	`309.00 <0.00> (-10.00)`
includes/embeds/class-amp-base-embed-handler.php	`48.93% <84.21%> (+24.79%)`	`21.00 <11.00> (+10.00)`
...k-validation/use-validation-error-state-updates.js	`84.50% <91.66%> (-9.25%)`	`0.00 <0.00> (ø)`
src/Validation/URLValidationRESTController.php	`92.39% <92.39%> (ø)`	`18.00 <18.00> (?)`
includes/embeds/class-amp-tumblr-embed-handler.php	`94.28% <96.96%> (+82.52%)`	`13.00 <11.00> (+8.00)`
... and 18 more

github-actions · 2021-02-25T06:10:19Z

Plugin builds for ad2e9be are ready 🛎️!

Download development build
Download production build

tests/php/test-amp-tumblr-embed-handler.php

…nes-work`

westonruter

Any idea why this Tumblr post doesn't expand like the other does?

includes/embeds/class-amp-base-embed-handler.php

includes/embeds/class-amp-tumblr-embed-handler.php

Co-authored-by: Weston Ruter <westonruter@google.com>

pierlon · 2021-02-25T21:06:26Z

Any idea why this Tumblr post doesn't expand like the other does?

It should, as the iframe document is sending a embed-size request:

window.parent.postMessage({
	sentinel: "amp",
	type: "embed-size",
	height: document.body.scrollHeight
}, "*")

Ref: https://assets.tumblr.com/client/prod/standalone/embeddable-internal/index.build.js

pierlon · 2021-02-25T21:09:18Z

From the docs on resizing amp-iframe:

Here are some factors that affect how fast the resize will be executed:

Whether the resize is triggered by the user action.

Whether the resize is requested for a currently active iframe.

Whether the resize is requested for an iframe below the viewport or above the viewport.

So it seems neither of those were happening in your case.

westonruter · 2021-02-25T21:02:01Z

includes/embeds/class-amp-tumblr-embed-handler.php

+					'overflow'   => '',
+					'tabindex'   => 0,
+					'role'       => 'button',
+					'aria-label' => __( 'See more', 'amp' ),


Why is the aria-label needed if the button text is the same?

includes/embeds/class-amp-tumblr-embed-handler.php

westonruter · 2021-02-25T21:03:37Z

includes/embeds/class-amp-tumblr-embed-handler.php

+				'div',
 				[
-					'width'   => $this->args['width'],
-					'height'  => $this->args['height'],
-					'layout'  => 'responsive',
-					'sandbox' => 'allow-scripts allow-popups', // The allow-scripts is needed to allow the iframe to render; allow-popups needed to allow clicking.
-					'src'     => $matches['href'],
-				],
-				sprintf( '<a placeholder href="%s">Tumblr</a>', $url )
+					'overflow'   => '',
+					'tabindex'   => 0,
+					'role'       => 'button',


I think we can rather use a button here. Then we won't need the role nor tabindex attributes

Done in 7df8bbe. Here's how it looks in Twenty Twenty with the button:

includes/embeds/class-amp-tumblr-embed-handler.php

includes/embeds/class-amp-base-embed-handler.php

westonruter · 2021-02-25T21:16:45Z

So it seems neither of those were happening in your case.

Oh, of course. It's because the embed was in the initial viewport.

The problem is that the overflow button is not being shown:

It needs some positioning to ensure it appears. It already has position:absolute from AMP. I think it just needs bottom:0 for example.

Co-authored-by: Weston Ruter <westonruter@google.com>

westonruter · 2021-02-26T05:27:36Z

I just checked in Twenty Twenty One and the button on hover has a transparent background:

button-background-tt1.mov

Should we consider this a theme bug?

westonruter · 2021-02-26T05:33:24Z

Also, for some reason I sometimes am seeing the button positioned outside the container making it invisible:

It's strange because I didn't see it this way when testing a bit ago. But now I am. The issue is fixed for me via 3323625.

pierlon · 2021-02-26T05:39:19Z

I just checked in Twenty Twenty One and the button on hover has a transparent background...

It seems that is intentional as the button wasn't meant to be displayed over a dark background.

I wonder if we should set a background color for the button as well.

westonruter · 2021-02-26T05:44:24Z

I wonder if we should set a background color for the button as well.

I tried setting white as the background of the button without success:

white-background-on-light-mode.mov

white-background-on-dark-mode.mov

I guess we'd have to set the color on a wrapper element instead? Or add a ::before pseudo element that is positioned absolutely behind the element? Ugh.

includes/embeds/class-amp-tumblr-embed-handler.php

westonruter · 2021-03-03T06:37:50Z

includes/embeds/class-amp-tumblr-embed-handler.php

-				],
-				sprintf( '<a placeholder href="%s">Tumblr</a>', $url )
+					'overflow' => '',
+					'style'    => 'bottom:0',


I suppose this might as well be moved to amp-default.css via this new rule:

button[overflow] { bottom: 0; }

This will have low specificity which will allow it to be easily overridden by themes/plugins.

Suggested change

'style' => 'bottom:0',

I'm no longer able to replicate the button being outside of the amp-iframe. Can you confirm if this is still needed? It seems the rule is already being set by the browser:

I'm not seeing that:

Nevertheless, I did seem to see that previously, but now I'm not.

Hmm, OK it won't hurt to add it as a rule in amp-default.css then.

In the Jetpack plugin, there is an overflow button defined:

https://github.com/Automattic/jetpack/blob/26db3e436ccca3e16a62d02d95e792a81f38a1e4/projects/plugins/jetpack/extensions/blocks/eventbrite/eventbrite.php#L104

But I don't see any styling for it: https://github.com/Automattic/jetpack/blob/26db3e436ccca3e16a62d02d95e792a81f38a1e4/projects/plugins/jetpack/extensions/blocks/eventbrite/style.scss

😕

In the overflow example on amp.dev, the positioning defaults to top:0: https://amp.dev/documentation/components/amp-list/#specifying-an-overflow

Doesn't it already have position: absolute from AMP?

Yes but that specific rule does not apply for me all the time, which is weird.

My bad, I was using a modified version of ampshared.css. So all that needs to be set as you mentioned earlier is:

button[overflow] { bottom: 0; }

Very good. Make it so.

Filed Optimizer issue upstream: ampproject/amp-toolbox#1155.

includes/embeds/class-amp-tumblr-embed-handler.php

Co-authored-by: Weston Ruter <westonruter@google.com>

…component

…ntytwentyone_styles

Sanitize Tumblr embeds

9de2cfc

pierlon added the WS:Core Work stream for Plugin core label Feb 25, 2021

pierlon added this to the v2.1 milestone Feb 25, 2021

pierlon requested a review from westonruter February 25, 2021 06:01

Fix lint errors

a414ddf

westonruter reviewed Feb 25, 2021

View reviewed changes

tests/php/test-amp-tumblr-embed-handler.php Outdated Show resolved Hide resolved

Add test for https://teded.tumblr.com/post/184736320764/how-do-vacci…

ebd31df

…nes-work`

pierlon requested a review from westonruter February 25, 2021 17:29

westonruter requested changes Feb 25, 2021

View reviewed changes

pierlon and others added 3 commits February 25, 2021 17:38

Use tagName instead of nodeName

9125cd6

Co-authored-by: Weston Ruter <westonruter@google.com>

Remove escaping of text

ca74ffa

Co-authored-by: Weston Ruter <westonruter@google.com>

Make use of tagName, use constants where possible, add phpdoc

da37307

westonruter reviewed Feb 25, 2021

View reviewed changes

pierlon and others added 3 commits February 25, 2021 21:51

Check if tumblr-post is apart of the list of classes

1475347

Co-authored-by: Weston Ruter <westonruter@google.com>

Use callback to determine if script should be removed

c330597

Use button as overflow element

7df8bbe

Add bottom:0 to Tumblr overflow button

3323625

pierlon commented Feb 26, 2021

View reviewed changes

includes/embeds/class-amp-tumblr-embed-handler.php Show resolved Hide resolved

westonruter reviewed Mar 3, 2021

View reviewed changes

Make xpath predicate more specific and improve condition formatting

8ae2574

westonruter reviewed Mar 3, 2021

View reviewed changes

includes/embeds/class-amp-tumblr-embed-handler.php Outdated Show resolved Hide resolved

westonruter and others added 3 commits March 2, 2021 22:46

Remove extraneous parentheses

005ea45

Apply style rules for AMP iframe resize button via core sanitizer

5c4b667

Fix indentation

6d4bef6

westonruter reviewed Mar 3, 2021

View reviewed changes

includes/embeds/class-amp-tumblr-embed-handler.php Show resolved Hide resolved

westonruter reviewed Mar 3, 2021

View reviewed changes

includes/embeds/class-amp-tumblr-embed-handler.php Outdated Show resolved Hide resolved

pierlon and others added 2 commits March 3, 2021 22:43

Set button type

f56f10c

Co-authored-by: Weston Ruter <westonruter@google.com>

Add style rule to place overflow button in bottom left corner of AMP …

2740d7e

…component

pierlon requested a review from westonruter March 4, 2021 04:04

westonruter added 12 commits March 3, 2021 22:02

Make sure TT1 stylesheet is enqueued before adding inline style

7da8a35

Opt to combine add_twentytwentyone_overflow_button_fix into amend_twe…

f642be8

…ntytwentyone_styles

Make sure that Tumblr embed has a link to use as a placeholder

0c6666e

Make use of MarkupComparison

685bdc9

Test removal of Tumblr script when wpautop was not involved

435b6ad

Add test for tumblr in embed block

bcbdf00

Remove extraneous sprintf()

8fa44dd

Combine conditions

c482fcb

Use xpath query to obtain the placeholder

ce2b8b4

Set placeholder attribute when we are sure we will match

77c7367

Remove extraneous sprintf()

8839cc1

Fix tests in WP 4.9

ad2e9be

westonruter approved these changes Mar 4, 2021

View reviewed changes

westonruter changed the title ~~Sanitize Tumblr embeds~~ Improve handling of Tumblr embeds Mar 4, 2021

westonruter merged commit 1a1ad95 into develop Mar 4, 2021

westonruter deleted the fix/4757-tumblr-embed-validation-error branch March 4, 2021 17:13

westonruter mentioned this pull request Mar 4, 2021

Inconsistency in placement of overflow button when SSR prepends sizer element ampproject/amp-toolbox#1155

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of Tumblr embeds #5926

Improve handling of Tumblr embeds #5926

pierlon commented Feb 25, 2021

codecov bot commented Feb 25, 2021 •

edited

Loading

github-actions bot commented Feb 25, 2021 •

edited

Loading

westonruter left a comment

pierlon commented Feb 25, 2021

pierlon commented Feb 25, 2021

westonruter Feb 25, 2021

westonruter Feb 25, 2021

pierlon Feb 26, 2021

westonruter Feb 26, 2021

westonruter commented Feb 25, 2021

westonruter commented Feb 26, 2021

westonruter commented Feb 26, 2021

pierlon commented Feb 26, 2021 •

edited

Loading

westonruter commented Feb 26, 2021

westonruter Mar 3, 2021

pierlon Mar 3, 2021

westonruter Mar 3, 2021

pierlon Mar 3, 2021

westonruter Mar 3, 2021

westonruter Mar 4, 2021

pierlon Mar 4, 2021

pierlon Mar 4, 2021

westonruter Mar 4, 2021

westonruter Mar 4, 2021

Improve handling of Tumblr embeds #5926

Improve handling of Tumblr embeds #5926

Conversation

pierlon commented Feb 25, 2021

Summary

Checklist

codecov bot commented Feb 25, 2021 • edited Loading

Codecov Report

github-actions bot commented Feb 25, 2021 • edited Loading

westonruter left a comment

Choose a reason for hiding this comment

pierlon commented Feb 25, 2021

pierlon commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Feb 25, 2021

westonruter commented Feb 26, 2021

westonruter commented Feb 26, 2021

pierlon commented Feb 26, 2021 • edited Loading

westonruter commented Feb 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 25, 2021 •

edited

Loading

github-actions bot commented Feb 25, 2021 •

edited

Loading

pierlon commented Feb 26, 2021 •

edited

Loading