Proxying the form to call the prototype methods/properties, incl Chrome 45 #6944
Conversation
| // Exclude constants. | ||
| name.toUpperCase() == name || | ||
| // Exclude on-events. | ||
| name.substring(0, 2) == 'on') { |
There was a problem hiding this comment.
I'd use indexOf because it avoids string allocation.
There was a problem hiding this comment.
Or startsWith, which'll avoid a full string search.
| for (const name in proto) { | ||
| if (name in proxy || | ||
| // Exclude constants. | ||
| name.toUpperCase() == name || |
There was a problem hiding this comment.
Can't we use the correct case?
There was a problem hiding this comment.
What's the correct case?
There was a problem hiding this comment.
Oh, never mind. I read this wrong.
| const element = dev().assertElement(current); | ||
| const nextSibling = element.nextSibling; | ||
| const parent = element.parentNode; | ||
| parent.removeChild(element); |
There was a problem hiding this comment.
This is completely insane. Just saying.
| if (desc.toggle) { | ||
| value = (value != null); | ||
| } else { | ||
| value = (value === 'true'); |
There was a problem hiding this comment.
Isn't truthiness for attributes based on presence?
| } | ||
| } | ||
| if (value && desc.url) { | ||
| value = parseUrl(/** @type {string} */ (value)).href; |
| // Exclude constants. | ||
| name.toUpperCase() == name || | ||
| // Exclude on-events. | ||
| name.substring(0, 2) == 'on') { |
There was a problem hiding this comment.
Or startsWith, which'll avoid a full string search.
| value = (value === 'true'); | ||
| } | ||
| } | ||
| if (value && desc.url) { |
There was a problem hiding this comment.
Should this be joined as an else if?
There was a problem hiding this comment.
Yes. I redid it to communicate exlcusivity more clearly.
| dev().assert(false, 'unknown property access type: %s', desc.access); | ||
| } | ||
| } else { | ||
| // Not currently overriden by an input. |
There was a problem hiding this comment.
I don't understand the comment. How does not having a desc mean there's no overriding input?
There was a problem hiding this comment.
Updated. Not sure what I was thinking, but that comment makes 0 sense.
| * def: *, | ||
| * }>} | ||
| */ | ||
| const LEGACY_PROPS = { |
There was a problem hiding this comment.
How was this list compiled?
There was a problem hiding this comment.
Existing properties in Node/Element/HTMLElement/HTMLFormElement prototype chain minus properties we definitely disabled in validator. For instance, this list excludes things like offsetLeft. These are always properties, not methods.
|
@cramforce @jridgewell PTAL. |
| * @param {!Object} proxy | ||
| */ | ||
| function setupLegacyProxy(form, proxy) { | ||
| const proto = form.cloneNode(/* deep */ false); |
There was a problem hiding this comment.
#cloneDeep can be overridden. HTMLFormElement.prototype.cloneNode.call(form)?
| // Exclude constants. | ||
| name.toUpperCase() == name || | ||
| // Exclude on-events. | ||
| name.startsWith('on')) { |
There was a problem hiding this comment.
You have to import our startsWith helper, #startsWith is only in recent ES6 browsers.
| // A property such as `style`. The only way is to read this value | ||
| // once and use it for all subsequent calls. | ||
| let actual; | ||
| if (isElement) { |
There was a problem hiding this comment.
Can we move the isElement variable to here?
…me 45 (ampproject#6944) * Proxying the form to call the prototype methods/properties, incl Chrome 45 support * Support legacy properties * review fixes * review fixes
Rollforward of #6720 including changes necessary for Chrome 45 and below.
The only difference from the previously rolled back #6720 is concentrated in the
setupLegacyProxymethod.