googlearchive / core-input

A single-line text field with some extra powers
7 stars 20 forks source link

Data-bound 'tabindex' attribute generates errors #39

Closed kbenjamin closed 9 years ago

kbenjamin commented 10 years ago

I'm not sure that this error is limited to <core-input> or if it is a more general Polymer lifecycle issue so please read on to see what you think.

When setting a data-bound tabindex attribute on <paper-input> or <core-input> an error is thrown from core-input::handleTabIndex() that originates in core-input::attributeChanged().

I've submitted a PR that fixes this issue.

Here's the output from the test code below:

created. test.html:17
Uncaught TypeError: Cannot read property 'input' of undefined core-input.html:319
ready. this.tabindex = 100 , attributes.tabindex = 100 test.html:20
attached. this.tabindex = 100 , attributes.tabindex = 100 test.html:23
domReady test.html:26

The issue is that <core-input> uses the attributeChanged() lifecycle method which seems to fire before the ready() lifecycle method when there is bound data. The question that affects Polymer generally is whether or not this is the correct timing. Should attributeChanged() run before ready?

My guess is that it should but if there are other places in the Polymer web components collection that rely on attributeChanged() they may similarly have issues with data binding.

Fixing the specific problem with <core-input> is a simple matter of using a property change watcher rather than the attributeChanged() lifecycle method as the ready() lifecycle method already forces a run of handleTabindex(). I'm submitting a PR with that change in it.

Test Code

<!DOCTYPE html>
<html>
<head>
    <style>body { padding: 5%; }</style>
    <script src="../platform/platform.js"></script>
    <polymer-element name="my-test" attributes="tabindex">
        <link rel="import" href="../polymer/polymer.html">
        <link rel="import" href="../core-input/core-input.html">

        <template>
            <core-input id="input_1" tabindex="{{tabindex}}" autofocus placeholder="Enter something"></core-input>
        </template>
        <script>
            Polymer('my-test', {
                tabindex : 100,         // set a baseline tabindex
                created: function () {
                    console.log('created.');
                },
                ready: function () {
                    console.log('ready. this.tabindex =', this.tabindex, ', attributes.tabindex =', '' + (this.$) ? this.$.input_1.getAttribute('tabindex') : 'undefined' );
                },
                attached: function () {
                    console.log('attached. this.tabindex =', this.tabindex, ', attributes.tabindex =', '' + (this.$) ? this.$.input_1.getAttribute('tabindex') : 'undefined' );
                },
                domReady: function(){
                    console.log('domReady');
                },
                attributeChanged: function(attr, oldVal, curVal) {
                    console.log('attributeChanged', attr, oldVal, curVal);
                }
            });
      </script>
    </polymer-element>
</head>
<body>
<my-test ></my-test>
</body>
</html>
morethanreal commented 9 years ago

This should be a non-issue now that core-input extends from input.