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

Can't write space chars in input when it in tag with ng-click with ngAria since 1.7.3 #16664

Closed
1 of 3 tasks
Sergi0Limit opened this issue Aug 16, 2018 · 21 comments · Fixed by #16680
Closed
1 of 3 tasks

Comments

@Sergi0Limit
Copy link

I'm submitting a ...

  • bug report
  • feature request
  • other

Current behavior:

When input/textarea in the wrapped tag with ng-click i can't wrote space chars in the text.

Expected / new behavior:

When the input / textarea in the wrapped tag with ng-click i should be able to write space characters in the text.

Minimal reproduction of the problem with instructions:
Demo https://codepen.io/sergi0limit/pen/mjYLGE

AngularJS version: 1.7.3

Browser: all

Anything else:

commit 6c224a2

@hoeni
Copy link

hoeni commented Aug 16, 2018

We have the same problem here. When upgrading from 1.7.2 to 1.7.3 all presses of the space key are lost in all input and textarea fields.

Maybe there is something wrong with the following change (We do use ngAria, indeed)?:

ngAria: do not scroll when pressing spacebar on custom buttons (3a517c, #14665, #16604)

@Sergi0Limit
Copy link
Author

Sergi0Limit commented Aug 16, 2018

@hoeni problem in ngClick that in ngAria.js. If you are using ngMaterial module than need ngAria module.

@petebacondarwin
Copy link
Contributor

I think you are right @hoeni - this is because of that ngAria change. You can see in this version of the CodePen that pressing space causes the click handler: https://codepen.io/petebacondarwin/pen/OweOYK?editors=1111

I guess we need to ensure that ngAria does not prevent default on input/textarea elements?

@gkalpak
Copy link
Member

gkalpak commented Aug 16, 2018

Yeah, ngAria assumes that any element with ngClick is a custom button (adding the button role, handling ENTER/SPACEBAR as clicks, etc.).

Doing this for input/select/textarea elements (except for input[type"button/reset/submit"]) doesn't make sense indeed.

@Sergi0Limit
Copy link
Author

@gkalpak but it's not realy true for all solutions. In my case i have multiple div as row in list and it have some fields and input field.
These divs can selected by click.

@gkalpak
Copy link
Member

gkalpak commented Aug 16, 2018

Not sure what you mean, @Sergi0Limit 😕

@Sergi0Limit
Copy link
Author

I mean... Blocking space chars in my case is this a bug or a feature?

@gkalpak
Copy link
Member

gkalpak commented Aug 16, 2018

Preventing tge default browser behavior (i.e. scrolling down) on the divs sounds like the right thing to do, no?

@Sergi0Limit
Copy link
Author

Focus in the input, not on div. Why it do click instead of space?

@Narretz Narretz changed the title Can't write space chars in input when it in tag with ng-click Can't write space chars in input when it in tag with ng-click with ngAria since 1.7.3 Aug 17, 2018
@Narretz
Copy link
Contributor

Narretz commented Aug 17, 2018

Here are my thoughts on this:
This is expected behavior.
ngAria automatically adds role=button to elements with ngClick that are not natively interactive. This is correct and necessary.

However, if you have an input or a button, and on one of its parent elements use ngClick, you don't want this ngClick to be role=button. This is because it's not logical to have two nested buttons. I'd assume screen readers and other assistive technology will have problems with this.

HTML actually disallows the nesting of most interactive elements. Of course, a div with a click event is not native interactive content, but once you include ngAria into the mix, you make it "interactive":
Here's a good summary for this: http://adrianroselli.com/2016/12/be-wary-of-nesting-roles.html

So I can think of two ways to go from here:

  • use ng-aria-disable on the ngClick element
  • use ng-event-click ng-on-click instead of ngClick on the element
  • use addEventListener directly from a directive / service

@hoeni
Copy link

hoeni commented Aug 17, 2018

In our case, we have a rather complex form generator. If you click a whole form line with SHIFT and ALT pressed, it will open a form inspector (for form debugging by developers). This is the surrounding ng-click. I can confirm that removing the ngClick handler makes the space key work again in contained inputs.

On the one hand I can understand that ngAria should clearly regard space key press as a button press and suppress page scrolling when activated. On the other hand I see this as a rather breaking change I would not expect in a minor AngularJS Update without prior heads up.

@Narretz :

  • ng-aria-disable on our ng-click element fixes our problem. Spaces work and also the click handler.
  • ng-event-click instead of ngClick does not trigger the handler. I have never seen that notation, can you give me some pointer?

hoeni pushed a commit to hoeni/angular.js that referenced this issue Aug 17, 2018
add warning about changed space key in inputs

angular#16664
@Narretz
Copy link
Contributor

Narretz commented Aug 17, 2018

Sorry, ng-event-click is wrong, correct is ng-on-click. https://codepen.io/narretz/pen/ajeZmZ This is also new in 1.7.3: https://docs.angularjs.org/api/ng/directive/ngOn

It's imo still debatable if this is a breaking change. Nesting interactive elements like this was wrong even before this change. Although we'll probably have to revert this if it turns out many apps are affected.

@Sergi0Limit
Copy link
Author

@hoeni
Problem solved by ng-aria-disable me too.

@Sergi0Limit
Copy link
Author

@Narretz
ng-on-click this is good idea) Thanks.

@petebacondarwin
Copy link
Contributor

I think that ng-aria-disable is the best workaround. Thanks for the write up @Narretz. We should document that. (And perhaps the Material project should do so too! @Splaktar?)

@gkalpak
Copy link
Member

gkalpak commented Aug 20, 2018

Hm...I didn't realize this also affects child elements. In that case, I don't think this is a BC worth having.
Maybe we could change preventDefault() to only happen if the ngClick element is the target of the keydown event only (here):

 if (keyCode === 13 || keyCode === 32)
-  event.preventDefault();
+  if (event.target === elem[0]) event.preventDefault();
   scope.$apply(callback);
 }

We could additionally check whether nodeBlackList.includes(event.target.nodeName).

@Narretz
Copy link
Contributor

Narretz commented Aug 20, 2018

Hm, I don't think labelling this as a BC is correct if we assume that setting ngClick -> input element is incorrect in the first place (with ngAria)
Even if we try to improve the space press behavior, the constellation still introduces a case where you have an element with role="button" and as a child another interactive element, and this incorrect based on aria rules: https://w3c.github.io/html-aria/#allowed-aria-roles-states-and-properties

I feel like we shouldn't make it easier again to introduce a suboptimal aria state in apps, especially with ngAria included.

Sergi0Limit referenced this issue Aug 24, 2018
By default, pressing spacebar causes the browser to scroll down.
However, when a native button is focused, the button is clicked instead.

`ngAria`'s `ngClick` directive, sets elements up to behave like buttons.
For example, it adds `role="button"` and forwards `ENTER` and `SPACEBAR`
keypresses to the `click` handler (to emulate the behavior of native
buttons).

Yet, pressing spacebar on such an element, still invokes the default
browser behavior of scrolling down.

This commit fixes this, by calling `preventDefault()` on the keyboard
event, thus preventing the default scrolling behavior and making custom
buttons behave closer to native ones.

Closes #14665

Closes #16604
gkalpak referenced this issue in angular/bower-angular-aria Aug 27, 2018
@gkalpak
Copy link
Member

gkalpak commented Aug 27, 2018

Still, having people's apps break like this (in a patch release), is bad imo.
FWIW, there are apps that are not great ARIA-wise, but work. With this change they will break 😕

@Splaktar
Copy link
Member

I tested all of the AngularJS Material docs with 1.7.3 and I don't see any breakages there related to the space key being blocked on any kind of inputs. This is likely due to us not nesting inputs inside of interactive elements.

That said, I'm sure that many developers who are using AngularJS Material are doing this. I've seen it in the past (and warned people about it).

AngularJS Material recently fixed a related issue with ngAria where it was adding a role="button" to placeholders in our inputs. We were able to fix this issue via aria-hidden="true" but I don't believe that is necessarily the right fix for this issue.

I would suggest that the release notes should warn developers about this breaking change coming in the future, provide them with clear guidance on why nesting interactive elements is bad, and then move this change to a future release.

@mgol
Copy link
Member

mgol commented Aug 27, 2018

I agree with @gkalpak & @Splaktar we should fix this issue as it may break many users. We're (almost?) feature-frozen now so we should never re-apply this back, though. It's IMO too late for such disruptive changes, especially in a patch release, even if they're technically right.

@Narretz
Copy link
Contributor

Narretz commented Aug 28, 2018

We can add an addtional check as suggested by @gkalpak:

check whether nodeBlackList.includes(event.target.nodeName).

and also possibly check if the target element has aria-roles that make it interactive (i.e. button).

This way, we get the better aria support and still allow this use case.

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