fix form.elements -- this is used by NodeBind getAssociatedRadioButtons #462

jmesserly · 2014-06-26T23:26:26Z

form.elements should return an HTMLCollection.

     return filter(element.form.elements, function(el) {
        return el != element &&
            el.tagName == 'INPUT' &&
            el.type == 'radio' &&
            el.name == element.name;
      });

...uses form.elements, which was returning unwrapped nodes under SD polyfill. This later causes an error because Node.bind stores bindings_ on the wrapper, and when it doesn't find the data it stored, blows up with a TypeError.

Caught this by running NodeBind tests with platform.js (normally they don't run with SD polyfill -- maybe they should?)

jmesserly · 2014-06-26T23:28:15Z

Hey, @arv do you mind taking a look?

I patterned it after similar wrappers like HTMLTableRowElement. All tests pass after this change.

arv · 2014-06-27T12:50:35Z

src/wrappers/HTMLFormElement.js

+  var registerWrapper = scope.registerWrapper;
+  var wrapHTMLCollection = scope.wrapHTMLCollection;
+  var unwrap = scope.unwrap;
+  var wrap = scope.wrap;


It doesn't look like you are using wrap.

good catch. fixed!

arv · 2014-06-27T12:52:02Z

LGTM

HTMLFormControlsCollection is one of the worst abominations of the DOM APIs. It has named interceptors too and sometimes these return an element and sometime another collection. Hopefully we do not have to ever implement that.

fix form.elements -- this is used by NodeBind getAssociatedRadioButtons

@export

… with Dart binding paths for things that are not JSON serializable. Two big things this enables: * elements themselves can be data bound * anything jsify can handle, including Dart functions. This means as we get @export and such, it will be possible to bind those Dart types directly into JS elements. With that we can make things like <core-style> much nicer to use. This also removes ported code from https://github.com/Polymer/NodeBind except for interfaces, since we don't need it anymore. Added a Polymer test to verify it works for the typical end-to-end case. This uncovered the "deliver" method that needs to be there on Bindable. Finally one Node.bind test depends on googlearchive/ShadowDOM#462, which fixes an issue where SD polyfill doesn't wrap an API used by Node.bind. Apparently no one has ever hit this on the JS side (the Dart port was immune, because our treatment of HTMLCollection is better). [email protected] Review URL: https://codereview.chromium.org//355133002 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge@38051 260f80e4-7a28-3924-810f-c04153c831b5

@export

… with Dart binding paths for things that are not JSON serializable. Two big things this enables: * elements themselves can be data bound * anything jsify can handle, including Dart functions. This means as we get @export and such, it will be possible to bind those Dart types directly into JS elements. With that we can make things like <core-style> much nicer to use. This also removes ported code from https://github.com/Polymer/NodeBind except for interfaces, since we don't need it anymore. Added a Polymer test to verify it works for the typical end-to-end case. This uncovered the "deliver" method that needs to be there on Bindable. Finally one Node.bind test depends on googlearchive/ShadowDOM#462, which fixes an issue where SD polyfill doesn't wrap an API used by Node.bind. Apparently no one has ever hit this on the JS side (the Dart port was immune, because our treatment of HTMLCollection is better). [email protected] Review URL: https://codereview.chromium.org//355133002 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart/pkg/polymer@38051 260f80e4-7a28-3924-810f-c04153c831b5

@export

… with Dart binding paths for things that are not JSON serializable. Two big things this enables: * elements themselves can be data bound * anything jsify can handle, including Dart functions. This means as we get @export and such, it will be possible to bind those Dart types directly into JS elements. With that we can make things like <core-style> much nicer to use. This also removes ported code from https://github.com/Polymer/NodeBind except for interfaces, since we don't need it anymore. Added a Polymer test to verify it works for the typical end-to-end case. This uncovered the "deliver" method that needs to be there on Bindable. Finally one Node.bind test depends on googlearchive/ShadowDOM#462, which fixes an issue where SD polyfill doesn't wrap an API used by Node.bind. Apparently no one has ever hit this on the JS side (the Dart port was immune, because our treatment of HTMLCollection is better). [email protected] Review URL: https://codereview.chromium.org//355133002 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart/pkg/observe@38051 260f80e4-7a28-3924-810f-c04153c831b5

@export

… with Dart binding paths for things that are not JSON serializable. Two big things this enables: * elements themselves can be data bound * anything jsify can handle, including Dart functions. This means as we get @export and such, it will be possible to bind those Dart types directly into JS elements. With that we can make things like <core-style> much nicer to use. This also removes ported code from https://github.com/Polymer/NodeBind except for interfaces, since we don't need it anymore. Added a Polymer test to verify it works for the typical end-to-end case. This uncovered the "deliver" method that needs to be there on Bindable. Finally one Node.bind test depends on googlearchive/ShadowDOM#462, which fixes an issue where SD polyfill doesn't wrap an API used by Node.bind. Apparently no one has ever hit this on the JS side (the Dart port was immune, because our treatment of HTMLCollection is better). [email protected] Review URL: https://codereview.chromium.org//355133002 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart/pkg/polymer_expressions@38051 260f80e4-7a28-3924-810f-c04153c831b5

@export

… with Dart binding paths for things that are not JSON serializable. Two big things this enables: * elements themselves can be data bound * anything jsify can handle, including Dart functions. This means as we get @export and such, it will be possible to bind those Dart types directly into JS elements. With that we can make things like <core-style> much nicer to use. This also removes ported code from https://github.com/Polymer/NodeBind except for interfaces, since we don't need it anymore. Added a Polymer test to verify it works for the typical end-to-end case. This uncovered the "deliver" method that needs to be there on Bindable. Finally one Node.bind test depends on googlearchive/ShadowDOM#462, which fixes an issue where SD polyfill doesn't wrap an API used by Node.bind. Apparently no one has ever hit this on the JS side (the Dart port was immune, because our treatment of HTMLCollection is better). [email protected] Review URL: https://codereview.chromium.org//355133002 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart/pkg/template_binding@38051 260f80e4-7a28-3924-810f-c04153c831b5

@export

… with Dart binding paths for things that are not JSON serializable. Two big things this enables: * elements themselves can be data bound * anything jsify can handle, including Dart functions. This means as we get @export and such, it will be possible to bind those Dart types directly into JS elements. With that we can make things like <core-style> much nicer to use. This also removes ported code from https://github.com/Polymer/NodeBind except for interfaces, since we don't need it anymore. Added a Polymer test to verify it works for the typical end-to-end case. This uncovered the "deliver" method that needs to be there on Bindable. Finally one Node.bind test depends on googlearchive/ShadowDOM#462, which fixes an issue where SD polyfill doesn't wrap an API used by Node.bind. Apparently no one has ever hit this on the JS side (the Dart port was immune, because our treatment of HTMLCollection is better). [email protected] Review URL: https://codereview.chromium.org//355133002 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart/pkg/web_components@38051 260f80e4-7a28-3924-810f-c04153c831b5

arv reviewed Jun 27, 2014
View reviewed changes

jmesserly added cla: yes and removed cla: no labels Jun 27, 2014

fix form.elements -- this is used by NodeBind getAssociatedRadioButtons

a6d36f8

jmesserly pushed a commit that referenced this pull request Jun 27, 2014

Merge pull request #462 from Polymer/fix_form_elements

b2c730a

fix form.elements -- this is used by NodeBind getAssociatedRadioButtons

jmesserly merged commit b2c730a into master Jun 27, 2014

jmesserly deleted the fix_form_elements branch June 27, 2014 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix form.elements -- this is used by NodeBind getAssociatedRadioButtons #462

fix form.elements -- this is used by NodeBind getAssociatedRadioButtons #462

jmesserly commented Jun 26, 2014

jmesserly commented Jun 26, 2014

arv Jun 27, 2014

jmesserly Jun 27, 2014

arv commented Jun 27, 2014

fix form.elements -- this is used by NodeBind getAssociatedRadioButtons #462

fix form.elements -- this is used by NodeBind getAssociatedRadioButtons #462

Conversation

jmesserly commented Jun 26, 2014

jmesserly commented Jun 26, 2014

arv Jun 27, 2014

Choose a reason for hiding this comment

jmesserly Jun 27, 2014

Choose a reason for hiding this comment

arv commented Jun 27, 2014