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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 [Story clicks] Make grid layer content not click shield other elements #38377

Merged
merged 5 commits into from Oct 25, 2022
Merged

馃悰 [Story clicks] Make grid layer content not click shield other elements #38377

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
787c4f5
Made grid layer content not click shield other elements
mszylkowski Jul 29, 2022
4a8e731
Update extensions/amp-story/1.0/amp-story.css
mszylkowski Aug 1, 2022
747f708
Added example of unclickable div on top of quiz
mszylkowski Sep 23, 2022
2ac53f0
Merge branch 'ampproject:main' into no_click_shield
mszylkowski Sep 23, 2022
e080153
Merge branch 'ampproject:main' into no_click_shield
mszylkowski Oct 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions extensions/amp-story-360/0.1/amp-story-360.css
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@ amp-story-360 amp-img {
display: flex !important;
align-items: center !important;
z-index: 100000 !important;
pointer-events: auto !important;
}

.i-amphtml-story-360-activate-button-icon {
Expand Down
1 change: 1 addition & 0 deletions extensions/amp-story-shopping/0.1/amp-story-shopping-tag.css
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ amp-story-shopping-tag {
width: auto !important;
color: white !important;
position: absolute !important;
pointer-events: auto !important;
/* Height and width ensures layoutCallback is called. */
min-height: 1px !important;
min-width: 1px !important;
Expand Down
3 changes: 2 additions & 1 deletion extensions/amp-story/1.0/amp-story.css
View file
Open in desktop
Expand Up @@ -337,7 +337,8 @@ amp-story-page[active]:not(.i-amphtml-layout) amp-video.i-amphtml-poolbound:not(
display: none !important;
}

amp-story-grid-layer > * {
/** Make clickable elements in light dom accept pointer events */
amp-story-grid-layer a, amp-story-grid-layer amp-twitter {
pointer-events: auto !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

This would reset all publisher CSS incorrectly setting pointer-events, removing it is pretty risky and impossible to test :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only 2 values of pointer-events: auto and none. If they manually set it to auto, they receive the same behavior as is currently happening (noop). If they manually set it to none the clickable elements will override that property and catch clicks as expected (improvement if added to foreground layers that are "covering" clickable elements).

I don't believe there's a scenario where pubs can incorrectly set the pointer events to break the story. The system layer is a separate layer not in the grid-layer so it doesn't get affected, and all clickable elements override the pointer-events: auto so they can be clicked

Copy link
Contributor

Choose a reason for hiding this comment

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

This solution does not scale well and is so easy to break. Each developer needs to set its own pointer-events if they want to receive clicks?
You forgot some, introducing bugs already: e.g. i-amphtml-story-page-play-button and maybe some others that we're forgetting about

Copy link
Contributor

Choose a reason for hiding this comment

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

Another example: people using button elements to trigger flows, e.g. consent for CCPA implementations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i-amphtml-story-page-play-button is not added to the inside of a grid-layer, therefore is not affected by this at all. Only the elements inside grid-layers are affected (look at the CSS selectors). Amp consent is on a button outside the grid-layer so doesn't get affected. User-created buttons are not allowed inside grid-layers. You should only look at the valid list of components under "amp-story-grid-layer-allowed-descendants".

This solution does not scale well

The reason why this doesn't scale is because we (AMP devs) take the burden of making it work, instead of relying on creation tools to add the right values. We do the same for many things, and if someone sends a bug we can just fix it (eg: someone says X component is not clickable -> we make a PR with the CSS rule). "Developer Experience > Ease of Implementation". The alternatives are worse for DevX or UX:

  • add a new template: the template would still need us to force the pointer events on clickable elements, otherwise they would be non-clickable. Otherwise, if we disallow interactive elements inside this template (eg: if we make a new component similar to grid-layer) we'd need to allowlist every single non-interactive component there, which is even harder to scale.
  • Let creators set the right pointer events: it can actually break stories with interactive elements because we can't retroactively ask creators to add pointer events

What alternative would you suggest that doesn't put the burden on the creator and scales better?

Copy link
Contributor

Choose a reason for hiding this comment

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

For user using buttons, doesn't amp convert them to a tags ? If not, why not just add button to the css rule ?

The amp framework is restraining enough, we should be able to list exhaustively every possible interactions isn't it ? That way the framework would also prevent not interactive layer to prevent you from clicking, I don't see how this could be a regression as long as the list is exhaustive and tested on every possible interactive amp components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not aware of any buttons that users can add onto stories, the button tag is specifically not allowed in stories. What you refer is links (<a> tags) which we replace the functionality so they are always clickable with the change amp-story-grid-layer a {pointer-events: auto} in this PR.

Edit: I do agree that we could exhaustively list all the clickable elements in amp-story-grid-layer, which is what I'm doing in this PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

up

Copy link
Contributor

Choose a reason for hiding this comment

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

Up again, this is still a very big issue for us.

}

Expand Down