🐛 [Story clicks] Make grid layer content not click shield other elements

[mszylkowski]

Closes #34632

Context: currently, grid-layers that are above others will consume the clicks (acting as click shields) for elements that are behind. In the cases where quizzes, shopping tags, or links are added on a layer below the top-most layer, they would not be clickable.

This PR keeps the grid-layers as pointer-events: none but doesn't make the children clickable (removes amp-story-grid-layer * {pointer-events: auto !important}), so we opt-in each clickable element in the light dom (which are: links, amp-twitter) and in the shadow doms (which are: interactive components (already clickable), shopping tags).

Tested that all clickable elements that could go inside grid-layer remain clickable (interactive components, shopping tags, links, amp-twitter), and also tested that the area that grid-layers don't have clickable elements remain transparent to pointer-events (can be clicked through to other layers below them)

[amp-owners-bot]

Hey @gmajoulet! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.css
extensions/amp-story-shopping/0.1/amp-story-shopping-tag.css
extensions/amp-story/1.0/amp-story.css

Hey @processprocess, @t0mg! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.css

Hey @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story.css

[gmajoulet]

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

[mszylkowski]

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

[gmajoulet]

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

[gmajoulet]

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

[mszylkowski]

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?

[edoudou]

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.

[mszylkowski]

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 :)

[edoudou]

up

[edoudou]

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

[gmajoulet]

Quick comment for @edoudou, Matias is very confident that this PR will work so we'll merge it, but I'd like to call out that all side effects it could have are hard to predict. If something goes wrong we might have to send followup fixes, or potentially (Matias believes this won't happen, but we never know) revert it.

[edoudou]

Quick comment for @edoudou, Matias is very confident that this PR will work so we'll merge it, but I'd like to call out that all side effects it could have are hard to predict. If something goes wrong we might have to send followup fixes, or potentially (Matias believes this won't happen, but we never know) revert it.

Yes I get that. I understand this is a pretty big PR (even if it does not look like it), but it also seems so important as we get feedback from client almost every week on this subject. Moreover, this solution is so much simpler for us and other tools or developers at it does not need any development or changes in our stories structure.

Thank you very much for moving ahead with this solution, let's hope Matias is right (and I am sure he is). Worth case scenario, we know amp team we'll make the fix/revert in time to be sure the fewer possible user will be affected by a bug.

[mszylkowski]

Just tested thoroughly this PR and it works properly for amp-twitter, interactive components, outlinks, page attachments, system icons. Effectively, adding any divs on layers above interactive elements now pass-through the clicks to the layers below them (exactly what we wanted).

@edoudou you shouldn't need to override the pointer-events for this PR to fix interactivity in your stories, but let us know if you see any occasion where you're tempted to override it (that could be because of a wrong CSS rule that we've set).

[ampprojectbot]

Warning: disparity between this PR Percy build and its main build

The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the main branch that this PR was merged into, and there appears to be a mismatch between the two.

This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build / main commit's Percy build > and determine further action:

• If the disparity appears to be caused by this PR, please create an bug report or send out a new PR to fix
• If the disparity appears to be a flake, please @-mention ampproject/wg-approvers in a comment
• If there is no disparity and this comment was created by mistake, please @-mention ampproject/wg-infra
• If unsure, @-mention ampproject/wg-approvers