Add AMP.navigateTo action #9932
Conversation
dreamofabear
changed the title
|
/to @aghassemi @cvializ PTAL |
| @@ -229,8 +228,7 @@ export class ActionService { | |||
| if (!event.defaultPrevented && | |||
| element.getAttribute('role') == 'button') { | |||
| event.preventDefault(); | |||
| // TODO(choumx, #9699): HIGH. | |||
| this.trigger(element, name, event, ActionTrust.MEDIUM); | |||
| this.trigger(element, name, event, ActionTrust.HIGH); | |||
There was a problem hiding this comment.
@jridgewell FYI I think this is the correct trust level for clicking a button via keyboard.
| if (!invocation.satisfiesTrust(ActionTrust.HIGH)) { | ||
| return; | ||
| } | ||
| const url = invocation.args['url']; |
There was a problem hiding this comment.
use isProtocolValid from url.js to assert that url is not dangerous (also a test for navigateTo(javascript:) to ensure the validation.
There was a problem hiding this comment.
Would an error message here be better, or would you prefer it silently fail?
There was a problem hiding this comment.
Good idea. Added a user error.
src/service/standard-actions-impl.js
Outdated
| const expandedUrl = this.urlReplacements_.expandUrlSync(url); | ||
| const node = invocation.target; | ||
| const win = (node.ownerDocument || node).defaultView; | ||
| win.location = expandedUrl; |
There was a problem hiding this comment.
do we wanna take an optional target?
There was a problem hiding this comment.
For nested iframes? I haven't heard this particular FR and it can be accomplished by binding to iframe[src] with amp-bind. We could extend this to myIframeId.navigateTo(url=) if requested.
There was a problem hiding this comment.
Mostly was thinking about blank rather than nested case.
There was a problem hiding this comment.
Oh I see. Looks like window.open() has a few additional params and doesn't support opening in new tabs. If it's alright with you, I'd like to defer this until we receive a FR so we can spec it appropriately.
| @@ -125,6 +131,21 @@ export class StandardActions { | |||
| * @param {!./action-impl.ActionInvocation} invocation | |||
| * @private | |||
| */ | |||
| handleAmpNavigateTo_(invocation) { | |||
| if (!invocation.satisfiesTrust(ActionTrust.HIGH)) { | |||
| return; | |||
There was a problem hiding this comment.
Maybe a user().error() log here?
There was a problem hiding this comment.
Already happens inside satisfiesTrust.
| @@ -125,6 +131,21 @@ export class StandardActions { | |||
| * @param {!./action-impl.ActionInvocation} invocation | |||
| * @private | |||
| */ | |||
| handleAmpNavigateTo_(invocation) { | |||
| if (!invocation.satisfiesTrust(ActionTrust.HIGH)) { | |||
| return; | |||
There was a problem hiding this comment.
Already happens inside satisfiesTrust.
| if (!invocation.satisfiesTrust(ActionTrust.HIGH)) { | ||
| return; | ||
| } | ||
| const url = invocation.args['url']; |
src/service/standard-actions-impl.js
Outdated
| const expandedUrl = this.urlReplacements_.expandUrlSync(url); | ||
| const node = invocation.target; | ||
| const win = (node.ownerDocument || node).defaultView; | ||
| win.location = expandedUrl; |
There was a problem hiding this comment.
For nested iframes? I haven't heard this particular FR and it can be accomplished by binding to iframe[src] with amp-bind. We could extend this to myIframeId.navigateTo(url=) if requested.
dreamofabear
force-pushed
the
navigate-to
branch
from
b8ba5a3
to
143d929
Compare
| if (!invocation.satisfiesTrust(ActionTrust.HIGH)) { | ||
| return; | ||
| } | ||
| const url = invocation.args['url']; |
There was a problem hiding this comment.
Would an error message here be better, or would you prefer it silently fail?
Fixes #8976, partial for #9699.
AMP.navigateTo(url=)action -- requires high trust and supports var subs.tapandchange(from<select>) events to high trust.ActionService.execute().Documentation changes in #9933.