After upgrading to 2.2.0, <input> tag 'readonly' attribute is not rendered #151

Closed
jancurn opened this Issue Nov 10, 2016 · 19 comments

Comments

Projects
None yet
7 participants
@jancurn

jancurn commented Nov 10, 2016

Hi there, after I upgraded blaze@2.1.9 to blaze@2.2.0 in our Meteor app, the 'readonly' attributes on tag are no longer rendered. This only happens if the tag has some dynamically generated attributes, for example:

<input type="text" readonly value="{{todo.text}}">

This one works correctly:

<input type="text" readonly>

I replicated the problem also in "Todos" example app. I cloned the repo from https://github.com/meteor/todos, updated meteor and all packages to their latest versions and have the same problem (actually I tested the above code in Todos app). When I downgraded to blaze@2.1.9, readonly attribute is rendered (although not as boolean attribute like 'disabled', but that's another story). Strangely enough, other attributes like 'disabled' are rendered normally.

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Nov 10, 2016

Collaborator

So this might be related to #102. @gcacars, could you take a look?

@jancurn: could you make a pull request with a test to Blaze to test for this case? Are we missing a test for this?

Collaborator

mitar commented Nov 10, 2016

So this might be related to #102. @gcacars, could you take a look?

@jancurn: could you make a pull request with a test to Blaze to test for this case? Are we missing a test for this?

@jancurn

This comment has been minimized.

Show comment
Hide comment
@jancurn

jancurn Nov 11, 2016

re @mitar: I will try to add some test, although not sure when I find time for it :-/

jancurn commented Nov 11, 2016

re @mitar: I will try to add some test, although not sure when I find time for it :-/

@thearabbit

This comment has been minimized.

Show comment
Hide comment
@thearabbit

thearabbit Nov 13, 2016

I have the same problem

// Work fine for blaze 2.1.9, but now don't work
{{> afQuickField name='branchId' readonly=true}}

How to fix?

I have the same problem

// Work fine for blaze 2.1.9, but now don't work
{{> afQuickField name='branchId' readonly=true}}

How to fix?

@gcacars

This comment has been minimized.

Show comment
Hide comment
@gcacars

gcacars Nov 16, 2016

Contributor

I done some tests with my project.

Template.login.helpers({
    attrs(){
        return {
            readonly: false
        };
    },

    test(){
        return false;
    }
});

<input class="form-control" type="text" name="username" {{attrs}} disabled="{{test}}" required>

Any helper like disabled (helper in attribute) works fine.
But nothing in attrs works correctly.
I think that Blaze do something different with dynamic attributes.
@mitar Could you help with this? Where in the code Blaze treat this.

I also test an input like jancurn said:
<input type="text" readonly value="{{todo.text}}">

For me works fine.

Contributor

gcacars commented Nov 16, 2016

I done some tests with my project.

Template.login.helpers({
    attrs(){
        return {
            readonly: false
        };
    },

    test(){
        return false;
    }
});

<input class="form-control" type="text" name="username" {{attrs}} disabled="{{test}}" required>

Any helper like disabled (helper in attribute) works fine.
But nothing in attrs works correctly.
I think that Blaze do something different with dynamic attributes.
@mitar Could you help with this? Where in the code Blaze treat this.

I also test an input like jancurn said:
<input type="text" readonly value="{{todo.text}}">

For me works fine.

@thearabbit

This comment has been minimized.

Show comment
Hide comment
@thearabbit

thearabbit Nov 17, 2016

Thanks, but I use AutoForm so I can't use like this

Template.login.helpers({
    attrs(){
        return {
            readonly: false
        };
    },
});
--------
{{> afQuickField name='branchId' attrs}}

Could you help me?

Thanks, but I use AutoForm so I can't use like this

Template.login.helpers({
    attrs(){
        return {
            readonly: false
        };
    },
});
--------
{{> afQuickField name='branchId' attrs}}

Could you help me?

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Nov 17, 2016

Collaborator

@gcacars: Maybe here or here?

But where does your pull request interfere with that?

Collaborator

mitar commented Nov 17, 2016

@gcacars: Maybe here or here?

But where does your pull request interfere with that?

@thearabbit

This comment has been minimized.

Show comment
Hide comment
@thearabbit

thearabbit Nov 17, 2016

Now I downgrade to 2.1.9

Now I downgrade to 2.1.9

@bluefangs

This comment has been minimized.

Show comment
Hide comment
@bluefangs

bluefangs Nov 17, 2016

@thearabbit - Could you please tell me how you downgraded to 2.1.9?

@thearabbit - Could you please tell me how you downgraded to 2.1.9?

@thearabbit

This comment has been minimized.

Show comment
Hide comment
@thearabbit

thearabbit Nov 17, 2016

please look in

.meteor/
   versions

And then find the blaze@2.1.9

thearabbit commented Nov 17, 2016

please look in

.meteor/
   versions

And then find the blaze@2.1.9

@jancurn

This comment has been minimized.

Show comment
Hide comment
@jancurn

jancurn Nov 17, 2016

hey guys, I wanted to write a test for this bug, but I wasn't able to find out how to run tests in this project...

jancurn commented Nov 17, 2016

hey guys, I wanted to write a test for this bug, but I wasn't able to find out how to run tests in this project...

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Nov 17, 2016

Collaborator

One approach is explained in #70. What I do is I configure METEOR_PACKAGES_DIRS to packages directory of this repository. And then I run from a package I am testing (like blaze):

meteor test-packages ./
Collaborator

mitar commented Nov 17, 2016

One approach is explained in #70. What I do is I configure METEOR_PACKAGES_DIRS to packages directory of this repository. And then I run from a package I am testing (like blaze):

meteor test-packages ./
@gcacars

This comment has been minimized.

Show comment
Hide comment
@gcacars

gcacars Nov 18, 2016

Contributor

@mitar I put a break point inside Blaze._makeAttributeHandler and BooleanHandler but nothing hits both break points, I don't know why.
I will take a look in this HTML.flattenAttributes.

Contributor

gcacars commented Nov 18, 2016

@mitar I put a break point inside Blaze._makeAttributeHandler and BooleanHandler but nothing hits both break points, I don't know why.
I will take a look in this HTML.flattenAttributes.

@gcacars

This comment has been minimized.

Show comment
Hide comment
@gcacars

gcacars Nov 19, 2016

Contributor

With the same example, the debug shows me:

image

The first object is the attributes declared in HTML. But disabled have a null value here (in helper, this value is false, but I think that helper do your job later). The second object is the dynamic attributes, with readonly false as set in helper object.

image

But HTML.isNully don't interpret the value false as nully, adding it to the list that render attributes. So HTML is still rendered in the wrong way with readonly="false".

image

The final result object is:
image

Blaze._makeAttributeHandler (pull request change) never was called. (nor Blaze._materializeDOM, materializeTag, (function that call _makeAttributeHandler) and so on... could be because of Iron Router?)

Contributor

gcacars commented Nov 19, 2016

With the same example, the debug shows me:

image

The first object is the attributes declared in HTML. But disabled have a null value here (in helper, this value is false, but I think that helper do your job later). The second object is the dynamic attributes, with readonly false as set in helper object.

image

But HTML.isNully don't interpret the value false as nully, adding it to the list that render attributes. So HTML is still rendered in the wrong way with readonly="false".

image

The final result object is:
image

Blaze._makeAttributeHandler (pull request change) never was called. (nor Blaze._materializeDOM, materializeTag, (function that call _makeAttributeHandler) and so on... could be because of Iron Router?)

@gcacars

This comment has been minimized.

Show comment
Hide comment
@gcacars

gcacars Nov 19, 2016

Contributor

Maybe other commit is impacting. checked is not working too.

Contributor

gcacars commented Nov 19, 2016

Maybe other commit is impacting. checked is not working too.

@andreaderuvo andreaderuvo referenced this issue in aldeed/meteor-autoform Nov 21, 2016

Closed

novalidate attribute missed on rendered form #1545

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Nov 29, 2016

Collaborator

@gcacars: Any progress on fixing this?

Collaborator

mitar commented Nov 29, 2016

@gcacars: Any progress on fixing this?

@serkandurusoy

This comment has been minimized.

Show comment
Hide comment
@serkandurusoy

serkandurusoy Dec 5, 2016

People are reporting success with changing the attribute name from readonly to readOnly, which makes sense since that's the actual name on the dom object when fetched using the javascript api.

So if perhaps this is a case sensitivity issue, can't we just make the name comparison in

Blaze._makeAttributeHandler = function (elem, name, value) {
case insensitive?

People are reporting success with changing the attribute name from readonly to readOnly, which makes sense since that's the actual name on the dom object when fetched using the javascript api.

So if perhaps this is a case sensitivity issue, can't we just make the name comparison in

Blaze._makeAttributeHandler = function (elem, name, value) {
case insensitive?

@Netherdrake

This comment has been minimized.

Show comment
Hide comment
@Netherdrake

Netherdrake Dec 10, 2016

I can confirm that:

        autoform: {
            readOnly: true
        }

Works fine, but only on String input types.

I can confirm that:

        autoform: {
            readOnly: true
        }

Works fine, but only on String input types.

@mitar mitar closed this in 84c58a6 Dec 31, 2016

@bluefangs

This comment has been minimized.

Show comment
Hide comment
@bluefangs

bluefangs Dec 31, 2016

@mitar - Safe to upgrade to the latest version without any modifications? (readonly to readOnly) ?

@mitar - Safe to upgrade to the latest version without any modifications? (readonly to readOnly) ?

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Dec 31, 2016

Collaborator

I hope so. :-) 2.2.1 should work the same as 2.1.9. But I would love feedback. :-)

Collaborator

mitar commented Dec 31, 2016

I hope so. :-) 2.2.1 should work the same as 2.1.9. But I would love feedback. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment