Implement anchor href expand on document click with X/Y support#4773
Merged
jridgewell merged 54 commits intoampproject:masterfrom Sep 15, 2016
google:a4a_clickUrlExpand
Merged
Implement anchor href expand on document click with X/Y support#4773jridgewell merged 54 commits intoampproject:masterfrom google:a4a_clickUrlExpand
jridgewell merged 54 commits intoampproject:masterfrom
google:a4a_clickUrlExpand
Conversation
Merged
…nt target rewrite for shadowDom elements; was not binding whether capture to click handler properly. Test coverage for fixes to be added shortly
…y offset, look for existence of event.path and use event.target which should match host element
Member
|
Will let @jridgewell do another round of review, but LGTM from me. |
| * @visibleForTesting | ||
| */ | ||
| export function getElementByTagNameFromEventShadowDomPath(e, tagName) { | ||
| for (let i = 0; i < (e.path ? e.path.length : 0); i++) { |
Contributor
There was a problem hiding this comment.
We can check outside for e.path the for loop, so that we don't deopitmize it.
The previous change was made in error (wrong branch).
|
Randomly, I just noticed that this PR doesn't extend the variable substitution docs. Probably worth adding docs on your new click X/Y support (and its specialized behavior for shadow DOM and iframes and so on) to that. |
src/service/url-replacements-impl.js
Outdated
| if (opt_sync) { | ||
| user().error('ignoring promise value for key: ' + name); | ||
| return ''; | ||
| } else { |
Contributor
There was a problem hiding this comment.
This else is unnecessary since the if before it returns.
Contributor
Author
|
Updated variable substitution docs. Please give it a read - @tdrl @cramforce @jridgewell - back to you for review. Thanks |
Contributor
Author
|
@jridgewell I have rebased from master. Please let me know if you require anything else otherwise I will assume you will merge? Thanks! |
src/document-click.js
Outdated
| } | ||
|
|
||
| /** | ||
| <<<<<<< 848be7f9dffbffe8922cb19af504dc939caf6a17 |
Contributor
|
You've got some linting errors: |
Contributor
Author
|
@jridgewell lint and conflict errors fixed. Sorry about that. |
Contributor
|
Not a problem, merge conflicts always suck to get right. Typehints and test errors now: |
Contributor
Author
|
PTAL - lint, type check, and test errors fixed. @jridgewell let me know if you require anything else. |
Closed
dvoytenko
reviewed
Sep 15, 2016
| return viewerFor(this.win_).getReferrerUrl(); | ||
| this.setAsync_('DOCUMENT_REFERRER', () => { | ||
| return viewerFor(this.win_).getReferrerUrl().then(referrer => { | ||
| return referrer === undefined ? '' : referrer; |
Contributor
There was a problem hiding this comment.
Could you please clarify, what's the purpose of this? Did we disallow undefined values downstream?
Contributor
Author
There was a problem hiding this comment.
No, in retrospect this should not have been necessary. I was debating making the result explicitly string given we need to replace the macro but after some thought decided null/undefined was equivalent to empty string. Should you decide to roll this back, I will fix. Otherwise I can address in a subsequent PR.
Contributor
|
This PR causes significant test failures. I'm planning to roll it back. Let me know if it's in a critical path. |
avimehta
added a commit
to avimehta/amphtml
that referenced
this pull request
Sep 15, 2016
This was broken because of ampproject#4773. Fixes ampproject#5043
Closed
Contributor
Author
|
Feel free to roll back. Apologies, I will address failures. Not sure how they slipped through given I ran the full test suite locally. Must not have caught the expand to expandAsync name change. |
erwinmombay
added a commit
to erwinmombay/amphtml
that referenced
this pull request
Sep 15, 2016
…rt (ampproject#4773)" This reverts commit fa58235.
dreamofabear
pushed a commit
to dreamofabear/amphtml
that referenced
this pull request
Sep 16, 2016
…roject#4773) * Implement anchor href expand on document click with X/Y support * Fixes after full stack testing: need to look at event path due to event target rewrite for shadowDom elements; was not binding whether capture to click handler properly. Test coverage for fixes to be added shortly * update tests; instead of looking for shadowRoot boundary to update x/y offset, look for existence of event.path and use event.target which should match host element * slight refactor for readability * fix javadoc error * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * changes in response to PR review * update var substititions documentation * make capture listener actually use capture! * remove unnecessary else clause * fix pull conflicts * Fixes after full stack testing: need to look at event path due to event target rewrite for shadowDom elements; was not binding whether capture to click handler properly. Test coverage for fixes to be added shortly * update tests; instead of looking for shadowRoot boundary to update x/y offset, look for existence of event.path and use event.target which should match host element * slight refactor for readability * fix javadoc error * changes in response to PR review * update var substititions documentation * make capture listener actually use capture! * remove unnecessary else clause * fix test failure due to issue after merge * address PR comments * Implement anchor href expand on document click with X/Y support * Fixes after full stack testing: need to look at event path due to event target rewrite for shadowDom elements; was not binding whether capture to click handler properly. Test coverage for fixes to be added shortly * update tests; instead of looking for shadowRoot boundary to update x/y offset, look for existence of event.path and use event.target which should match host element * slight refactor for readability * fix javadoc error * changes in response to PR review * update var substititions documentation * make capture listener actually use capture! * remove unnecessary else clause * fix pull conflicts * Fixes after full stack testing: need to look at event path due to event target rewrite for shadowDom elements; was not binding whether capture to click handler properly. Test coverage for fixes to be added shortly * update tests; instead of looking for shadowRoot boundary to update x/y offset, look for existence of event.path and use event.target which should match host element * slight refactor for readability * changes in response to PR review * fix test failure due to issue after merge * Revert "reenable history tests (ampproject#4758)" This reverts commit 1375883. * Revert "Reverting doc changes" This reverts commit 8d8c6dd. * Revert "Small Validator Updates, including blacklisting __amp_source_origin for URLs. (ampproject#4781)" This reverts commit bffeb2d. * Revert "Remove video-id from the amp-youtube extension runtime. This attr had (ampproject#4507)" This reverts commit 8d1799d. * Revert unintentionally added files. * Revert unintentionally added file. * Fix test failure from latest merge * Fix conflict and lint errors * Fix lint errors, type check failures, and test failures
dreamofabear
pushed a commit
to dreamofabear/amphtml
that referenced
this pull request
Sep 16, 2016
…rt (ampproject#4773)" (ampproject#5048) This reverts commit fa58235.
Contributor
|
@keithwrightbos I'll make couple of notes now and please make sure I get a chance to glance over the review for the updated PR. For information, here's the fix Avi put together for the failing tests https://github.com/ampproject/amphtml/pull/5046/files. |
dvoytenko
reviewed
Sep 16, 2016
| return createIframePromise().then(iframe => { | ||
| installUrlReplacementsService(iframe.win); | ||
| const replacements = urlReplacementsFor(iframe.win); | ||
| Math.random = function() { return 135; }; |
Contributor
There was a problem hiding this comment.
Please use sandbox.stub() for this. Otherwise, Math.random() will be replaced for all subsequent tests.
mityaha
pushed a commit
to brightcove-archive/ooyala_amphtml
that referenced
this pull request
Nov 30, 2016
…roject#4773) * Implement anchor href expand on document click with X/Y support * Fixes after full stack testing: need to look at event path due to event target rewrite for shadowDom elements; was not binding whether capture to click handler properly. Test coverage for fixes to be added shortly * update tests; instead of looking for shadowRoot boundary to update x/y offset, look for existence of event.path and use event.target which should match host element * slight refactor for readability * fix javadoc error * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * changes in response to PR review * update var substititions documentation * make capture listener actually use capture! * remove unnecessary else clause * fix pull conflicts * Fixes after full stack testing: need to look at event path due to event target rewrite for shadowDom elements; was not binding whether capture to click handler properly. Test coverage for fixes to be added shortly * update tests; instead of looking for shadowRoot boundary to update x/y offset, look for existence of event.path and use event.target which should match host element * slight refactor for readability * fix javadoc error * changes in response to PR review * update var substititions documentation * make capture listener actually use capture! * remove unnecessary else clause * fix test failure due to issue after merge * address PR comments * Implement anchor href expand on document click with X/Y support * Fixes after full stack testing: need to look at event path due to event target rewrite for shadowDom elements; was not binding whether capture to click handler properly. Test coverage for fixes to be added shortly * update tests; instead of looking for shadowRoot boundary to update x/y offset, look for existence of event.path and use event.target which should match host element * slight refactor for readability * fix javadoc error * changes in response to PR review * update var substititions documentation * make capture listener actually use capture! * remove unnecessary else clause * fix pull conflicts * Fixes after full stack testing: need to look at event path due to event target rewrite for shadowDom elements; was not binding whether capture to click handler properly. Test coverage for fixes to be added shortly * update tests; instead of looking for shadowRoot boundary to update x/y offset, look for existence of event.path and use event.target which should match host element * slight refactor for readability * changes in response to PR review * fix test failure due to issue after merge * Revert "reenable history tests (ampproject#4758)" This reverts commit 1375883. * Revert "Reverting doc changes" This reverts commit 8d8c6dd. * Revert "Small Validator Updates, including blacklisting __amp_source_origin for URLs. (ampproject#4781)" This reverts commit bffeb2d. * Revert "Remove video-id from the amp-youtube extension runtime. This attr had (ampproject#4507)" This reverts commit 8d1799d. * Revert unintentionally added files. * Revert unintentionally added file. * Fix test failure from latest merge * Fix conflict and lint errors * Fix lint errors, type check failures, and test failures
mityaha
pushed a commit
to brightcove-archive/ooyala_amphtml
that referenced
this pull request
Nov 30, 2016
…rt (ampproject#4773)" (ampproject#5048) This reverts commit fa58235.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementation of document click listener expanding anchor target href including new X/Y click location. See I2I #3665 Enforces sync/async via type check though source wide check is not currently enabled.
Refer to original PR #4153 which was abandoned due to long period between action (cleaner to start new PR).