amp-bind: Restore canBind() #8272
Conversation
dreamofabear
force-pushed
the
bind-revert-can-bind
branch
from
546d1da
to
949ca33
Compare
|
/to @jridgewell @kmh287 /cc @molnarg |
| @@ -33,9 +34,23 @@ let PropertyRulesDef; | |||
| * @private {Object<string, ?PropertyRulesDef>} | |||
| */ | |||
| const GLOBAL_PROPERTY_RULES = map({ | |||
| 'text': null, | |||
There was a problem hiding this comment.
Do we want to allow any tag to have the [text] attribute? I could see this being potentially problematic on a <div> for instance, where reassigning its textContent could be used to remove its contents.
There was a problem hiding this comment.
Yea, I originally had only whitelisted text elements for [text] but I think a more general solution is more consistent (Node.textContent is not limited to text elements) and makes the validator changes smaller.
|
Also, depending on which PR you submit first, don't forget to add amp-iframe to the rules map 😄 |
| // If there are no rules governing this binding, return true. | ||
| if (!rules) { | ||
| // If binding to (tag, property) is not allowed, return false. | ||
| if (rules === undefined) { |
There was a problem hiding this comment.
This should be calling #canBind.
There was a problem hiding this comment.
It did at one point, but it causes a duplicate call to rulesForTagAndProperty_. This is slightly more efficient.
| @@ -33,9 +34,23 @@ let PropertyRulesDef; | |||
| * @private {Object<string, ?PropertyRulesDef>} | |||
| */ | |||
| const GLOBAL_PROPERTY_RULES = map({ | |||
| 'text': null, | |||
There was a problem hiding this comment.
Yea, I originally had only whitelisted text elements for [text] but I think a more general solution is more consistent (Node.textContent is not limited to text elements) and makes the validator changes smaller.
| // If there are no rules governing this binding, return true. | ||
| if (!rules) { | ||
| // If binding to (tag, property) is not allowed, return false. | ||
| if (rules === undefined) { |
There was a problem hiding this comment.
It did at one point, but it causes a duplicate call to rulesForTagAndProperty_. This is slightly more efficient.
* restore canBind() * disallow binding to 'name' * add brightcove and youtube * update tests * fix amp property rules
Fixes #8271.
BindValidator.canBind()with latest integrations.