Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxying the form to call the prototype methods/properties, incl Chrome 45 #6944

Merged
merged 4 commits into from Jan 10, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions extensions/amp-form/0.1/amp-form.js
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/

import {installFormProxy} from './form-proxy';
import {triggerAnalyticsEvent} from '../../../src/analytics';
import {isExperimentOn} from '../../../src/experiments';
import {getService} from '../../../src/service';
Expand Down Expand Up @@ -77,6 +78,8 @@ export class AmpForm {
* @param {string} id
*/
constructor(element, id) {
installFormProxy(element);

/** @private @const {string} */
this.id_ = id;

Expand Down
356 changes: 356 additions & 0 deletions extensions/amp-form/0.1/form-proxy.js
@@ -0,0 +1,356 @@
/**
* Copyright 2016 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import {dev} from '../../../src/log';
import {parseUrl} from '../../../src/url';


/**
* Blacklisted properties. Used mainly fot testing.
* @type {?Array<string>}
*/
let blacklistedProperties = null;


/**
* @param {?Array<string>} properties
* @visibleForTesting
*/
export function setBlacklistedPropertiesForTesting(properties) {
blacklistedProperties = properties;
}


/**
* Creates a proxy object `form.$p` that proxies all of the methods and
* properties to the original DOM APIs. This is to work around the problematic
* forms feature where inputs mask DOM APIs.
*
* E.g. a `<input id="id">` will override `form.id` from the original DOM API.
* Form proxy will give access to the original `id` value via `form.$p.id`.
*
* See https://medium.com/@dvoytenko/solving-conflicts-between-form-inputs-and-dom-api-535c45333ae4
*
* @param {!HTMLFormElement} form
* @return {!Object}
*/
export function installFormProxy(form) {
const constr = getFormProxyConstr(form.ownerDocument.defaultView);
const proxy = new constr(form);
if (!('action' in proxy)) {
setupLegacyProxy(form, proxy);
}
form['$p'] = proxy;
return proxy;
}


/**
* @param {!Window} win
* @return {function(new:Object, !HTMLFormElement)}
*/
function getFormProxyConstr(win) {
if (!win.FormProxy) {
win.FormProxy = createFormProxyConstr(win);
}
return win.FormProxy;
}


/**
* @param {!Window} win
* @return {function(new:Object, !HTMLFormElement)}
*/
function createFormProxyConstr(win) {

/**
* @param {!HTMLFormElement} form
* @constructor
*/
function FormProxy(form) {
/** @private @const {!HTMLFormElement} */
this.form_ = form;
}

const FormProxyProto = FormProxy.prototype;

// Hierarchy:
// Node <== Element <== HTMLElement <== HTMLFormElement
// EventTarget <== HTMLFormElement
const prototypes = [
win.HTMLFormElement.prototype,
win.HTMLElement.prototype,
win.Element.prototype,
win.Node.prototype,
win.EventTarget.prototype,
];
prototypes.forEach(function(prototype) {
for (const name in prototype) {
const property = win.Object.getOwnPropertyDescriptor(prototype, name);
if (!property ||
// Exclude constants.
name.toUpperCase() == name ||
// Exclude on-events.
name.substring(0, 2) == 'on' ||
// Exclude properties that already been created.
win.Object.prototype.hasOwnProperty.call(FormProxyProto, name) ||
// Exclude some properties. Currently only used for testing.
blacklistedProperties && blacklistedProperties.indexOf(name) != -1) {
continue;
}
if (typeof property.value == 'function') {
// A method call. Call the original prototype method via `call`.
const method = property.value;
FormProxyProto[name] = function() {
return method.apply(this.form_, arguments);
};
} else {
// A read/write property. Call the original prototype getter/setter.
const spec = {};
if (property.get) {
spec.get = function() {
return property.get.call(this.form_);
};
}
if (property.set) {
spec.set = function(value) {
return property.set.call(this.form_, value);
};
}
win.Object.defineProperty(FormProxyProto, name, spec);
}
}
});

return FormProxy;
}


/**
* This is a very heavy-handed way to support browsers that do not have
* properties defined in the prototype chain. Specifically, this is necessary
* for Chrome 45 and under.
*
* See https://developers.google.com/web/updates/2015/04/DOM-attributes-now-on-the-prototype-chain
* for more info.
*
* @param {!HTMLFormElement} form
* @param {!Object} proxy
*/
function setupLegacyProxy(form, proxy) {
const proto = form.cloneNode(/* deep */ false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#cloneDeep can be overridden. HTMLFormElement.prototype.cloneNode.call(form)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

for (const name in proto) {
if (name in proxy ||
// Exclude constants.
name.toUpperCase() == name ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use the correct case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the correct case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, never mind. I read this wrong.

// Exclude on-events.
name.substring(0, 2) == 'on') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use indexOf because it avoids string allocation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or startsWith, which'll avoid a full string search.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

continue;
}
const desc = LEGACY_PROPS[name];
const current = form[name];
const isElement = (current && current.nodeType);
if (desc) {
if (desc.access == LegacyPropAccessType.READ_ONCE) {
// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the isElement variable to here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// The overriding input, if present, has to be removed and re-added
// (renaming does NOT work).
const element = dev().assertElement(current);
const nextSibling = element.nextSibling;
const parent = element.parentNode;
parent.removeChild(element);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is completely insane. Just saying.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

try {
actual = form[name];
} finally {
parent.insertBefore(element, nextSibling);
}
} else {
actual = current;
}
Object.defineProperty(proxy, name, {
get: function() {
return actual;
},
});
} else if (desc.access == LegacyPropAccessType.ATTR) {
// An attribute-based property. We can use DOM API to read and write
// with a minimal type conversion.
const attr = desc.attr || name;
Object.defineProperty(proxy, name, {
get: function() {
let value = proxy.getAttribute(attr);
if (value == null && desc.def !== undefined) {
value = desc.def;
} else if (desc.bool) {
if (desc.toggle) {
value = (value != null);
} else {
value = (value === 'true');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't truthiness for attributes based on presence?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}
}
if (value && desc.url) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be joined as an else if?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I redid it to communicate exlcusivity more clearly.

value = parseUrl(/** @type {string} */ (value)).href;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
return value;
},
set: function(value) {
if (desc.bool && desc.toggle) {
if (value) {
value = '';
} else {
value = null;
}
}
if (value != null) {
proxy.setAttribute(attr, value);
} else {
proxy.removeAttribute(attr);
}
},
});
} else {
dev().assert(false, 'unknown property access type: %s', desc.access);
}
} else {
// Not currently overriden by an input.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the comment. How does not having a desc mean there's no overriding input?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Not sure what I was thinking, but that comment makes 0 sense.

Object.defineProperty(proxy, name, {
get: function() {
return form[name];
},
set: function(value) {
form[name] = value;
},
});
}
}
}


/**
* @enum {number}
*/
const LegacyPropAccessType = {
ATTR: 1,
READ_ONCE: 2,
};


/**
* @const {!Object<string, {
* access: !LegacyPropAccessType,
* attr: (string|undefined),
* url: (boolean|undefined),
* bool: (boolean|undefined),
* toggle: (boolean|undefined),
* def: *,
* }>}
*/
const LEGACY_PROPS = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How was this list compiled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'acceptCharset': {
access: LegacyPropAccessType.ATTR,
attr: 'accept-charset',
},
'accessKey': {
access: LegacyPropAccessType.ATTR,
attr: 'accesskey',
},
'action': {
access: LegacyPropAccessType.ATTR,
url: true,
},
'attributes': {
access: LegacyPropAccessType.READ_ONCE,
},
'autocomplete': {
access: LegacyPropAccessType.ATTR,
def: 'on',
},
'children': {
access: LegacyPropAccessType.READ_ONCE,
},
'dataset': {
access: LegacyPropAccessType.READ_ONCE,
},
'dir': {
access: LegacyPropAccessType.ATTR,
},
'draggable': {
access: LegacyPropAccessType.ATTR,
bool: true,
def: false,
},
'elements': {
access: LegacyPropAccessType.READ_ONCE,
},
'encoding': {
access: LegacyPropAccessType.READ_ONCE,
},
'enctype': {
access: LegacyPropAccessType.ATTR,
},
'hidden': {
access: LegacyPropAccessType.ATTR,
bool: true,
toggle: true,
def: false,
},
'id': {
access: LegacyPropAccessType.ATTR,
def: '',
},
'lang': {
access: LegacyPropAccessType.ATTR,
},
'localName': {
access: LegacyPropAccessType.READ_ONCE,
},
'method': {
access: LegacyPropAccessType.ATTR,
def: 'get',
},
'name': {
access: LegacyPropAccessType.ATTR,
},
'noValidate': {
access: LegacyPropAccessType.ATTR,
attr: 'novalidate',
bool: true,
toggle: true,
def: false,
},
'prefix': {
access: LegacyPropAccessType.READ_ONCE,
},
'spellcheck': {
access: LegacyPropAccessType.ATTR,
},
'style': {
access: LegacyPropAccessType.READ_ONCE,
},
'target': {
access: LegacyPropAccessType.ATTR,
def: '',
},
'title': {
access: LegacyPropAccessType.ATTR,
},
'translate': {
access: LegacyPropAccessType.ATTR,
},
};
8 changes: 8 additions & 0 deletions extensions/amp-form/0.1/test/test-amp-form.js
Expand Up @@ -146,6 +146,14 @@ describe('amp-form', () => {
expect(form.className).to.contain('-amp-form');
});

it('should install proxy', () => {
const form = getForm();
form.setAttribute('action-xhr', 'https://example.com');
new AmpForm(form);
expect(form.$p).to.be.ok;
expect(form.$p.getAttribute('action-xhr')).to.equal('https://example.com');
});

it('should do nothing if already submitted', () => {
const form = getForm();
const ampForm = new AmpForm(form);
Expand Down