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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ amp-next-page: Allow hiding of arbitrary elements in child pages #15150
✨ amp-next-page: Allow hiding of arbitrary elements in child pages #15150
Conversation
04d51e8
to
9e3af68
Compare
- Reads from hideSelectors key in config - Blacklists i-amphtml-* selectors, like the validator - Runs querySelectorAll() for all the selectors when the shadow document is attached. Generating and injecting CSS would presumably be more efficient but would need more validation/sanitisation on the selectors. AFAIK passing user input directly to `querySelectorAll` should be safe.
9e3af68
to
cc6fc3b
Compare
function assertSelectors(selectors) { | ||
selectors.forEach(selector => { | ||
BANNED_SELECTOR_PATTERNS.forEach(pattern => { | ||
user().assert(typeof selector === 'string', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user().assertString
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, and updated other usages.
win.dispatchEvent(new Event('scroll')); | ||
yield macroTask(); | ||
|
||
const shadowDoc = attachShadowDocSpy.firstCall.returnValue.ampdoc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aghassemi @peterjosling This test is causing widespread unit test failures on master
today: https://travis-ci.org/ampproject/amphtml/jobs/377848593#L2008
Can you fix this test, or skip if the fix isn't obvious?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see this repeated failure from the same set of tests:
https://travis-ci.org/ampproject/amphtml/jobs/377819689#L1977
https://travis-ci.org/ampproject/amphtml/jobs/377835829#L1977
https://travis-ci.org/ampproject/amphtml/jobs/377850058#L1977
Primary use case is hiding repeated page headers/footers.
querySelectorAll
should be safe.