Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(autocomplete, select, virtualrepeat, input): correctly validate attribute values #6803

Conversation

devversion
Copy link
Member

Fixes the validation of the boolean attributes.

Autocomplete

Attributes:

required disabled
  • required ignored an empty attribute value (<md-autocomplete required>)
  • disabled ignored an empty attribute value (<md-autocomplete disabled>)

Input

Attributes:

md-no-asterisk
- `md-no-asterisk` is ignoring false attribute value (``)

Extra Changes:

  • required is not updating the md-required class (now observing for ng-required)
  • Wrong test-case for md-no-asterisk

Select

Attributes:

multiple
- `multiple` is ignoring false attribute value (``)

Virtual Repeat

Attributes

md-on-demand
- `md-on-demand` is ignoring false attribute value (``)

Extra Changes:

  • Missing attribute test for md-on-demand

Notes

  • Validating negative attribute values for required, readonly, disabled will be ignored.
  • Validating 0 as falsy value is also a useful feature and should be implemented too, because sometimes your backend will return a boolean in numeric style (See here).

Fixes #5831 Fixes #7063 Fixes #6415 Fixes #6393 Fixes #5896 Fixes #6698 Fixes #7105

@@ -163,8 +163,8 @@ function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming,
*/
function configureWatchers () {
var wait = parseInt($scope.delay, 10) || 0;
$attrs.$observe('disabled', function (value) { ctrl.isDisabled = !!value; });
$attrs.$observe('required', function (value) { ctrl.isRequired = !!value; });
$attrs.$observe('disabled', function (value) { ctrl.isDisabled = value !== 'false'; });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the value not interpolate to '0' also ? This solutions seems limited.

@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from e6c68b4 to 5cb2785 Compare January 26, 2016 21:37
@devversion
Copy link
Member Author

@ThomasBurleson - It now supports interpolated 0 values. Added a util method for attribute parsing, this will be helpful for #6714 too (if a util method isn't desired let me know :) ) + accompanying tests.

@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from 5cb2785 to ab5f6c4 Compare January 27, 2016 12:53
@devversion devversion changed the title fix(autocomplete): correctly validate required and disabled attribute fix(autocomplete, select): correctly validate attribute values as boolean Jan 27, 2016
@devversion
Copy link
Member Author

-- Now fixing #6698 too, after merging my old PR into this by using the new util method.

* @param value Interpolated Attribute Value
* @returns {boolean}
*/
parseAttributeBoolean: function(value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jelbourn - I think we should regex match on the value string. Thoughts or code suggestions ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't do a negated match for /(false|0)/g?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't consider the "zero" string to be falsy. Without that the check is fine as-is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devversion Can you remove the check for '0' and update the PR? Other than that, it looks good to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertmesserle - Yes will do, just thought I need that because Thomas asked about the interpolated zero as a falsy value.

@ThomasBurleson ThomasBurleson added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: feedback The issue creator or community need to respond to questions in this issue and removed needs: work labels Jan 30, 2016
@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from ab5f6c4 to 6721dbf Compare February 3, 2016 12:20
@devversion
Copy link
Member Author

@ThomasBurleson @robertmesserle @jelbourn - Okay removed the zero check (but kept the initial check structure). But in my opinion I think zero should be used as a falsy value too.

@robertmesserle
Copy link
Contributor

Just to clarify, the use-case would be something like:

<md-autocomplete ... required="0"></md-autocomplete>

Is that right?

I don't have a strong opinion one way or the other, I'll defer to @ThomasBurleson and @jelbourn for this.

@devversion
Copy link
Member Author

Okay, just wan't to answer your question 😄 .

  • Yes exactly this will allow using zero as false value too. This would be really useful when using something like a backend which is returning booleans in numeric style.

@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from 6721dbf to 38b20f3 Compare February 8, 2016 08:54
@devversion devversion changed the title fix(autocomplete, select): correctly validate attribute values as boolean fix(autocomplete, select, virtualrepeat): correctly validate attribute values as boolean Feb 8, 2016
@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from 38b20f3 to d86f34a Compare February 8, 2016 09:00
@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from d86f34a to 5688de2 Compare February 8, 2016 12:59
@devversion
Copy link
Member Author

@ThomasBurleson At the moment in this PR, in most cases we support required="false" required="true", so the required attribute is accepting a boolean as value. But this is not a default behavior in HTML from W3C.

The users should better use ng-disabled, ng-readonly and ng-required.
So IMO we should not parse the attribute values from these type of attributes.

  • disabled
  • readonly
  • required

Just tell me, then I will remove the new checks for these types. But please think about the issues too (#5896, #6415)

@jelbourn
Copy link
Member

jelbourn commented Feb 8, 2016

While treating these as boolean attributes is correct, it would be a breaking API change.

@devversion
Copy link
Member Author

@jelbourn Sorry, I didn't get it. Should we continue this.. or should I remove that?

Edit: Ah, you answered to @robertmesserle's message.

@devversion devversion changed the title fix(autocomplete, select, virtualrepeat): correctly validate attribute values as boolean fix(autocomplete, select, virtualrepeat, input): correctly validate attribute values Feb 10, 2016
@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from 3c4d106 to 5412559 Compare February 10, 2016 22:07
@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from 5412559 to 4b4a226 Compare February 11, 2016 09:13
@EladBezalel
Copy link
Member

@jelbourn according what I've read this PR doesn't seems like a breaking API, it just enables wider range of values, more than that the old behavior won't break.

@jelbourn
Copy link
Member

@EladBezalel We would be changing the behavior of, for example, <md-select disabled="false">. Someone might be depending on this already, so changing it would be a breaking API change.

@devversion
Copy link
Member Author

@jelbourn I've changed that a few days ago, it's currently only a issue of the autocomplete directive (required, disabled). Should I add a parameter to the util function which excludes the negative checks (this can be used for attributes like readonly, disabled, required), because in general I need the negative checks for attributes like md-no-asterisk or multiple. I think this will be a good solution. Thoughts?

BTW: md-chips also retrieves a boolean (like <md-chips readonly="true"> - <md-chips readonly> isn't working)

@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from 4b4a226 to 29ad320 Compare February 12, 2016 19:49
@devversion devversion force-pushed the fix/autocomplete-required-disabled-validation branch from 29ad320 to 71cd708 Compare February 12, 2016 19:55
@wembernard
Copy link

@devversion May I suggest to add required attribute in select section?

@devversion
Copy link
Member Author

@wembernard I would like to have this PR merge, before looking at the actual select issue, because this may not relate to the actual boolean validation.

BTW: As discussed with @jelbourn I removed the breaking attribute changes. PTAL

@leocaseiro
Copy link
Contributor

Will this PR fix the issue with multiple="false"?

If yes, I vote +1

UPDATE

I found the solution for multiple="false", we should bind using ng-multiple. As @rschmukler mentioned at #5459

@devversion
Copy link
Member Author

@leocaseiro Yes, that will be a good workaround for that, and yes this PR adds a basic boolean validation for multiple, this should be added, because the only attributes which should not validate for negated boolean values are readonly, required, disabled...

@leocaseiro
Copy link
Contributor

how about ng-required, ng-multiple, ng-disabled and ng-readonly?

@devversion
Copy link
Member Author

@leocaseiro Yes exactly that's the right usage for these attributes. By default readonly, required, disabled (which are native attributes - without the md- prefix) are not interpolating a boolean value. That's why you should use the ng- prefixed attributes for these three native attributes, when you wan't to interpolate the boolean.

@devversion devversion added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: feedback The issue creator or community need to respond to questions in this issue labels Feb 25, 2016
@ThomasBurleson ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Feb 25, 2016
@ThomasBurleson ThomasBurleson added this to the 1.0.6 milestone Feb 25, 2016
@ThomasBurleson ThomasBurleson self-assigned this Feb 25, 2016
@devversion devversion deleted the fix/autocomplete-required-disabled-validation branch April 19, 2016 19:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
- Breaking Change pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants