🐛 propagate title attribute in amp-iframe #27037
Conversation
|
Hey @ampproject/wg-caching, these files were changed: |
This reverts commit 2541fa3.
maxlever
changed the title
title attribute to amp-iframe validator
maxlever
force-pushed
the
amp-iframe-title
branch
from
4003820 to
e5828e7
Compare
honeybadgerdontcare
added
the
WG: Caching: Triaged
| @@ -618,6 +619,9 @@ export class AmpIframe extends AMP.BaseElement { | |||
| ); | |||
| } | |||
| } | |||
| if (this.iframe_) { | |||
| this.propagateAttributes(['title'], this.iframe_); | |||
There was a problem hiding this comment.
i would ideally propagate all ATTRIBUTES_TO_PROPAGATE, but this was causing an e2e test error when I tried that, so only propagating title on mutate.
There was a problem hiding this comment.
A comment here indicating why only title is propagated would be nice.
maxlever
changed the title
title attribute to amp-iframe validator
maxlever
changed the title
| @@ -46,6 +46,7 @@ const TAG_ = 'amp-iframe'; | |||
|
|
|||
| /** @const {!Array<string>} */ | |||
| const ATTRIBUTES_TO_PROPAGATE = [ | |||
| 'title', | |||
There was a problem hiding this comment.
Nit: The rest of these appear to be alphabetised. Might want to keep the pattern up.
| @@ -235,7 +236,8 @@ describes.realWin( | |||
| expect(iframe.getAttribute('referrerpolicy')).to.equal('no-referrer'); | |||
| expect(iframe.getAttribute('frameborder')).to.equal('3'); | |||
| expect(iframe.getAttribute('tabindex')).to.equal('-1'); | |||
| // unsupproted attributes | |||
| expect(iframe.getAttribute('title')).to.equal('example title'); | |||
There was a problem hiding this comment.
Can we also add a test for when the title is mutated since this change is propagating it?
| @@ -618,6 +619,9 @@ export class AmpIframe extends AMP.BaseElement { | |||
| ); | |||
| } | |||
| } | |||
| if (this.iframe_) { | |||
| this.propagateAttributes(['title'], this.iframe_); | |||
There was a problem hiding this comment.
A comment here indicating why only title is propagated would be nice.
|
Approved the visual diff change, and will look at the test addition in a few minutes. |
|
@maxlever You might want to pull the |
|
@kristoferbaxter i removed the update: split |
Improve a11y by propagating
titleattribute from amp-iframe to iframe. Resolves #17308.