ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

amp-script touchstart, touchmove, touchend do not register as interaction add Feature #33557

Open andrei0x309 opened 3 years ago

andrei0x309 commented 3 years ago

New Feature

Existing code: https://github.com/ampproject/amphtml/blob/d2999e1b4776c438059695d7d525f2e036154e28/extensions/amp-script/0.1/user-activation-tracker.js#L18-L28

New Code:

const ACTIVATION_EVENTS = [
  'touchstart',
  'touchmove', 
  'touchend',
  'change',
  'click',
  'dblclick',
  'input',
  'keypress',
  'submit',
  'keydown',
];

Problem

amp-script will block mutation, because touchstart, touchmove, touchend do not count as interaction.

This makes it impossible to implement things that open/close by swipe, outside of a fixed layout.

Additional context

Should I open a PR with the suggested changes?

andrei0x309 commented 3 years ago

And maybe for performance don't include touchmove, probably touchstart and touchend is enough (if someone makes a really slow gesture over 5 sec.)

alanorozco commented 3 years ago

Hi @andrei0x309

touch events can be captured by an AMP viewer, since it owns higher-level scrolling and may allow sliding to navigate across documents. It's therefore not trivial to allow these events. Expanding this allowlist will surely work when accessing the document directly, but won't work when loaded inside a viewer.

@samouri or @kristoferbaxter may have more input on the matter.

andrei0x309 commented 3 years ago

@alanorozco

Question: Doesn't SGX disable the AMP Viewer?

Most sites have SGX enabled I didn't see a single instance of AMP Viewer on AMP-enabled websites in a very long time.

alanorozco commented 3 years ago

Most sites have SGX enabled I didn't see a single instance of AMP Viewer on AMP-enabled websites in a very long time.

iOS doesn't support Signed Exchanges.

andrei0x309 commented 3 years ago

iOS doesn't support Signed Exchanges.

I guess the dev could just omit the swipe feature, based on navigator.platform if is IOS.

I guess this feature would be used mainly for closing a custom sidebar, which usually will have a close button so in case your device doesn't support SGX you'll just have to tap the close button.

On the other hand, I thought the bigger problem would be that on any mobile device you can't scroll without a swipe, which means that every scroll will count as an interaction and allow DOM modification, which for many developers it sounds good, but I don't know if that sounds good for Google vision of AMP.

alanorozco commented 3 years ago

I thought the bigger problem would be that on any mobile device you can't scroll without a swipe, which means that every scroll will count as an interaction and allow DOM modification

Yes, the event type restriction guards against problems like CLS.

I'll let @kristoferbaxter or @samouri take this on.