Implement A2A support #13300
Implement A2A support #13300
Conversation
dreamofabear
force-pushed
the
fix-amp-navigate-to
branch
from
aadd679
to
483cea1
Compare
dreamofabear
force-pushed
the
fix-amp-navigate-to
branch
2 times, most recently
from
4f93c42
to
9dade4d
Compare
| * Lazy-generated list of A2A-enabled navigation features. | ||
| * @private {?Array<string>} | ||
| */ | ||
| this.a2aFeatures_ = null; |
There was a problem hiding this comment.
I thought we wanted to avoid calling it a2a?
There was a problem hiding this comment.
Externally, yes. Internally I think it's fine.
| } | ||
|
|
||
| // Otherwise, perform normal behavior of navigating the top frame. | ||
| win.top.location.href = url; |
There was a problem hiding this comment.
Why doesn't this handle custom protocols, too?
There was a problem hiding this comment.
I considered this too. amp-form by spec only allows https protocol, but we maybe should add this to AMP.navigateTo. Leaving this and the other <a> behaviors (getExtraParamsUrl, URL expansion, etc.) for future work.
There was a problem hiding this comment.
Please make an issue for it.
src/service/document-click.js
Outdated
| const meta = this.rootNode_.querySelector( | ||
| 'meta[name="amp-to-amp-navigation"]'); | ||
| this.a2aFeatures_ = (meta && meta.hasAttribute('content')) | ||
| ? meta.getAttribute('content').split(',').map(s => s.trim()) |
There was a problem hiding this comment.
We can remove the #map by splitting whitespace: content.trim().split(/\s*,\s*/)
There was a problem hiding this comment.
I'd prefer to leave as is for readability.
src/service/document-click.js
Outdated
| 'meta[name="amp-to-amp-navigation"]'); | ||
| this.a2aFeatures_ = (meta && meta.hasAttribute('content')) | ||
| ? meta.getAttribute('content').split(',').map(s => s.trim()) | ||
| : []; |
There was a problem hiding this comment.
Nit: split this into if/else.
src/service/document-click.js
Outdated
| ? meta.getAttribute('content').split(',').map(s => s.trim()) | ||
| : []; | ||
| } | ||
| if (this.a2aFeatures_.indexOf(opt_requestedBy) >= 0) { |
There was a problem hiding this comment.
Array#includes is polyfilled.
src/service/document-click.js
Outdated
| */ | ||
| handleA2AClick_(e, target, location) { | ||
| const relations = (target.hasAttribute('rel')) | ||
| ? target.getAttribute('rel').split(' ').map(s => s.trim()) |
There was a problem hiding this comment.
Ditto splitting whitespace.
src/service/document-click.js
Outdated
| const relations = (target.hasAttribute('rel')) | ||
| ? target.getAttribute('rel').split(' ').map(s => s.trim()) | ||
| : []; | ||
| if (relations.indexOf('amphtml') < 0) { |
dreamofabear
force-pushed
the
fix-amp-navigate-to
branch
from
d97ce0a
to
393a959
Compare
|
Tested locally with |
* squash and include a2a changes * fix lint * fix other tests * unit tests * fix lint * update amp-form tests * justin's comments
* squash and include a2a changes * fix lint * fix other tests * unit tests * fix lint * update amp-form tests * justin's comments
* squash and include a2a changes * fix lint * fix other tests * unit tests * fix lint * update amp-form tests * justin's comments
Closes #12496 and fixes #13056.
a2aviewer message and capabilityClickHandlerservice (will be renamed toNavigationafter)<a rel=amphtml>for links and<meta name="amp-to-amp-navigation" content="AMP.navigateTo, AMP-Redirect-To">for other navigational featuresTODO: Follow up with documentation and validator changes.