Port attributeBindings to AttrNode views #10186

mixonic · 2015-01-10T17:55:42Z

Ember's attribute bindings were a custom system based on the buffer and a view. The buffer would be updated with initial values, and the element have those attributes applied during it's own render pass. Observers were attached to watch changing values.

This refactors attribute bindings to use AttrNode objects, which in turn are rendered like a normal view on the renderer. They have access to morphs and the domHelper in a standard view-ish way.

TODO

There is a failing test that tries to assert a cursor position in a text field not changing. Needs a deep dive.
IE8 check. Should end up meaning type needs to be added to the buffer in a legacy string style. I've kept this in mind an not gutted the legacy system, so it should go fairly smoothly.
Upstream HTMLBars PR: Change setProperty and morph to remove an undefined or NaN attr value tildeio/htmlbars#248
Upstream PR to address attribute removal in IE IE <11 does not honor removeAttribute tildeio/htmlbars#251
Upstream PR to fix passing numbers to parseHTML Allow numbers to be parsed as HTML in IE tildeio/htmlbars#253
Check for any obvious performance regression

/cc @mmun @krisselden and view-layer crew.

Closes #9921

mixonic · 2015-01-11T05:47:37Z

packages/ember-views/tests/views/text_field_test.js

@@ -553,7 +553,6 @@ test('should not reset cursor position when text field receives keyUp event', fu

  appendView(view);

-  view.$().val('Brosiedoon, King of the Brocean');


The old logic for writing a value would compare the value to be set against the one currently in the DOM.

In the new logic, a cached last value is compared instead. The bound attribute does not want a 3rd party (here, a jQuery call) to change the properties.

We may need to fall back to the check-dom-every-time strategy if this is problematic.

I much prefer the new logic of caching the last value. IMHO, it has been fairly clear thst third parties changing the DOM underneath is not acceptable. It may have worked in this particular instance, but many many others did not.

rwjblue · 2015-01-11T13:40:25Z

This looks wonderful!

rwjblue · 2015-01-12T00:41:38Z

HTMLBars v0.8.1 includes the upstream HTMLBars changes needed for this.

rwjblue · 2015-01-12T01:59:40Z

I believe that this should close #9921, @mixonic if you agree can you update the description?

oneeman · 2015-01-12T03:08:26Z

packages/ember-views/lib/views/view.js

+      binding = attributeBindings[i];
+      split = binding.split(':');
+      property = split[0];
+      attrName = split[1] || property;


attrName should be the full name in case of namespaced attributes, rather than split[1]. I believe that would take care of #9298.

Oof this is for micro-syntax not for namespaced attrs. The solution is not as super-simple as it seems, imo.

isDisabled:disabled is a mapping of view-prop:attr

xlink:href is a namespaced attr

hrefLink:xlink:href is a mapping of view-prop:namespaced-attr

There isn't really a way to support all that. I would propose:

isDisabled:disabled is a mapping of view-prop:attr

xlink:href is a mapping of view-prop:attr (gotcha)

hrefLink:xlink:href is a mapping of view-prop:namespaced-attr

This would be consistent. Basically, the micro-syntax sucks. Maybe we can drop it in 2.0. I don't want to tackle it here though.

Good thinking! Since : isn't valid a valid character for property names, they'd have to use a view-prop anyway, right? In that case the gotcha's not much of a limitation, since there's no way they could define the property for xlink:href under its own name even if they wanted to. (If I remember the whole mapping thing right, that is)

mixonic · 2015-01-12T03:35:31Z

Opened an upstream PR at tildeio/htmlbars#253, and an issue at tildeio/htmlbars#251 that needs a PR.

IE10 is a go, still have two or three IE9 issues.

mixonic · 2015-01-14T01:50:20Z

IE9 is a go. PR opened for tildeio/htmlbars#256.

I need to wait on IE8 until HTMLBars is working again.

mixonic · 2015-01-18T22:43:38Z

Getting close. IE9 and up are 100% with htmlbars master. Just IE8.

rwjblue · 2015-01-25T01:19:44Z

I'm comfortable landing this for current master (which will branch to 1.11 shortly), but I think @mixonic is waiting to review and confirm that this general strategy will work for IE8.

rwjblue · 2015-01-26T22:55:48Z

ping @mixonic - have any updates here?

mixonic · 2015-01-30T05:33:44Z

Rebased this tonight.

Something in IE8 on master causes the page to crash then reload. I'm blocked on that now.

Unless someone else wants to hop on it first, some IE things we should do before trying to fix 8:

Most of the test changes here are IE fixes for master. They can be pulled into a new PR for master.
Many of them can likely land in beta as well.
First the reload issues in IE8 needs to be addressed. I want to suggest it has something to do with sanitization, but I'm very unsure at this point.
Then IE8 in general needs to be confirmed as working.
Then I can get the IE8 fixes for this patch done.

I'll be working on some of this over the weekend.

mixonic · 2015-02-01T03:00:40Z

Needs tildeio/htmlbars#276 to land for node tests to pass

mixonic · 2015-02-05T04:58:15Z

This is good implementation-wise. Performance benchmarks stand unchanged against master itself (tested with render list, render list with link-to, and render link-to).

I believe this is good to go.

mixonic · 2015-02-05T05:10:30Z

I swear this is a net lines-of-code win, just need to wait until we strip the ember-htmlbars-attribute-syntax feature flag. Re-using this pattern for classNameBindings will also help.

Port attributeBindings to AttrNode views

oneeman · 2015-02-05T15:35:02Z

Awesome! Since this is merged now, I could do a PR to implement #10186 (comment).

mixonic · 2015-02-06T04:55:22Z

Turns out this definitely breaks container view. But also there are no tests in all of Ember that use attribute bindings and container view so meeeeh.

Working on the classNameBindings patch this weekend, and will make part of that porting the attr nodes onto _attrViews or something not the _childViews array. I believe that is the best way out.

mixonic mentioned this pull request Jan 11, 2015

Change setProperty and morph to remove an undefined or NaN attr value tildeio/htmlbars#248

Merged

mixonic force-pushed the attribute-bindings branch 2 times, most recently from 200350a to e1375f3 Compare January 11, 2015 05:44

mixonic reviewed Jan 11, 2015
View reviewed changes

mixonic force-pushed the attribute-bindings branch 2 times, most recently from cebe456 to c32d9c3 Compare January 12, 2015 01:19

mixonic force-pushed the attribute-bindings branch 2 times, most recently from 09b7fc5 to d06d108 Compare January 12, 2015 02:42

oneeman reviewed Jan 12, 2015
View reviewed changes

mixonic force-pushed the attribute-bindings branch from d06d108 to 141571d Compare January 12, 2015 03:30

This was referenced Jan 13, 2015

attributeBinding with properties not available in element prototype do not get set. #10202

Closed

value as a attributeBinding is not rendered as an attribute #10183

Closed

mixonic force-pushed the attribute-bindings branch from 141571d to 4a30267 Compare January 14, 2015 01:42

miguelcobain mentioned this pull request Jan 14, 2015

[FEATURE] override attribute bindings #10213

Merged

mixonic force-pushed the attribute-bindings branch from 4a30267 to 0f6d58b Compare January 18, 2015 21:39

wagenet added the Needs Label Review label Jan 23, 2015

mixonic force-pushed the attribute-bindings branch 2 times, most recently from e27cafb to a4d4a90 Compare January 30, 2015 05:01

mixonic force-pushed the attribute-bindings branch from a4d4a90 to 79588e0 Compare January 31, 2015 14:55

mixonic mentioned this pull request Jan 31, 2015

[BUGFIX beta] IE10-8 fixes. normalizeProperty expects lowercase prop name #10318

Merged

mixonic force-pushed the attribute-bindings branch from 79588e0 to 049ac2a Compare February 1, 2015 02:06

mixonic force-pushed the attribute-bindings branch 3 times, most recently from cbdf7fc to 04ce7a1 Compare February 5, 2015 04:39

Port attributeBindings to AttrNode views

c352922

mixonic force-pushed the attribute-bindings branch from 04ce7a1 to c352922 Compare February 5, 2015 04:59

rwjblue added a commit that referenced this pull request Feb 5, 2015

Merge pull request #10186 from mixonic/attribute-bindings

0e0046a

Port attributeBindings to AttrNode views

rwjblue merged commit 0e0046a into emberjs:master Feb 5, 2015

rwjblue deleted the attribute-bindings branch February 5, 2015 15:31

mixonic mentioned this pull request Feb 5, 2015

Ember.Component attributeBindings doesn't work with namespaced attributes? (e.g. xlink:href) #9298

Closed

oneeman mentioned this pull request Feb 6, 2015

[BUGFIX] Handle binding namespaced attributes (#9298) #10365

Merged

rwjblue mentioned this pull request Feb 7, 2015

Port applyClassNameBindings to use attr nodes #9922

Closed

rwjblue mentioned this pull request Mar 9, 2015

Ember 1.11 beta: input placeholder "undefined" #10595

Closed

SlexAxton mentioned this pull request Apr 17, 2015

The form (or any other readOnly) attribute is not able to be bound to a valid element (button). #10894

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port attributeBindings to AttrNode views #10186

Port attributeBindings to AttrNode views #10186

mixonic commented Jan 10, 2015

mixonic Jan 11, 2015

rwjblue Jan 11, 2015

rwjblue commented Jan 11, 2015

rwjblue commented Jan 12, 2015

rwjblue commented Jan 12, 2015

oneeman Jan 12, 2015

mixonic Jan 14, 2015

oneeman Jan 14, 2015

mixonic commented Jan 12, 2015

mixonic commented Jan 14, 2015

mixonic commented Jan 18, 2015

rwjblue commented Jan 25, 2015

rwjblue commented Jan 26, 2015

mixonic commented Jan 30, 2015

mixonic commented Feb 1, 2015

mixonic commented Feb 5, 2015

mixonic commented Feb 5, 2015

oneeman commented Feb 5, 2015

mixonic commented Feb 6, 2015

		@@ -553,7 +553,6 @@ test('should not reset cursor position when text field receives keyUp event', fu

		appendView(view);

		view.$().val('Brosiedoon, King of the Brocean');

Port attributeBindings to AttrNode views #10186

Port attributeBindings to AttrNode views #10186

Conversation

mixonic commented Jan 10, 2015

mixonic Jan 11, 2015

Choose a reason for hiding this comment

rwjblue Jan 11, 2015

Choose a reason for hiding this comment

rwjblue commented Jan 11, 2015

rwjblue commented Jan 12, 2015

rwjblue commented Jan 12, 2015

oneeman Jan 12, 2015

Choose a reason for hiding this comment

mixonic Jan 14, 2015

Choose a reason for hiding this comment

oneeman Jan 14, 2015

Choose a reason for hiding this comment

mixonic commented Jan 12, 2015

mixonic commented Jan 14, 2015

mixonic commented Jan 18, 2015

rwjblue commented Jan 25, 2015

rwjblue commented Jan 26, 2015

mixonic commented Jan 30, 2015

mixonic commented Feb 1, 2015

mixonic commented Feb 5, 2015

mixonic commented Feb 5, 2015

oneeman commented Feb 5, 2015

mixonic commented Feb 6, 2015