detect props that aren't writable, fixes emberjs/ember.js#11221 #353

jayphelps · 2015-05-28T00:00:16Z

Working on tests, but want your input on the implementation while I do...

jayphelps · 2015-05-28T00:03:37Z

packages/dom-helper/lib/prop.js

+function isSettable(element, attrName) {
+  var desc = getOwnPropertyDescriptor(element.constructor.prototype, attrName);
+  if (!desc) { return true; }
+  if (!desc.writable || !desc.hasOwnProperty('value') && typeof desc.set !== 'function') {


This is the confusing and important part. We can change the "effective value" of a property (e.g. what we would get back if we accessed it) if it is writable OR it doesn't have a value AND has a set function. Browser spec guarantees that we will never get back a descriptor that has both a value and a set. Always mutually exclusive.

Property descriptors present in objects come in two main flavors: data descriptors and accessor descriptors. A data descriptor is a property that has a value, which may or may not be writable. An accessor descriptor is a property described by a getter-setter pair of functions. A descriptor must be one of these two flavors; it cannot be both. ~ Object.defineProperty

Note, while writing the tests, I just realized the first part !desc.writable isn't specific enough because of implicit boolean conversion of undefined. it needs to be desc.writable === false

stefanpenner · 2015-05-28T00:04:50Z

this may be better then: #352 but I wonder what examples beyond form require this functionality. I suspect in the world of custom WebComponents this may be the case, but i don't fully understand that scenario.

Can you share some further examples to help me understand this?

jayphelps · 2015-05-28T00:14:25Z

@stefanpenner

Absolutely. While I wouldn't be surprised if there were more prop/attr pairs in the spec with this behavior, my major concern is indeed custom elements.

class XPerson extends HTMLElement {
  @readonly
  age = 33;
}

document.registerElement('x-person', XPerson);

<x-person age={{someAge}}></x-person>

When I write his out, it actually indeed makes me wonder if properties truly marked as readonly via writeable: false should throw an error (or let the browser naturally throw it by writing to it) while properties which simply don't have a setter would instead just write to the attribute.

I can see cases both ways...So I'm unsure.

jayphelps · 2015-05-28T00:16:08Z

Summary: Do we need a distinction between "effectively readonly" and writeable: false?

AFAIK we can always setAttribute, but is there cases we this would be wrong and instead an error should be thrown? Because if someone has marked their prop as writeable: false, it may be very unintuitive that we set the attribute still.

rwjblue · 2015-05-28T09:54:52Z

packages/dom-helper/lib/prop.js

@@ -2,6 +2,9 @@ export function isAttrRemovalValue(value) {
  return value === null || value === undefined;
 }

+var getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;


Can you confirm this works on IE8 (which we still have to support)?

https://msdn.microsoft.com/library/dd548686%28v=vs.94%29.aspx

As per that article, getOwnPropertyDescroptor does work in IE8 for "DOM Objects", so it may be perfectly fine, I just want to confirm.

Thinking more about this: we have IE8 in CI so if tests are green (and we are testing this path), we are good.

stefanpenner · 2015-05-28T14:23:39Z

the input[type] scenario doesn't appear to be handled by this.

questions is, should it? Or should it be an additional whitelist

jayphelps · 2015-05-29T19:16:26Z

@stefanpenner If I'm following that thread correctly, IMO that's an unrelated case of "prop doesn't always reflect to attribute". The code for that might be in the same place, but unrelated concepts.

jayphelps · 2015-05-29T19:38:02Z

Okay, I think I understand better now, the issue is actually the fact that is errors, not that it doesn't reflect. (still unrelated, but yeah, fix is in the same code path. I'm attempting a PR right now)

stefanpenner · 2015-05-29T19:50:44Z

Okay, I think I understand better now, the issue is actually the fact that is errors, not that it doesn't reflect. (still unrelated, but yeah, fix is in the same code path. I'm attempting a PR right now)

some funkyness for sure.
👍

rwjblue · 2015-06-04T20:24:07Z

Can haz rebase?

jayphelps · 2015-06-05T01:31:28Z

packages/dom-helper/lib/prop.js

      }
    }
    propertyCaches[tagName] = cache;
  }

-  // presumes that the attrName has been lowercased.
-  var value = cache[attrName];
+  var value = cache[attrName.toLowerCase()];


I need @mixonic to chime in why before, this "presumed" attrName was lowercased, when in fact it never was (at least in my testing). From what I could tell, the cache always missed when the attrName !== attrName.toLowerCase(). Was that for some reason intended?

jayphelps · 2015-06-10T21:46:22Z

packages/dom-helper/lib/prop.js

+  // assign anything. Both cases we will defer to setAttribute instead
+  var desc = getPropertyDescriptor(element, attrName);
+  if (!desc) { return true; }
+  if (desc.writable === false || !desc.hasOwnProperty('value') && typeof desc.set !== 'function') {


@rwjblue and I are discussing this in Slack...but typeof desc.set !== 'function' fails in Safari due to this outstanding bug: https://bugs.webkit.org/show_bug.cgi?id=49739. tl;dr Safari is the only browser that doesn't return full descriptors for host element properties.

Chrome:

Safari:

emberjs/ember.js#11221

jayphelps · 2015-06-10T23:36:28Z

@rwjblue @stefanpenner @mixonic Given the discovery here #353 (comment), I've updated the solution to exclusively use a blacklist approach for all native elements. For Custom Elements, the robust solution is used for "ideal" interop. Whether or not setting the attribute when the property is not settable for Custom Elements is the right thing to do (vs. erroring or just not doing anything) is a bigger question of how third party web component libs like polymer, graffiti, etc expect things.

Please review and provide thoughts.

jayphelps · 2015-06-10T23:39:20Z

To further complicate things...in some ways we're going down the same path that React does. Check this out: https://github.com/facebook/react/blob/4c3e9650ba6c9ea90956a08542d9fa9b5d72ee88/src/renderers/dom/shared/HTMLDOMPropertyConfig.js

stefanpenner · 2015-06-10T23:40:42Z

Find me at the meetup tonight. I believe we have a much better long term fix. Although this may still be a nice interim solution.

Robdel12 · 2015-06-18T04:27:44Z

Any updates on this? :)

jayphelps · 2015-06-18T04:42:52Z

@Robdel12 My latest understanding is that the fundamental way we choose whether we use the el.property = value vs setAttribute is likely to change. That said, @stefanpenner and I believed the issue would still exist for people choosing the wrong syntax (which doesn't exist yet) and we'd try and still fix it with this PR, but throw a warning of some kind.

I'm not the authority on this, just my understanding of it.

The idea is that <x-foo attr="{{bar}"> would use setAttribute because you put the mustache in quotes (HTML attributes are, after-all always strings) but if you omit the quotes <x-foo prop={{bar}> it would then use the same "props first if exists, setAttribute if not" pattern currently in place.

I'm personally in the camp of "this is going to confuse the hell out of people" because: previously we didn't really make it clear to devs that we were doing a props first, setAttribute second to begin with, most devs I've queried understandably don't realize there's a different between properties and attributes (I'm not joking), and maybe it won't be clear when to quote your 'staches and when not to. I'm super attached to this subject though, so I wouldn't be surprised if I was wrong about one or all of my concerns. I think it needs community input, likely an RFC. It's a tough problem because in almost every case we can choose props first and win, the dev doesn't need to know or care about the internals (which is why it was done) but these edge cases are brutal.

tl;dr

I hope this PR gets merged so people can move on, at least for now.

Robdel12 · 2015-06-19T20:47:19Z

Thanks for the update! That helped me understand what's going on here.

jamesarosen · 2015-06-28T03:14:24Z

I don't know if it helps, but here's a JSBin that shows that

setting form via prop doesn't take
setting form via attr will cause <select>.form to be the new <form>
setting form via attr will cause the form to include the <select> when it submits

jayphelps · 2015-07-06T02:32:11Z

Superseded by #374

jayphelps reviewed May 28, 2015
View reviewed changes

jayphelps force-pushed the readonly-props branch from c23d957 to 065a932 Compare May 28, 2015 00:31

rwjblue reviewed May 28, 2015
View reviewed changes

stefanpenner mentioned this pull request May 28, 2015

Binding type attribute on a button element fails in PhantomJS emberjs/ember.js#11112

Closed

jayphelps mentioned this pull request May 29, 2015

[BUGFIX] Adds the ability to blacklist props that should use setAttribute because of browser compliance issues #355

Merged

jayphelps force-pushed the readonly-props branch from 065a932 to 15944de Compare June 5, 2015 01:27

jayphelps reviewed Jun 5, 2015
View reviewed changes

jayphelps force-pushed the readonly-props branch from 15944de to 665b64a Compare June 10, 2015 21:42

jayphelps reviewed Jun 10, 2015
View reviewed changes

jayphelps force-pushed the readonly-props branch from 665b64a to f192769 Compare June 10, 2015 23:19

detect props that aren't settable so we fallback to setAttribute, fixes

93e66f2

emberjs/ember.js#11221

jayphelps force-pushed the readonly-props branch from f192769 to 93e66f2 Compare June 10, 2015 23:20

toranb mentioned this pull request Jun 15, 2015

Button type is not submit in phantomJS for some odd reason DockYard/ember-async-button#50

Closed

jayphelps changed the title ~~[WIP] detect props that aren't writable, fixes emberjs/ember.js#11221~~ detect props that aren't writable, fixes emberjs/ember.js#11221 Jun 18, 2015

jamesarosen referenced this pull request in jamesarosen/emberx-select Jun 28, 2015

drop support for "form" attribute

9ee917f

jayphelps closed this Jul 6, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

detect props that aren't writable, fixes emberjs/ember.js#11221 #353

detect props that aren't writable, fixes emberjs/ember.js#11221 #353

jayphelps commented May 28, 2015

jayphelps May 28, 2015

jayphelps May 28, 2015

jayphelps May 28, 2015

stefanpenner commented May 28, 2015

jayphelps commented May 28, 2015

jayphelps commented May 28, 2015

rwjblue May 28, 2015

rwjblue May 28, 2015

stefanpenner commented May 28, 2015

jayphelps commented May 29, 2015

jayphelps commented May 29, 2015

stefanpenner commented May 29, 2015

rwjblue commented Jun 4, 2015

jayphelps Jun 5, 2015

jayphelps Jun 10, 2015

jayphelps commented Jun 10, 2015

jayphelps commented Jun 10, 2015

stefanpenner commented Jun 10, 2015

Robdel12 commented Jun 18, 2015

jayphelps commented Jun 18, 2015

Robdel12 commented Jun 19, 2015

jamesarosen commented Jun 28, 2015

jayphelps commented Jul 6, 2015

detect props that aren't writable, fixes emberjs/ember.js#11221 #353

detect props that aren't writable, fixes emberjs/ember.js#11221 #353

Conversation

jayphelps commented May 28, 2015

jayphelps May 28, 2015

Choose a reason for hiding this comment

jayphelps May 28, 2015

Choose a reason for hiding this comment

jayphelps May 28, 2015

Choose a reason for hiding this comment

stefanpenner commented May 28, 2015

jayphelps commented May 28, 2015

jayphelps commented May 28, 2015

rwjblue May 28, 2015

Choose a reason for hiding this comment

rwjblue May 28, 2015

Choose a reason for hiding this comment

stefanpenner commented May 28, 2015

jayphelps commented May 29, 2015

jayphelps commented May 29, 2015

stefanpenner commented May 29, 2015

rwjblue commented Jun 4, 2015

jayphelps Jun 5, 2015

Choose a reason for hiding this comment

jayphelps Jun 10, 2015

Choose a reason for hiding this comment

jayphelps commented Jun 10, 2015

jayphelps commented Jun 10, 2015

stefanpenner commented Jun 10, 2015

Robdel12 commented Jun 18, 2015

jayphelps commented Jun 18, 2015

tl;dr

Robdel12 commented Jun 19, 2015

jamesarosen commented Jun 28, 2015

jayphelps commented Jul 6, 2015