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.

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

馃悰Do not select hidden elements in amp-selector #23735

Merged
merged 3 commits into from Aug 7, 2019

Conversation

@cvializ
Copy link
Contributor

commented Aug 6, 2019

Fixes #19914

@cvializ cvializ requested a review from sparhami Aug 6, 2019

@googlebot googlebot added the cla: yes label Aug 6, 2019

if (!isElementHidden(this.elements_[this.focusedIndex_])) {
break;
}
} while (this.focusedIndex_ != originalIndex);

This comment has been minimized.

Copy link
@sparhami

sparhami Aug 6, 2019

Contributor

What happens here if the original index is the only non-hidden index? Seems like focus goes to a hidden element.

Would suggest not updating this.focusedIndex_ until just before the break.

This comment has been minimized.

Copy link
@cvializ

cvializ Aug 6, 2019

Author Contributor

I believe it would go back to the original element if it's the only non-disabled one

@amp-bundle-size amp-bundle-size bot requested a review from danielrozenberg Aug 6, 2019

@danielrozenberg danielrozenberg removed their request for review Aug 7, 2019

@cvializ cvializ merged commit 63def21 into ampproject:master Aug 7, 2019

14 of 15 checks passed

ampproject/pr-deploy Ready to create a test site.
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
ampproject/bundle-size 螖 -0.01KB | no approval necessary
Details
ampproject/tests/e2e (local) 308 tests passed
Details
ampproject/tests/integration (local) 304 tests passed
Details
ampproject/tests/integration (saucelabs) 449 tests passed
Details
ampproject/tests/integration (single-pass) 245 tests passed
Details
ampproject/tests/unit (local) 10056 tests passed
Details
ampproject/tests/unit (local-changes) 180 tests passed
Details
ampproject/tests/unit (saucelabs) 7841 tests passed
Details
cla/google All necessary CLAs are signed
codecov/patch 73.91% of diff hit (within 100% threshold of 100%)
Details
codecov/project 79.28% (+0.06%) compared to 7e27f4e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/amphtml Visual review automatically approved, no visual changes found.
Details
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
馃悰Do not select hidden elements in amp-selector (ampproject#23735)
* Do not select hidden elements in amp-selector

* Convert measurement to async

* Fix return type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.