Skip to content

Avoid swipe to dismiss errors being logged.#25139

Merged
sparhami merged 1 commit intoampproject:masterfrom
sparhami:lbg_swipe_error
Oct 18, 2019
Merged

Avoid swipe to dismiss errors being logged.#25139
sparhami merged 1 commit intoampproject:masterfrom
sparhami:lbg_swipe_error

Conversation

@sparhami
Copy link
Copy Markdown

It does not seem like this error should be causing any problems (since the error will just occur in the touch handler for the carousel, which has been nulled out due to the lightbox closing), but adding a check here to avoid logging errors.

I have not been able to reproduce the original error message through testing, but it seems like this change should avoid it.

Closes #22533

Copy link
Copy Markdown
Contributor

@cathyxz cathyxz left a comment

Choose a reason for hiding this comment

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

LGTM

* @param {!SwipeDef} data
*/
swipeGesture_(data) {
// Check if the carousel has been cleared out due to close. It is unclear
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The carousel is created on the first tap (I believe), so could it be a race condition where someone taps lightbox for the first time, and then triggers a swipe before the carousel has been initialized?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My thought is that the gestures are coming in out of order. If data.first doesn't come first, we'll have a null element reference.

Copy link
Copy Markdown
Author

@sparhami sparhami Oct 18, 2019

Choose a reason for hiding this comment

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

The carousel is created on the first tap (I believe), so could it be a race condition where someone taps lightbox for the first time, and then triggers a swipe before the carousel has been initialized?

I'm not sure, since the gesture is actually set up on the carousel element itself.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My thought is that the gestures are coming in out of order. If data.first doesn't come first, we'll have a null element reference.

Yeah that is possible, would need to investigate that possibility,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought it was the swipe element that was null, not the carousel? The swipe element gets initialized in the startSwipe, which happens when data.first is true.

@sparhami sparhami merged commit 7f81415 into ampproject:master Oct 18, 2019
@sparhami sparhami deleted the lbg_swipe_error branch October 18, 2019 21:00
jeffjose pushed a commit to jeffjose/amphtml that referenced this pull request Oct 19, 2019
joshuarrrr pushed a commit to Parsely/amphtml that referenced this pull request Oct 22, 2019
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error: Cannot read property 'style' of null

4 participants