Bypass click/mouse handlers for any contenteditable elements #9491

hanthomas · 2016-09-01T23:28:02Z

Fixes issues #9414 and #5937. I can't see any reason why ngMaterial would need to hijack clicks for elements that have contenteditable attribute set.

Fixes issues 9414 and 5937. I can't see any reason why ngMaterial would need to hijack clicks for elements that have contenteditable attribute set.

googlebot · 2016-09-01T23:28:04Z

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 you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2016-09-01T23:28:04Z

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 you signed the CLA as a corporation, please let us know the company's name.

hanthomas · 2016-09-01T23:56:16Z

I signed it!

hanthomas · 2016-09-01T23:56:53Z

I signed it!

googlebot · 2016-09-01T23:56:54Z

CLAs look good, thanks!

googlebot · 2016-09-01T23:56:54Z

CLAs look good, thanks!

ThomasBurleson · 2016-09-07T17:12:32Z

Pending/related to HammerJS efforts.

ThomasBurleson · 2016-09-07T17:12:41Z

@hanthomas - please add 1 or more unit tests.

hanthomas · 2016-09-14T15:34:20Z

@ThomasBurleson - Unit tests have been added to gesture.spec.js.

devversion · 2016-09-15T14:54:19Z

src/core/services/gesture/gesture.spec.js

+    // Click tests should only be enabled when `$$hijackClicks == true` (for mobile)
+
+    it('should not hijack click on contenteditable element', inject(function($document, $mdGesture) {
+      if ( $mdGesture.$$hijackClicks ) {


Where does this property exist? Never saw this one in our gesture service.

Also the tests should enable hijacking by default.

Hmm... you're right. I used one of the existing tests as a template, but apparently that property no longer exists. I'll make the correction.

devversion · 2016-09-15T14:54:29Z

src/core/services/gesture/gesture.spec.js

+    }));
+
+    it('should not hijack mousedown/mouseup on contenteditable element', inject(function($document, $mdGesture) {
+      if ( $mdGesture.$$hijackClicks ) {


Same for here

See reply to previous comment

devversion · 2016-09-15T14:55:32Z

src/core/services/gesture/gesture.spec.js

+        var spy2 = jasmine.createSpy('mouseup');
+        var el = angular.element('<div contenteditable>');
+
+        el.on('mousedown', spy1);


Where do you actually trigger the event? The tests seem to pass because you created that $$hijackClicks check, which always doesn't pass

True. So, once I take that condition out, it should pass if the mousedown and mouseup handlers are triggered.

I think it gets more difficult for this tests.

You need the element you want to trigger the mouse event on to the document.body to be able to test it with $mdGesture.

Then you need to dispatch a fake mouse event and determine if the triggered event from the event handlers is the same as the fake mouse event.

You're right... plus, now the tests fail. So, it looks like I have some work to do on this. Thanks for the explanation.

hanthomas · 2016-09-16T12:39:33Z

@devversion - It took a few tries, but I think the unit tests are correct now. My basic approach was to make sure that ngMaterial allows the mouse events to be processed by the handler attached to the element instead of the click hijacker.

devversion · 2016-10-01T20:05:22Z

src/core/services/gesture/gesture.spec.js

+    var el;
+
+    beforeEach(function() {
+      inject(function($mdGesture) {


You can simplify this

beforeEach(inject(function($mdGesture) { }));

devversion · 2016-10-01T20:06:20Z

src/core/services/gesture/gesture.spec.js

+    });
+
+    it('should not hijack click on contenteditable element', inject(function($document, $mdGesture) {
+      $document.triggerHandler('$$mdGestureReset');


Move this to the beforeEach, since you are repeating it for each test

devversion · 2016-10-01T20:09:45Z

src/core/services/gesture/gesture.spec.js

+
+      el.triggerHandler('click');
+
+      expect(spyClick).toHaveBeenCalled();


Shouldn't those checks also check for the event to not being prevented.

// Something like this var event = spyClick.calls.mostRecent().args[0]; bool = event.defaultPrevented

devversion · 2016-10-10T15:45:04Z

I will close that PR for now, because this is not a high priority on our queue right now, because it is planned to get rid of the $mdGesture service in the future (See #9362).

Anyways, Thanks for the PR!

kmturley · 2017-04-28T20:25:49Z

This issue also affects regular elements when using angular-material for other elements on the same page. Using this code fixes it: function mouseInputHijacker(ev) { var isKeyClick = !ev.clientX && !ev.clientY; if (!isKeyClick && !ev.$material && !ev.isIonicTap && !isInputEventFromLabelClick(ev) && !ev.target.nodeName === 'SELECT') { ev.preventDefault(); ev.stopPropagation(); } } Would be good to have a directive to disable click hijacking on specific elements

Bypass click/mouse handlers for any contenteditable elements

6ddef7b

Fixes issues 9414 and 5937. I can't see any reason why ngMaterial would need to hijack clicks for elements that have contenteditable attribute set.

hanthomas closed this Sep 1, 2016

hanthomas reopened this Sep 1, 2016

ThomasBurleson added the needs: unit tests This PR needs unit tests to cover the changes being proposed label Sep 7, 2016

ThomasBurleson added this to the 1.2.0 milestone Sep 7, 2016

ThomasBurleson assigned devversion Sep 7, 2016

Update gesture.spec.js

2be6bff

devversion suggested changes Sep 15, 2016

View reviewed changes

devversion added the needs: work label Sep 15, 2016

hanthomas added 7 commits September 15, 2016 11:28

Update gesture.spec.js

3351ef5

Update gesture.spec.js

79bd1cf

Update gesture.spec.js

ded6ec3

Update gesture.spec.js

c7248be

Update gesture.spec.js

f9b8389

Update gesture.spec.js

76d2555

Update gesture.spec.js

dd6a02d

ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Sep 15, 2016

Update gesture.spec.js

0bd8df7

devversion reviewed Oct 1, 2016

View reviewed changes

devversion removed the needs: unit tests This PR needs unit tests to cover the changes being proposed label Oct 1, 2016

devversion closed this Oct 10, 2016

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bypass click/mouse handlers for any contenteditable elements #9491

Bypass click/mouse handlers for any contenteditable elements #9491

hanthomas commented Sep 1, 2016 •

edited

Loading

googlebot commented Sep 1, 2016

googlebot commented Sep 1, 2016

hanthomas commented Sep 1, 2016

hanthomas commented Sep 1, 2016

googlebot commented Sep 1, 2016

googlebot commented Sep 1, 2016

ThomasBurleson commented Sep 7, 2016

ThomasBurleson commented Sep 7, 2016

hanthomas commented Sep 14, 2016

devversion Sep 15, 2016

hanthomas Sep 15, 2016

devversion Sep 15, 2016

hanthomas Sep 15, 2016

devversion Sep 15, 2016

hanthomas Sep 15, 2016

devversion Sep 15, 2016

hanthomas Sep 15, 2016

hanthomas commented Sep 16, 2016

devversion Oct 1, 2016

devversion Oct 1, 2016

devversion Oct 1, 2016

devversion commented Oct 10, 2016

kmturley commented Apr 28, 2017 •

edited

Loading


		el.triggerHandler('click');

		expect(spyClick).toHaveBeenCalled();

Bypass click/mouse handlers for any contenteditable elements #9491

Bypass click/mouse handlers for any contenteditable elements #9491

Conversation

hanthomas commented Sep 1, 2016 • edited Loading

googlebot commented Sep 1, 2016

googlebot commented Sep 1, 2016

hanthomas commented Sep 1, 2016

hanthomas commented Sep 1, 2016

googlebot commented Sep 1, 2016

googlebot commented Sep 1, 2016

ThomasBurleson commented Sep 7, 2016

ThomasBurleson commented Sep 7, 2016

hanthomas commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanthomas commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion commented Oct 10, 2016

kmturley commented Apr 28, 2017 • edited Loading

hanthomas commented Sep 1, 2016 •

edited

Loading

kmturley commented Apr 28, 2017 •

edited

Loading