Feature Capture Event #91

vipulbhj · 2020-09-05T14:14:51Z

First draft implementation for #33

s-yadav · 2020-09-05T14:21:36Z

src/updateAttribute.js

-      node.addEventListener(eventName, patchedHandler);
+      let isCaptureEvent = eventName.endWith('Capture');
+      if(isCaptureEvent) {
+        let cleanedEventName = eventName.replace('Capture', '');


As we are passing attrName for mapping patched handler. We can do this logic inside getEventName only.

Yes, making the update

s-yadav · 2020-09-05T14:23:13Z

src/updateAttribute.js

+      if(isCaptureEvent) {
+        let cleanedEventName = eventName.replace('Capture', '');
+        node.addEventListener(cleanedEventName, patchedHandler);
+      } else {


No need to have if/else, we can just do.

node.addEventListener(cleanedEventName, patchedHandler, isCaptureEvent);

If it is false it will assign it in bubble phase.

Yes that indeed is cleaner 👍
Will update this

vipulbhj · 2020-09-05T14:50:08Z

src/utils.js

@@ -21,7 +21,9 @@ export function getNodeName(node) {
 }

 export function getEventName(attrName) {
-  return attrName.replace('on', '').toLowerCase();
+  let eventName = attrName.replace('on', '').toLowerCase();
+  if(eventName.endsWith('capture')) return eventName.replace('capture', '');


I could add the capture replace in the same code itself, but it would be better if we only remove it my removing capture from the end via substring

Just not sure, if thats reasonable for some event to have capture in its name

Actually there are two, gotpointercapture and lostpointercapture.
https://developer.mozilla.org/en-US/docs/Web/Events

I don't see preact handling it. https://github.com/preactjs/preact/blob/master/src/diff/props.js#L92

Have to check react if they handle it differently

Also you can replace on and capture together using regex.

.replace(/^on(.*?)(Capture$|$)/, '$1')

s-yadav · 2020-09-05T15:23:36Z

src/updateAttribute.js

@@ -56,7 +56,8 @@ function setAttribute(node, attrName, attrValue, oldAttrValue, isSvgAttribute) {

      // if the event is getting added first time add a listener
    } else if (!oldAttrValue && attrValue) {
-      node.addEventListener(eventName, patchedHandler);
+      let isCaptureEvent = attrName.endWith('Capture');


Define it outside, so you can use it to remove the event listener as well.

s-yadav · 2020-09-05T16:41:46Z

src/utils.js

@@ -21,7 +21,9 @@ export function getNodeName(node) {
 }

 export function getEventName(attrName) {
-  return attrName.replace('on', '').toLowerCase();
+  let eventName = attrName.replace('on', '').toLowerCase();
+  if(eventName.endsWith('capture')) return eventName.replace('capture', '');


Actually there are two, gotpointercapture and lostpointercapture.
https://developer.mozilla.org/en-US/docs/Web/Events

I don't see preact handling it. https://github.com/preactjs/preact/blob/master/src/diff/props.js#L92

Have to check react if they handle it differently

s-yadav · 2020-09-05T16:44:48Z

src/utils.js

@@ -21,7 +21,9 @@ export function getNodeName(node) {
 }

 export function getEventName(attrName) {
-  return attrName.replace('on', '').toLowerCase();
+  let eventName = attrName.replace('on', '').toLowerCase();
+  if(eventName.endsWith('capture')) return eventName.replace('capture', '');


Also you can replace on and capture together using regex.

.replace(/^on(.*?)(Capture$|$)/, '$1')

s-yadav · 2020-09-06T17:22:50Z

src/updateAttribute.js

@@ -49,14 +49,15 @@ function setAttribute(node, attrName, attrValue, oldAttrValue, isSvgAttribute) {

    // get patched event handler
    const patchedHandler = getPatchedEventHandler(node, attrName, attrValue);
+
+    const isCaptureEvent = attrName.endWith('Capture');


Suggested change

const isCaptureEvent = attrName.endWith('Capture');

const isCaptureEvent = attrName.endsWith('Capture');

We will also need to add support for onGotPointerCapture, and onLostPointerCapture as well. React does handle it.
Also, we can have a capture phase on these events, so this becomes more tricky. You can try the below logic in browser.

const para = document.querySelector('p'); para.addEventListener('gotpointercapture', () => { console.log('I\'ve been captured!') }, false); para.addEventListener('pointerdown', (event) => { para.setPointerCapture(event.pointerId); }); document.body.addEventListener('gotpointercapture', () => { console.log('budy I\'ve been captured!') }, false); document.body.addEventListener('pointerdown', (event) => { para.setPointerCapture(event.pointerId); });

So ideally even this onGotPointerCaptureCapture and onLostPointerCaptureCapture is valid event.

We will have to add Edge case for this two events.

Looks like this recently got fixed in React as well.
https://github.com/facebook/react/blob/master/packages/react-dom/src/events/DOMPluginEventSystem.js#L498

Should we handle it the way react does ?
We can also check for these specific events and then do the special logic.

We can handle it in the same way.
https://www.measurethat.net/Benchmarks/Show/9484/0/substr-vs-endswith

We will also have to take care of getEventName method for this

s-yadav · 2020-09-08T20:27:14Z

src/utils.js

@@ -21,6 +21,11 @@ export function getNodeName(node) {
 }

 export function getEventName(attrName) {
+  const isSpecialCaptureEvent = attrName.substr(-7) === 'Capture' && attrName.substr(-14, 7) !== 'Pointer';


This logic we should put on updateAttribute method to find isCaptureEvent event. This case handles both the normal capture case and special case. So it's not just the isSpecialCaptureEvent

And we can pass isCaptureEvent as param in this function.

s-yadav · 2020-09-08T20:28:42Z

src/updateAttribute.js

@@ -50,7 +50,7 @@ function setAttribute(node, attrName, attrValue, oldAttrValue, isSvgAttribute) {
    // get patched event handler
    const patchedHandler = getPatchedEventHandler(node, attrName, attrValue);

-    const isCaptureEvent = attrName.endWith('Capture');
+    const isCaptureEvent = attrName.substr(-7) === "Capture";


This will break for the special case. We can use the logic below here and pass isCaptureEvent as param to getEventName, just check if getEventName is getting called from anywhere else.

why will this break ?

s-yadav · 2020-09-09T06:35:51Z

src/utils.js

+  if(isSpecialCaptureEvent) {
+    // Capture -> 7 , on -> 2 ;  7 + 2 = 9;
+    return attrName.replace('on', '').substr(0, (attrName.length - 9)).toLowerCase();
+  }
+  return attrName.replace(/^on(.*?)(Capture$|$)/, '$1').toLowerCase();


if (isCapture) attrName = attrName.replace(/Capture$/, ''); return attrName.replace('on', '').toLowerCase();

added suggested improvements capture added to removeEventListener and getEventName Made the appropriate changes Review Changes done

s-yadav reviewed Sep 5, 2020

View reviewed changes

vipulbhj commented Sep 5, 2020

View reviewed changes

s-yadav reviewed Sep 5, 2020

View reviewed changes

s-yadav reviewed Sep 6, 2020

View reviewed changes

s-yadav requested changes Sep 8, 2020

View reviewed changes

s-yadav reviewed Sep 9, 2020

View reviewed changes

Feature Capture Event

69185cc

added suggested improvements capture added to removeEventListener and getEventName Made the appropriate changes Review Changes done

vipulbhj force-pushed the issue#33 branch from d0a2451 to 69185cc Compare September 9, 2020 06:48

vipulbhj requested a review from s-yadav September 9, 2020 06:52

s-yadav approved these changes Sep 9, 2020

View reviewed changes

Added reference to React Source

5f0bf24

s-yadav merged commit ffc3710 into brahmosjs:master Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature Capture Event #91

Feature Capture Event #91

vipulbhj commented Sep 5, 2020

s-yadav Sep 5, 2020

vipulbhj Sep 5, 2020

s-yadav Sep 5, 2020

vipulbhj Sep 5, 2020

vipulbhj Sep 5, 2020

s-yadav Sep 5, 2020

s-yadav Sep 5, 2020

s-yadav Sep 5, 2020 •

edited

Loading

s-yadav Sep 5, 2020

s-yadav Sep 5, 2020

s-yadav Sep 6, 2020

s-yadav Sep 6, 2020

s-yadav Sep 6, 2020

vipulbhj Sep 7, 2020

s-yadav Sep 7, 2020

s-yadav Sep 8, 2020

s-yadav Sep 8, 2020

vipulbhj Sep 9, 2020

s-yadav Sep 9, 2020 •

edited

Loading

	const isCaptureEvent = attrName.endWith('Capture');
	const isCaptureEvent = attrName.endsWith('Capture');

Feature Capture Event #91

Feature Capture Event #91

Conversation

vipulbhj commented Sep 5, 2020

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

s-yadav Sep 5, 2020 • 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

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

s-yadav Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

s-yadav Sep 5, 2020 •

edited

Loading

s-yadav Sep 9, 2020 •

edited

Loading