search

ampproject / amphtml

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

🌸 Cherry-pick request for #33433 into #33276 (Complete) #33446

Closed micajuine-ho closed 3 years ago

micajuine-ho commented 3 years ago

Cherry-pick request

Issue PR Beta / Experimental? Stable? LTS? Release issue
#33433 #33444 Yes No Yes #33276

Why does this issue meet the cherry-pick criteria?

  1. break existing AMP web pages in a significant way
  2. would otherwise cause a significant harm to AMP's reputation if left unresolved

Why is an LTS cherry-pick needed?

Bug is in LTS and we would like to fix this for publishers using LTS

List the steps to manually verify the changes in this cherry-pick

  1. Opt into a version of AMP

  2. Open dev tools console (Chrome only)

  3. Search for NYT in Google

  4. Select an AMP NYT article

  5. Find the amp-carousel at the top of page (The Coronavirus Outbreak) image

  6. Swipe on carousel and ensure that the carousel scrolls instead of new Article being shown

Mini-postmortem

TODO: This postmortem will be written after the cherry-pick deployment and before this issue is closed. Delete this TODO when the postmortem is ready.

Summary

The touchmove event within amp-carousel's scroll container was being propagated up to the viewer, causing the top stories carousel to be moved rather than the amp-carousel. This behavior had been present since October 2020 (earliest possible RTV to test with).

This feature may have regressed sometime between June 2020 (when the publisher said they implemented amp-carousel 0.2) and October 2020 (last RTV we checked). We know that this issue has not worked since October 2020 (at the AMP level).

Another possibility is that #24324 may have intentionally removed the feature where amp-carousel consumes the entire touchmove/touchstart event. No testing/documentation of this feature makes is seem like maintainers may not have been aware that this was a critical feature.

We should continue to bisect/test PRs/RTVs until we can find where the regression occurs

In any case, we should re-evaluate our process so that owners of components are aligned to the critical feature set we support. One action item towards this for this feature is adding carousels inside the viewer to our manual QA test passes for every release

Another action item could be to include the ability to test RTVs more long term than 6mo back.

Impact

Action Items

Make sure gesture based components to do not propagate these sorts of events to the viewer:

/cc @ampproject/release-on-duty @ampproject/wg-approvers @ampproject/cherry-pick-approvers

kristoferbaxter commented 3 years ago

Approved. Thanks again everyone for moving quickly to address.

rsimha commented 3 years ago

This has been CP'd and released to stable (https://github.com/ampproject/amphtml/commits/2103122145004) and LTS (https://github.com/ampproject/amphtml/commits/2102200206009).

Back to @micajuine-ho to fill out the post mortem.