fix(ngAria): check if element is contenteditable before blocking spac… #16762

Eblax · 2018-11-16T13:16:38Z

…ebar

ngAria blocks spacebar keypresses on non-blacklisted elements which is an issue
when the element is contenteditable

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
#16761

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

googlebot · 2018-11-16T13:16:40Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-11-16T13:16:41Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

Eblax · 2018-11-16T13:18:45Z

I signed it!

googlebot · 2018-11-16T13:18:47Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
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. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-11-16T13:18:47Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
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. If the bot doesn't comment, it means it doesn't think anything has changed.

Eblax · 2018-11-16T14:34:20Z

I signed it again!

googlebot · 2018-11-16T14:34:23Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-11-16T14:34:23Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Eblax · 2018-11-16T14:38:53Z

Added the email I used for the commit to my account.

gkalpak

LGTM, but we need some tests that verify the fix.

gkalpak · 2018-11-23T13:55:43Z

@Eblax, are you still interested in moving this forward?

googlebot · 2018-11-23T14:30:17Z

CLAs look good, thanks!

googlebot · 2018-11-23T14:30:17Z

CLAs look good, thanks!

Eblax · 2018-11-23T15:19:54Z

Yeah but writing a Jasmine test for this turned out to be a bit tricky.

Edit: That is, triggering a keypress inside a contenteditable programmatically is what I can't figure out how to do.

gkalpak · 2018-11-26T12:47:58Z

Thx for getting back, @Eblax. I'll take a look.

…ing spacebar `ngAria`'s `ngClick` blocks spacebar keypresses on non-blacklisted elements, which is an issue when the element is `contenteditable`.

…e blocking spacebar

googlebot · 2018-11-26T13:17:56Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-11-26T13:17:57Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

gkalpak · 2018-11-26T13:19:56Z

I added the test, @Eblax, and also moved it to a more appropriate block (where we test for similar stuff).
In case you are wondering, the browserTrigger() helper can be used for triggering fake events (and it also supports bubbling 🎉).

Eblax · 2018-11-26T13:37:56Z

Great, thanks a lot!

petebacondarwin

One question... otherwise LGTM

petebacondarwin · 2018-11-26T14:59:36Z

test/ngAria/ariaSpec.js

+        compileElement(
+          '<section>' +
+            '<div ng-click="onClick($event)" ng-attr-contenteditable="{{ edit }}"></div>' +
+            '<ul ng-click="onClick($event)"><li ng-attr-contenteditable="{{ edit }}"></li></ul>' +


Why not also check static contenteditable attributes, as well as dynamically setting the attribute via ng-attr...? Or is this already covered elsewhere?

This is just an easy way to add/remove the attribute. Since we are not really checking the attribute itself (but rather checking the isContentEditable property which is automatically updated by the browser based on the attribute), I felt this is fine.

…e blocking spacebar

…ing spacebar `ngAria`'s `ngClick` blocks spacebar keypresses on non-blacklisted elements, which is an issue when the element is `contenteditable`. Closes #16762

googlebot added the cla: no label Nov 16, 2018

gkalpak suggested changes Nov 16, 2018

View reviewed changes

gkalpak mentioned this pull request Nov 16, 2018

ngAria disables spacebar for contenteditable divs #16761

Closed

3 tasks

gkalpak added this to the 1.7.x milestone Nov 16, 2018

googlebot added cla: yes and removed cla: no labels Nov 23, 2018

Eblax and others added 2 commits November 26, 2018 15:14

fix(aria/ngClick): check if element is contenteditable before block…

3f23fd9

…ing spacebar `ngAria`'s `ngClick` blocks spacebar keypresses on non-blacklisted elements, which is an issue when the element is `contenteditable`.

fixup! fix(aria/ngClick): check if element is contenteditable befor…

628bd59

…e blocking spacebar

gkalpak force-pushed the master branch from 8634120 to 628bd59 Compare November 26, 2018 13:17

googlebot added cla: no and removed cla: yes labels Nov 26, 2018

gkalpak approved these changes Nov 26, 2018

View reviewed changes

gkalpak added the type: bug label Nov 26, 2018

gkalpak added needs: review component: ngAria severity: regression labels Nov 26, 2018

petebacondarwin approved these changes Nov 26, 2018

View reviewed changes

gkalpak force-pushed the master branch 4 times, most recently from 6938b3b to 35f0308 Compare November 28, 2018 16:25

fixup! fix(aria/ngClick): check if element is contenteditable befor…

2ed73c8

…e blocking spacebar

gkalpak force-pushed the master branch from 35f0308 to 2ed73c8 Compare November 28, 2018 16:39

gkalpak closed this in 0cdff42 Dec 2, 2018

This was referenced Dec 2, 2018

File Nesting angular-indonesia/angular.js#170

Merged

File Nesting javascript-indonesias/angular.js#124

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngAria): check if element is contenteditable before blocking spac… #16762

fix(ngAria): check if element is contenteditable before blocking spac… #16762

Eblax commented Nov 16, 2018

googlebot commented Nov 16, 2018

googlebot commented Nov 16, 2018

Eblax commented Nov 16, 2018

googlebot commented Nov 16, 2018

googlebot commented Nov 16, 2018

Eblax commented Nov 16, 2018

googlebot commented Nov 16, 2018

googlebot commented Nov 16, 2018

Eblax commented Nov 16, 2018

gkalpak left a comment

gkalpak commented Nov 23, 2018

googlebot commented Nov 23, 2018

googlebot commented Nov 23, 2018

Eblax commented Nov 23, 2018 •

edited

Loading

gkalpak commented Nov 26, 2018

googlebot commented Nov 26, 2018

googlebot commented Nov 26, 2018

gkalpak commented Nov 26, 2018

Eblax commented Nov 26, 2018

petebacondarwin left a comment

petebacondarwin Nov 26, 2018

gkalpak Nov 26, 2018

fix(ngAria): check if element is contenteditable before blocking spac… #16762

fix(ngAria): check if element is contenteditable before blocking spac… #16762

Conversation

Eblax commented Nov 16, 2018

googlebot commented Nov 16, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Nov 16, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Eblax commented Nov 16, 2018

googlebot commented Nov 16, 2018

googlebot commented Nov 16, 2018

Eblax commented Nov 16, 2018

googlebot commented Nov 16, 2018

googlebot commented Nov 16, 2018

Eblax commented Nov 16, 2018

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak commented Nov 23, 2018

googlebot commented Nov 23, 2018

googlebot commented Nov 23, 2018

Eblax commented Nov 23, 2018 • edited Loading

gkalpak commented Nov 26, 2018

googlebot commented Nov 26, 2018

googlebot commented Nov 26, 2018

gkalpak commented Nov 26, 2018

Eblax commented Nov 26, 2018

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Nov 26, 2018

Choose a reason for hiding this comment

gkalpak Nov 26, 2018

Choose a reason for hiding this comment

Eblax commented Nov 23, 2018 •

edited

Loading