Checks if an element is able to trigger an attached delegate click event - Fixes #134 #157

fernandosouza · 2016-09-20T01:16:16Z

No description provided.

fernandosouza · 2016-09-20T01:30:39Z

packages/metal-dom/src/dom.js

-			ret &= dom.triggerMatchedListeners_(container, currElement, event, defFns);
-			currElement = currElement.parentNode;
-		}
+		if (dom.isAbleToInteractWith_(currElement, event.type)) {


This diff seems kind of messy but I've only written this if statement.

In my opinion, this method is becoming difficult to read. Maybe we can make improvements on it. I have some suggestions:

1 - We can create a new method for checking the isAbleToInteractWith_ condition before this handleDelegateEvent_ method, or, put the logic of the handleDelegateEvent_ in a new one.

2 - We can keep the checking of isAbleToInteractWith_ here, in this method, but above the code, and then, we can return the default value of the ret variable. For me, it is strange to return the same variable in two cases, at the end of the code and at the beginning�, but, it is an options.

I would like to hear your opinion @mairatma.

You're right, this is getting more complex. I think that the complexity is coming mainly from the for/while loops though, not just this new if case, so ideally we should instead find a way to break this method into smaller parts. There could be a function that covers to logic of running the default listeners for example, a triggerDefaultFns_(event, defFns), and another for the while loop in the same way. This would clean this up a lot, don't you think?

If you wish you could do this in this pr, or it could be a separate pr sent later, so that we can merge this fix sooner. For this fix to get in, the only thing missing right now is a test :)

It is up to you, @mairatma. :)

If you have time then sending everything would be great :) But just make sure you add the test and I'll merge, even if the refactoring comes separately later.

fernandosouza · 2016-09-28T04:07:11Z

Branch updated @mairatma

mairatma · 2016-09-28T17:06:19Z

Just started reviewing :)

Sent from GH.

mairatma · 2016-09-28T17:42:27Z

packages/metal-dom/test/dom.js

+        		var target = element.querySelector('.match');
+        		target.dispatchEvent(eventObj);
+
+				assert.strictEqual(0, listener.callCount);


This test just made me notice something weird. What this test is doing is:

Creating a disabled button with a span content.

Listening to clicks on the span.

Checking that the listener isn't called.

The problem is that in the browser, that listener is supposed to be called. I just tried this same use case myself in Chrome, and clicking the span will still trigger the span's listener, it will only stop triggering the button's listener, since that's the element that was disabled.

So the fact that this test is passing shows that something is wrong with the logic. The logic should be such that the span will still trigger listeners, but not the disabled button. There should be a test that listens to delegate on the button (dom.delegate(element, 'click', 'button', listener) for example), and makes sure that clicking the span does not cause that listener to be triggered. If the listener is on the span it should keep being triggered though, so this test should be kept, but also updated to check that the callCount is 1, not 0.

This seems to be showing that isAbleToInteractWith_ is doing the wrong check. The link that you posted on the issue explaining how this should work seems to also be saying this. It basically says that a disabled element should prevent clicks on itself, and that disabled elements are only button, input, select or textarea that are disabled (not their children too). It also mentions descendants of disabled fieldset but I also tested this on Chrome, and could only reproduce when these descendants are also button, input, select or textarea, the others still get the event triggered regardless of the parent.

So I think that we need to change isAbleToInteractWith_ to only return false if the given node is button, input, select or textarea, and is either disabled or a child of a disabled fieldset. Then, in handleDelegateEvent_ this should be called for each listener triggered in each element, not just for the first target, so probably inside triggerListeners_ instead of handleDelegateEvent_.

Let me know if you have any questions about this, it's hard to explain it all in a single comment :)

Yeah, @mairatma. You are totally right. I will fix it. Thx for your help.

eduardolundgren · 2016-09-29T14:33:01Z

Guys, we should be careful with very custom methods to not become too specific.

mairatma · 2016-09-29T14:35:56Z

Which one in particular do you mean, @eduardolundgren?

mairatma

Thanks for the changes, I think this is closer to the expected behavior now 😄.

There's just one more thing related to the behavior that I've commented inline, and some other minor comments for improvements.

Other than those, could you just rename the most recent commit to explain in more detail what it's doing? It helps to have good commit titles when looking at the commit history. Also, we have a pattern of always having titles as if they're starting with the word It, indicating that this is what it (the commit) does. So instead of having something like Fix indentation it would be Fixes indentation.

mairatma · 2016-09-29T14:46:57Z

packages/metal-dom/src/dom.js

@@ -318,7 +318,7 @@ function handleDelegateEvent_(event) {
 	var limit = event.currentTarget.parentNode;
 	var defFns = [];

-	while (currElement && currElement !== limit && !event.stopped) {
+	while (currElement && isAbleToInteractWith_(currElement, event.type) && currElement !== limit && !event.stopped) {


Putting this check here will cause us to stop propagation of the event to all elements after the disabled one, but I've just checked here in Chrome and the browser actually keeps bubbling the event upwards in this case, it only skips the disabled element itself, not its parents too. To make that work as expected I believe this check needs to be inside triggerMatchedListeners_ instead of here, so that the loop can keep going on even when the event is not triggered for a given element.

mairatma · 2016-09-29T14:58:53Z

packages/metal-dom/src/dom.js

@@ -265,11 +265,11 @@ export function delegate(element, eventName, selectorOrTarget, callback, opt_def
 function isAbleToInteractWith_(node, eventName) {
 	var currElement = node;


I think this method could be simplified now by using the parent function in this file, for example:

function isAbleToInteractWith_(node, eventName) { var matchesSelector = ['BUTTON', 'INPUT', 'SELECT', 'TEXTAREA', 'FIELDSET']; if (eventName === 'click' && matchesSelector.indexOf(currElement.tagName) > -1) { return node.disabled || parent(node, 'fieldset[disabled]'); } return true; }

It will be a bit less performant than using indexOf for the fieldset too, but it'll be much easier to understand and maintain, so I think it's worth it. @eduardolundgren will probably like it best too, he said the code looked more complex than it should.

I totally agree with you, @mairatma. I would suggest you to use this approach�.

mairatma · 2016-09-29T15:02:11Z

packages/metal-dom/test/dom.js

+			assert.strictEqual(0, listener4.callCount);
+		});
+
+		it('should not trigger dom click event to an element which its valid parent is disabled', function() {


The wording on this description sounds a bit weird in English right now, starting from the which word. It also makes it seem as if any parent would cause this, on any child, when it should only happen for form control elements with fieldset parents. Could you replace it with something more like:

should not trigger dom click event on a form control with a disabled fieldset parent

Same goes for other descriptions that are using which in a similar way to this.

mairatma · 2016-09-29T15:08:20Z

packages/metal-dom/test/dom.js

+			assert.strictEqual(0, listener4.callCount);
+		});
+
+		it('should trigger listeners of the click event for non-disabled elements even its parent being disabled', function() {


The wording here is also a bit weird, it's missing the word with after even, like:

should trigger listeners of the click event for non-disabled elements even with its parent being disabled

Don't worry about it though, we're not native English speakers, we get it wrong from time to time, but it's good to help each other out so we can improve these descriptions whenever we can 😃.

Thank you, @mairatma. And feel free to correct me as much as you can. I really care about improving my English skills. 😃

mairatma · 2016-10-04T20:13:43Z

Just started reviewing :)

Sent from GH.

fernandosouza changed the title ~~Checks if an element is able to trigger an attached delegate click event~~ Checks if an element is able to trigger an attached delegate click event - Fixes #134 Sep 20, 2016

fernandosouza commented Sep 20, 2016

View reviewed changes

fernandosouza force-pushed the disabled-on-delegate branch 2 times, most recently from 80bcb9b to b5478ce Compare September 28, 2016 04:06

mairatma reviewed Sep 28, 2016

View reviewed changes

mairatma reviewed Sep 29, 2016

View reviewed changes

Fernando Souza added 2 commits October 4, 2016 11:04

Fixes wording on test description

fef6508

Adds missed trigger for textarea

83c925f

fernandosouza force-pushed the disabled-on-delegate branch from 550cd89 to 3f65d31 Compare October 4, 2016 19:18

Fernando Souza added 3 commits October 4, 2016 16:49

Improves isAbleToInteractWith_ method to ignore right click on Firefox

59646a0

Splits handleDelegateEvent_ method logic into two new ones.

19753ed

Adds new test cases for disabled elements behaviour

a70fd40

fernandosouza force-pushed the disabled-on-delegate branch from 3f65d31 to a70fd40 Compare October 4, 2016 19:50

mairatma merged commit a70fd40 into metal:master Oct 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checks if an element is able to trigger an attached delegate click event - Fixes #134 #157

Checks if an element is able to trigger an attached delegate click event - Fixes #134 #157

fernandosouza commented Sep 20, 2016

fernandosouza Sep 20, 2016 •

edited

Loading

mairatma Sep 20, 2016

fernandosouza Sep 20, 2016

mairatma Sep 21, 2016

fernandosouza commented Sep 28, 2016

mairatma commented Sep 28, 2016

mairatma Sep 28, 2016

fernandosouza Sep 28, 2016

eduardolundgren commented Sep 29, 2016

mairatma commented Sep 29, 2016 •

edited

Loading

mairatma left a comment

mairatma Sep 29, 2016

mairatma Sep 29, 2016

fernandosouza Sep 29, 2016 •

edited

Loading

mairatma Sep 29, 2016 •

edited

Loading

mairatma Sep 29, 2016 •

edited

Loading

fernandosouza Sep 30, 2016

mairatma commented Oct 4, 2016

		@@ -265,11 +265,11 @@ export function delegate(element, eventName, selectorOrTarget, callback, opt_def
		function isAbleToInteractWith_(node, eventName) {
		var currElement = node;

Checks if an element is able to trigger an attached delegate click event - Fixes #134 #157

Checks if an element is able to trigger an attached delegate click event - Fixes #134 #157

Conversation

fernandosouza commented Sep 20, 2016

fernandosouza Sep 20, 2016 • edited Loading

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

fernandosouza commented Sep 28, 2016

mairatma commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardolundgren commented Sep 29, 2016

mairatma commented Sep 29, 2016 • edited Loading

mairatma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandosouza Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

mairatma Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

mairatma Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mairatma commented Oct 4, 2016

fernandosouza Sep 20, 2016 •

edited

Loading

mairatma commented Sep 29, 2016 •

edited

Loading

fernandosouza Sep 29, 2016 •

edited

Loading

mairatma Sep 29, 2016 •

edited

Loading

mairatma Sep 29, 2016 •

edited

Loading