Roll forward: Implement anchor href expand on document click with X/Y support by keithwrightbos · Pull Request #5053 · ampproject/amphtml

keithwrightbos · 2016-09-16T15:23:07Z

Attempt to roll forward PR #4773 which was reverted due to test failures which have subsequently been addressed.

…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

The previous change was made in error (wrong branch).

…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

…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

dvoytenko · 2016-09-19T17:17:02Z

src/service/url-replacements-impl.js

+   * Enforces synchronous evaluation.
+   * @param {string} startEvent
+   * @param {string=} endEvent
+   * @return {string|undefined} undefined if API is not available, empty string


Update return type.

dvoytenko · 2016-09-19T17:18:27Z

@keithwrightbos thanks a lot! Couple of smaller comments from my side and LGTM. I'm leaving the final LGTM to @jridgewell

keithwrightbos · 2016-09-20T13:21:19Z

@jridgewell PTAL. Note this also addresses issue #5113

jridgewell

Nitpicks left, LGTM.

jridgewell · 2016-09-20T17:55:21Z

src/service/url-replacements-impl.js

-    const timingInfo = this.win_['performance']
-        && this.win_['performance']['timing'];
+  getTimingDataAsync_(startEvent, endEvent) {
+    let metric = this.getTimingDataSync_(startEvent, endEvent);


do not need to unnecessarily wait for load

We're returning a promise, though. Even if it's resolved sync (Promise.resolve(metric)), the caller can't get it's value until ~.1ms later. Adding another sync resolved Promise (loadPromise(this.win_)) only adds another ~.1ms delay.

This can definitely be in another PR, though.

jridgewell · 2016-09-20T17:55:58Z

src/service/url-replacements-impl.js

+      // Metric is not yet available. Retry after a delay.
+      return loadPromise(this.win_).then(() => {
+        metric = this.getTimingDataSync_(startEvent, endEvent);
+        return metric === '' ? undefined : metric;


Nitpick: metric || undefined

Done, collapse to return this.getTimingDataSync_(startEvent, endEvent);

jridgewell · 2016-09-20T17:56:25Z

src/service/url-replacements-impl.js

    if (!navigationInfo || navigationInfo[attribute] === undefined) {
      // PerformanceNavigation interface is not supported or attribute is not implemented.
-      return Promise.resolve();
+      return ;


Nitpick: no spacing before ;.

keithwrightbos · 2016-09-20T19:57:43Z

@jridgewell PTAL. Thanks

jridgewell · 2016-09-20T20:00:20Z

LGTM.

@keithwrightbos

This "rollsback" ampproject#5053, since it won't cleanly revert. To rollforward, revert only this commit. /cc @keithwrightbos

@keithwrightbos

This "rollsback" #5053, since it won't cleanly revert. To rollforward, revert only this commit. /cc @keithwrightbos

A more limited (to A4A) exposure of the work in ampproject#5053. - Implements ability to provide a white list of replacements for a given operation. - Moves the click handler from a global position to A4A specific code. - Switches to simpler click position implementation (just `pageX/Y`). This is currently incorrect, but will be correct after A4A moves to friendly iframes from ShadowDOM. Should help with ampproject#5205 and ampproject#4078. Related to ampproject#4891.

…A. (#5275) A more limited (to A4A) exposure of the work in #5053. - Implements ability to provide a white list of replacements for a given operation. - Moves the click handler from a global position to A4A specific code. - Switches to simpler click position implementation (just `pageX/Y`).

…A. (ampproject#5275) A more limited (to A4A) exposure of the work in ampproject#5053. - Implements ability to provide a white list of replacements for a given operation. - Moves the click handler from a global position to A4A specific code. - Switches to simpler click position implementation (just `pageX/Y`).

… support (ampproject#5053) * 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 * Fix test failures * Remove Math.random override causing breakages in other tests * PR comment feedback * PR review changes * PR code review * PR review * Address error in issue ampproject#5113 * PR review

@keithwrightbos

This "rollsback" ampproject#5053, since it won't cleanly revert. To rollforward, revert only this commit. /cc @keithwrightbos

…A. (ampproject#5275) A more limited (to A4A) exposure of the work in ampproject#5053. - Implements ability to provide a white list of replacements for a given operation. - Moves the click handler from a global position to A4A specific code. - Switches to simpler click position implementation (just `pageX/Y`).

keithwrightbos and others added 30 commits August 30, 2016 14:20

Implement anchor href expand on document click with X/Y support

98fefdb

Fixes after full stack testing: need to look at event path due to eve…

083ed19

…nt 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/…

3eb70de

…y offset, look for existence of event.path and use event.target which should match host element

slight refactor for readability

14aba83

fix javadoc error

5bcf863

Update amp-ad-network-doubleclick-impl.md

2687820

Reverting doc changes

8d8c6dd

The previous change was made in error (wrong branch).

changes in response to PR review

0fbb1c0

update var substititions documentation

a6c0796

make capture listener actually use capture!

ef7c65b

remove unnecessary else clause

f0f2f49

fix pull conflicts

6dc446c

Fixes after full stack testing: need to look at event path due to eve…

e34364b

…nt 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/…

aee0a42

…y offset, look for existence of event.path and use event.target which should match host element

slight refactor for readability

1880a80

fix javadoc error

a6d009d

changes in response to PR review

9819e50

update var substititions documentation

f318b60

make capture listener actually use capture!

e4ca34c

remove unnecessary else clause

abb56e0

fix conflicts

3629491

fix test failure due to issue after merge

d9aeef8

Merge branch 'master' of github.com:google/amphtml

91938ab

address PR comments

5df943b

Implement anchor href expand on document click with X/Y support

d603c9d

Fixes after full stack testing: need to look at event path due to eve…

d27d93b

…nt 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/…

5e2b28b

…y offset, look for existence of event.path and use event.target which should match host element

slight refactor for readability

0a56699

fix javadoc error

2528e37

changes in response to PR review

d8c6619

dvoytenko self-assigned this Sep 19, 2016

dvoytenko added the NEEDS REVIEW label Sep 19, 2016

dvoytenko reviewed Sep 19, 2016

View reviewed changes

dvoytenko approved these changes Sep 19, 2016

View reviewed changes

keithwrightbos mentioned this pull request Sep 20, 2016

JS error on document-click #5113

Closed

keithwrightbos added 2 commits September 20, 2016 09:16

PR review

50c5c9e

Address error in issue #5113

9ea3a4f

jridgewell approved these changes Sep 20, 2016

View reviewed changes

PR review

e172c8b

jridgewell added LGTM and removed NEEDS REVIEW labels Sep 20, 2016

jridgewell merged commit 6ee4876 into ampproject:master Sep 20, 2016

jridgewell deleted the a4a_clickUrlExpand branch September 20, 2016 20:37

jridgewell mentioned this pull request Sep 23, 2016

Variable substitution in href of <a> tags? #4078

Closed

jridgewell added a commit to jridgewell/amphtml that referenced this pull request Sep 23, 2016

Rollback: anchor href expand on document click

597353e

This "rollsback" ampproject#5053, since it won't cleanly revert. To rollforward, revert only this commit. /cc @keithwrightbos

jridgewell mentioned this pull request Sep 23, 2016

Rollback: anchor href expand on document click #5197

Merged

jridgewell added a commit that referenced this pull request Sep 23, 2016

Rollback: anchor href expand on document click (#5197)

18d41e7

This "rollsback" #5053, since it won't cleanly revert. To rollforward, revert only this commit. /cc @keithwrightbos

cramforce mentioned this pull request Sep 28, 2016

Allow for white listed replacement of URL placements and use it in A4A. #5275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roll forward: Implement anchor href expand on document click with X/Y support#5053

Roll forward: Implement anchor href expand on document click with X/Y support#5053
jridgewell merged 66 commits intoampproject:masterfrom
google:a4a_clickUrlExpand

keithwrightbos commented Sep 16, 2016

Uh oh!

dvoytenko Sep 19, 2016

Uh oh!

keithwrightbos Sep 19, 2016

Uh oh!

dvoytenko commented Sep 19, 2016

Uh oh!

keithwrightbos commented Sep 20, 2016

Uh oh!

jridgewell left a comment

Uh oh!

jridgewell Sep 20, 2016 •

edited

Loading

Uh oh!

jridgewell Sep 20, 2016

Uh oh!

keithwrightbos Sep 20, 2016

Uh oh!

jridgewell Sep 20, 2016

Uh oh!

keithwrightbos Sep 20, 2016

Uh oh!

keithwrightbos commented Sep 20, 2016

Uh oh!

jridgewell commented Sep 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

keithwrightbos commented Sep 16, 2016

Uh oh!

dvoytenko Sep 19, 2016

Choose a reason for hiding this comment

Uh oh!

keithwrightbos Sep 19, 2016

Choose a reason for hiding this comment

Uh oh!

dvoytenko commented Sep 19, 2016

Uh oh!

keithwrightbos commented Sep 20, 2016

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

jridgewell Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

keithwrightbos Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

jridgewell Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

keithwrightbos Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

keithwrightbos commented Sep 20, 2016

Uh oh!

jridgewell commented Sep 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jridgewell Sep 20, 2016 •

edited

Loading