fix(dialog): fix prompt closing on ENTER key when input is required and empty #10990

eknowles · 2017-11-20T11:46:17Z

When using the required method in $mdDialog.prompt() the user is able to hide the dialog by hitting the ENTER key even when the input is empty, defeating the implication of a required input. This PR adds a check on keypress to ensure if the dialog is prompt and is required that a value is present.

Adds missing documentation for required method.
Adds missing test for required method.

Fixes #10953

codepen with issue reproducible https://codepen.io/eknowles/pen/JOpgrq

…nd empty * Adds missing documentation for `required` method. * Adds missing test for `required` method. Fixes 10953

googlebot · 2017-11-20T11:46:27Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

eknowles · 2017-11-20T14:05:34Z

I signed it!

googlebot · 2017-11-20T14:05:36Z

CLAs look good, thanks!

eknowles · 2017-11-20T17:57:50Z

I'd be happy to add a demo to the documentation for this if anyone thinks it would benefit

Splaktar · 2017-11-21T07:17:16Z

@eknowles a simple CodePen to show the problematic behavior would be a good first step since the original issue did not include that.

Thank you very much for your contribution.

Splaktar · 2017-11-21T07:27:38Z

@eknowles as mentioned in #10953 (comment), a CodePen isn't needed as the Dialog demos in the docs demonstrate this issue clearly.

eknowles · 2017-11-21T08:28:47Z

No worries, if needs be, I've got a codepen here showing it https://codepen.io/eknowles/pen/JOpgrq

Splaktar

This is a very good start. I think that it just needs one minor change and one more test. Thank you!

Splaktar · 2018-01-26T19:38:18Z

src/components/dialog/dialog.spec.js

@@ -781,6 +781,34 @@ describe('$mdDialog', function() {

      expect(response).toBe('responsetext');
    }));
+
+    it('should not submit after ENTER key when input is empty and prompt is required', inject(function($mdDialog, $rootScope, $timeout, $mdConstant) {


This is good, but I think that we should also have a test that: when required is set and the input has a value that hide is called and the result is correct.

Splaktar · 2018-01-26T19:44:49Z

src/components/dialog/dialog.js

@@ -658,7 +659,9 @@ function MdDialogProvider($$interimElementProvider) {
        $mdDialog.cancel();
      };
      this.keypress = function($event) {
-        if ($event.keyCode === $mdConstant.KEY_CODE.ENTER) {
+        var invalidPrompt = isPrompt && this.required && !this.result;


I don't think that this check is accurate enough. The input could be asking for the number of children and 0 would be a valid response. However !this.result would be false and flag the result as invalid in that case. angular.isDefined(this.result) is a safer approach and is used in many other components.

…value

eknowles · 2018-02-09T11:02:58Z

@Splaktar I've added that missing test case and fix the value check change, sorry for the delay!

Splaktar

LGTM

…nd empty (angular#10990) * fix(dialog): fix prompt closing on ENTER key when input is required and empty * Adds missing documentation for `required` method. * Adds missing test for `required` method. Fixes 10953 * fix(dialog): fix issue where prompt value of 0 would pass required check * test(dialog): check prompt should call hide when is required and has value

…nd empty (#10990) * fix(dialog): fix prompt closing on ENTER key when input is required and empty * Adds missing documentation for `required` method. * Adds missing test for `required` method. Fixes 10953 * fix(dialog): fix issue where prompt value of 0 would pass required check * test(dialog): check prompt should call hide when is required and has value

fix(dialog): fix prompt closing on ENTER key when input is required a…

67c6abb

…nd empty * Adds missing documentation for `required` method. * Adds missing test for `required` method. Fixes 10953

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Nov 20, 2017

googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Nov 20, 2017

Splaktar requested a review from mgol November 21, 2017 07:17

Splaktar added this to the 1.2.0 milestone Nov 21, 2017

Splaktar added needs: review This PR is waiting on review from the team P3: important Important issues that really should be fixed when possible. labels Nov 21, 2017

Splaktar modified the milestones: 1.2.0, 1.1.6 Nov 21, 2017

angular deleted a comment from googlebot Nov 21, 2017

Splaktar modified the milestones: 1.1.6, 1.1.7 Jan 17, 2018

Splaktar self-assigned this Jan 26, 2018

Splaktar suggested changes Jan 26, 2018

View reviewed changes

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs type: bug ux: integration and removed needs: review This PR is waiting on review from the team labels Jan 26, 2018

Splaktar modified the milestones: 1.1.7, 1.1.8 Feb 7, 2018

fix(dialog): fix issue where prompt value of 0 would pass required check

3089b77

test(dialog): check prompt should call hide when is required and has …

47313af

…value

Splaktar approved these changes Feb 11, 2018

View reviewed changes

Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Feb 11, 2018

andrewseguin merged commit 69470a6 into angular:master Feb 16, 2018

GulajavaMinistudio mentioned this pull request Feb 17, 2018

Update upstream GulajavaMinistudio/material#39

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(dialog): fix prompt closing on ENTER key when input is required and empty #10990

fix(dialog): fix prompt closing on ENTER key when input is required and empty #10990

Uh oh!

eknowles commented Nov 20, 2017 •

edited

Loading

Uh oh!

googlebot commented Nov 20, 2017

Uh oh!

eknowles commented Nov 20, 2017

Uh oh!

googlebot commented Nov 20, 2017

Uh oh!

eknowles commented Nov 20, 2017

Uh oh!

Splaktar commented Nov 21, 2017

Uh oh!

Splaktar commented Nov 21, 2017

Uh oh!

eknowles commented Nov 21, 2017

Uh oh!

Splaktar left a comment

Uh oh!

Splaktar Jan 26, 2018

Uh oh!

Splaktar Jan 26, 2018

Uh oh!

eknowles Jan 28, 2018

Uh oh!

eknowles commented Feb 9, 2018

Uh oh!

Splaktar left a comment

Uh oh!

Uh oh!

fix(dialog): fix prompt closing on ENTER key when input is required and empty #10990

fix(dialog): fix prompt closing on ENTER key when input is required and empty #10990

Uh oh!

Conversation

eknowles commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Nov 20, 2017

Uh oh!

eknowles commented Nov 20, 2017

Uh oh!

googlebot commented Nov 20, 2017

Uh oh!

eknowles commented Nov 20, 2017

Uh oh!

Splaktar commented Nov 21, 2017

Uh oh!

Splaktar commented Nov 21, 2017

Uh oh!

eknowles commented Nov 21, 2017

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Splaktar Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

Splaktar Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

eknowles Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

eknowles commented Feb 9, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eknowles commented Nov 20, 2017 •

edited

Loading