Using attributeBindings for component in 1.11+ doesn't bind the form attr

[Robdel12]

When upgrading emberx-select I encountered an odd bug where using attributeBindings in a component to bind the form attribute doesn't actually bind form as of 1.11+.

I've provided two JSBins below. One is 1.10 where the binding works correctly and the other is 1.11+ where the binding doesn't work.

Also here is a failing test for emberx-select on ember 1.12.

1.10: http://emberjs.jsbin.com/hejeqevosu/1/edit?html,js,output

1.11+: http://emberjs.jsbin.com/fahiwurece/1/edit?html,js,output

I'm going to CC @stefanpenner since I reached out to him on twitter :P

[rwjblue]

s = document.createElement('select')
// => <select>​</select>​
s.form = 'foo'
// => "foo"
s.form
// => null

Oh, DOM...

[simi]

@rwjblue it looks like form is readonly property returning target form (HTMLSelectElement).

[stefanpenner]

I am inclined to thing that this is actually a bug in emberx-select @robdel12 can you explain to my why this is to be considered an ember bug?

[stefanpenner]

so setAttribute works correctly

s = document.createElement('select')
// => <select>​</select>​
s.setAttribute('form', 'foo')
// => undefined
s.form
// confusingly => null

s.getAttribute('form')
// => "bar"

[stefanpenner]

So it appears ember is setting this as a property, not an attribute... I suspect the AttrNode, needs to be aware its actually a property? Something seems extremely fishy...

cc @mixonic I believe you are more familiar with this code-path (as you wrote it), thoughts?

[mixonic]

Ember will always set a property if a property exists. This is decided around here.

We can probably deem select[form] to always be an attribute by ensuring it does not return a value from normalizeProperty

[stefanpenner]

I believe this also affects select.labels, select.options, select.selectIndex, select.selectedOptions, select.type, select.validationMessage, select.validity, select.willValidate and likely more.

• legend.form
• option.form
• ...

I suspect we need to nail the heuristics and see what other strange attributes exists we need to take into account.

[stefanpenner]

i wonder if we can handle it via.

var originalValue = element[name];
element[name] = value
if (element[name]  === originalValue && // if the value hasn't changed
    value !== originalValue && // if the value is different then the originalValue
    value === value // if the value is not NaN
) {
  element.setAttribute(name, value)
}

[vectart]

@stefanpenner could that cause some calculations due to reading such properties as clientHeight?

As an alternative, we can create a list of readonly attributes from the DOM specification: http://www.w3.org/TR/DOM-Level-2-HTML/html.html Searching for readonly attribute reveals that there are only 40 of them.

[stefanpenner]

@vectart but 40 across how many elements, with what combination / permutation? Also the DOM changes over time..

[stefanpenner]

@vectart we could likely cache the outcome to minimize the chance of an issue.

[vectart]

Just for considering, I was thinking of building a list of read-only attributes and checking the element prototype. Is this solution overcomplicated?

element = document.createElement('select');
attr = 'form';

// some dictionaries, might be generated by creating elements and brute-forcing their attributes 
HTMLSelectElement.readOnlyAttributes = ['type', 'form', 'option'];
HTMLObjectElement.readOnlyAttributes = ['form', 'contentDocument'];

// check if it's impossible to set attribute
if (element instanceof HTMLSelectElement && HTMLSelectElement.readOnlyAttributes.indexOf(attr) > -1) {
  set(element, attr, value);
} else {
  element[attr] = value;
}

[stefanpenner]

I was thinking of building a list of read-only

shipping the list or computing it?

[vectart]

@stefanpenner it's possible to ship it

[{
  interface: Document,
  attrs: ['doctype', 'implementation', 'documentElement']
},
{
  interface: Node,
  attrs: ['nodeName', 'nodeType', 'parentNode', 'childNodes', 'firstChild', 'lastChild', 'previousSibling', 'nextSibling', 'attributes', 'ownerDocument']
},
{
  interface: NodeList,
  attrs: ['length']
},
{
  interface: NamedNodeMap,
  attrs: ['length']
},
{
  interface: CharacterData,
  attrs: ['length']
},
{
  interface: Attr,
  attrs: ['name', 'specified']
},
{
  interface: Element,
  attrs: ['tagName']
},
{
  interface: DocumentType,
  attrs: ['name', 'entities', 'notations']
},
{
  interface: ProcessingInstruction,
  attrs: ['target']
},
{
  interface: HTMLCollection,
  attrs: ['length']
},
{
  interface: HTMLDocument,
  attrs: ['referrer', 'domain', 'URL', 'images', 'applets', 'links', 'forms', 'anchors']
},
{
  interface: HTMLFormElement,
  attrs: ['elements', 'length']
},
{
  interface: HTMLSelectElement,
  attrs: ['type', 'length', 'form', 'options']
},
{
  interface: HTMLOptionElement,
  attrs: ['form', 'text', 'selected']
},
{
  interface: HTMLInputElement,
  attrs: ['form', 'type']
},
{
  interface: HTMLTextAreaElement,
  attrs: ['form', 'type']
},
{
  interface: HTMLButtonElement,
  attrs: ['form', 'type']
},
{
  interface: HTMLLabelElement,
  attrs: ['form']
},
{
  interface: HTMLFieldSetElement,
  attrs: ['form']
},
{
  interface: HTMLLegendElement,
  attrs: ['form']
},
{
  interface: HTMLObjectElement,
  attrs: ['form']
},
{
  interface: HTMLMapElement,
  attrs: ['areas']
},
{
  interface: HTMLTableElement,
  attrs: ['rows', 'tBodies']
},
{
  interface: HTMLTableSectionElement,
  attrs: ['rows']
}]

I've performed a speed test with frequent used interfaces only and it displays quite low overhead http://codepen.io/anon/pen/EjgvXP?editors=001

[rwjblue]

For our purposes I think we can limit the map to just the list of *Element entries in the above list.

[vectart]

@rwjblue I've updated the test with your recommendation: http://codepen.io/anon/pen/EjgvXP?editors=001

[stefanpenner]

Seems good to me. Should I integrate & test this or @vectart do you want to take it from here?

[vectart]

@stefanpenner I'd be glad to contribute, can work on that today.

[mixonic]

I'd prefer to see a dynamic system instead of a whitelist.

[stefanpenner]

I'd prefer to see a dynamic system instead of a whitelist.

I'm somewhat unsure as well.

pro:

• work is done upfront, and does not rely on heuristics to guess if something is an attr or prop

con:

• my lose sync as the DOM changes over time.
• the system current attempts to be detect driven

[vectart]

@mixonic this list is converted from IDL definitions that are 17 years old, so it's quite stable :-) http://www.w3.org/TR/REC-DOM-Level-1/idl-definitions.html

[stefanpenner]

@mixonic hmm, wont the detection be screwed up once glimmer re-uses DOM produced by SSR ? As the current check is merely existence of a property, which deems it a property over an attribute. This in theory would confuse the check, and I suspect it would result attributes being incorrectly characterized as properties.

I suspect I am also confused when overlap is acceptable, and when it is not. @mixonic can you share?

[mixonic]

@stefanpenner not all attributes become properties- for example if you set data-foo in the HTML is will not become a property, which I think is what you are implying?

[stefanpenner]

So we can't really safely detect, as some attributes throw when being set to. This means copious amounts of try/catch based detection. Which will be a debugging hazard i want to avoid.

I think the whitelist is the right way.

[stefanpenner]

so, SELECT.labels SELECT.validty are also readOnly... I suspect this list is larger then we suspect.

[stefanpenner]

[edit: content to large to display reasonably]

the true list of readOnly appears to be: https://gist.github.com/stefanpenner/a3cf3b3f49e429965cec

this is far far to big to actually ship.

So we have some options:

• only detect attributes that are actually are about to be attempted
• only support webIDL level 1 readOnly attributes
• detect readOnly attributes
• combination of hard-coding common readOnly attributes + detection.

[stefanpenner]

After some thought, although someone should sanity check me, I actually believe we just need to care about:

var READ_ONLY_ATTR_BY_TAG = {
  SELECT:       ['form'],
  OPTION:       ['form'],
  INPUT:        ['form'],
  TEXTAREA:     ['form'],
  BUTTON:       ['form'],
  LABEL:        ['form'],
  FIELDSET:     ['form'],
  LEGEND:       ['form'],
  OBJECT:       ['form']
};

This is because, most of the documented readOnly properties, such as table.rows and form.length really have no meaning. For custom attributes, users should use data- attributes.

[stefanpenner]

interesting discovery thanks to @jayphelps

var form = document.createElement('form');
form.id = 'i-am-form';
var input = document.createElement('input');
input.form = 'i-am-form';
document.body.appendChild(form);
input.form === 'i-am-form' //=> false
document.body.appendChild(input);
input.form === 'i-am-form' //=> false

but the following (with setAttribute) works. Please note, form can only be read after both, the form and the element have been inserted. Also note, the order of insertion doesn't matter.

var form = document.createElement('form');
form.id = 'i-am-form';
var input = document.createElement('input');
input.setAttribute('form', 'i-am-form');
document.body.appendChild(form);
input.form === 'i-am-form' //=> false
document.body.appendChild(input);
input.form === 'i-am-form' //=> true

[jayphelps]

Note that we might actually be be able to detect these instances in HTMLBars.

Observe:

Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'form')
/*
Object {
  configurable: true
  enumerable: true
  get: () { [native code] }
  set: undefined
}
*/

It isn't "readonly" (technically) is just doesn't have a setter, which makes it just effectively readonly. Hence why it does not error when you attempt to write to it.

whether this is the same descriptor cross-browser, for all we support is a different story.....

[stefanpenner]

@jayphelps you are correct, but ES5 only. Also, it appears this attribute is the only one I have found so far. So we can likely just hard-code the solution.

I am curious if we can find other ones.

[stefanpenner]

one work-around: https://github.com/tildeio/htmlbars/pull/352

[stefanpenner]

@jayphelps should we use the gOPD and detect the missing setter or..

[jayphelps]

@stefanpenner I'm working on testing a solution that uses it.

@everyoneelse--getOwnPropertyDescriptor works IE8+ on DOM objects, so I'm running with that knowledge to test whether we can prevent a hardcoded list and instead detect "readonly" behavior of a given property.

[stefanpenner]

Object.getOwnPropertyDescriptor(element.constructor.prototype, 'form').set === undefined

[jayphelps]

@stefanpenner :stuck_out_tongue_closed_eyes: not that simple, but close. Could have .value or readonly: true cause even if the browser doesn't do it, custom components can. hold your horses.

[stefanpenner]

@jayphelps yes, but the question is, should setAttribute be used if other modes are active?

[stefanpenner]

quick list of attributes with Object.getOwnPropertyDescriptor(tag, attr).set === undefined

https://gist.github.com/stefanpenner/575cfc90889cbab9d52b

but still unable to find another attribute that requires this (beyond form). Basically, i think we are looking for readOnly, but meant for updating...

[aFarkas]

@stefanpenner I think the list attribute/property should be handled also like form.

[jayphelps]

@aFarkas Fixed in https://github.com/tildeio/htmlbars/pull/363

[stefanpenner]

should be fixed in 1.13.3 due to https://github.com/tildeio/htmlbars/pull/374

[rwjblue]

Please confirm, and we can close...