Adds Event Handler Options #847

samends · 2020-09-25T15:49:58Z

Type: Feature

The following has been addressed in the PR:

There is a related issue
All code has been formatted with prettier as per the readme code style guidelines
Unit or Functional tests are included in the PR

Adds ability to use event handler options as described here: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener. The syntax would be the following:

const scrollElementDiv = v('div', { onscroll: {
	passive: true,
	handler: (event: UIEvent) => {
		// handle onscroll event here
	}
} });

As describe in the open issue, this would allow events on elements to be label as passive, preventing the warning on chrome. This will also allow a developer to use the options capture and once.

Resolves #810

codesandbox-ci · 2020-09-25T15:51:44Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 478ddcb:

Sandbox	Source
dojo/dojo-codesandbox-template	Configuration

codecov · 2020-09-25T15:56:47Z

Codecov Report

Merging #847 into master will decrease coverage by 2.82%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #847      +/-   ##
==========================================
- Coverage   97.64%   94.81%   -2.83%     
==========================================
  Files         127      127              
  Lines        8034     8049      +15     
  Branches     1861     1869       +8     
==========================================
- Hits         7845     7632     -213     
- Misses        189      417     +228

Impacted Files	Coverage Δ
src/testing/renderer.ts	`97.66% <ø> (+0.01%)`	⬆️
src/testing/decorate.ts	`96.03% <80.00%> (-1.97%)`	⬇️
src/core/vdom.ts	`97.92% <100.00%> (-0.14%)`	⬇️
src/core/registerCustomElement.ts	`6.53% <0.00%> (-91.03%)`	⬇️
src/core/mixins/Themed.ts	`99.00% <0.00%> (-1.00%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7cf6b9...478ddcb. Read the comment docs.

matt-gadd · 2020-09-26T10:35:05Z

@samends I don't think the current implementation correctly works with once because of the way event handlers are re-attached. for example try this:

import { renderer, create, tsx, invalidator } from "@dojo/framework/core/vdom";

const factory = create({ invalidator });

const App = factory(function App({ middleware: { invalidator } }) {
  return (
    <div>
      <button
        onclick={{
          once: true,
          handler: () => {
            console.log("hello world");
            invalidator();
          }
        }}
      >
        Click
      </button>
    </div>
  );
});

const r = renderer(() => <App />);
r.mount();

matt-gadd · 2020-09-26T10:42:26Z

tests/testing/unit/harness/harness.tsx

@@ -301,7 +301,11 @@ describe('harness', () => {
 			const h = harness(() => w(MyWidget, {}));
 			h.trigger('*[key="span"]', (node: WNode | VNode) => {
 				if (isVNode(node)) {
-					return node.properties.onclick;
+					const onclick = node.properties.onclick;


How come we had to change this here?

This now needs to check if the event is an object with handler or an function

this is a test though isn't it, and you haven't changed the test? so why has the vnode changed?

I think I now that this is a type guard, and you can't possibly ever get to the handler code - so I think this would be better as just a type cast instead

Hmm so the more I think about this I think the more problematic this type change is for users downstream. Given how this is not the common use case, have we considered not changing VNodeProperties at all, and instead creating a utility function that basically creates a function with properties decorated on it (thus adhering to the existing interface?). so something like:

<button onclick={ eventWithOptions({ capture: true }, () => console.log('onclick')) }>Button</button>

or perhaps even nicer if we just want to solve the passive case for now like the issue raised:

<button onscroll={ passive(() => console.log('onscroll')) } >Button</button>

Hmm I haven't considered something like that but it would make the typing alot easier

Let me look into this

For this to work you would need to change the types so that the onscroll function could optionally take a function with any parameters

Going back to this problem what your suggesting would still require a change to the event types so we'd still need to check to see if there's a handler here. What could work instead is if we optionally took two values for events?

@samends I'm still not sure what the problem with a wrapper function is with regards to types? It looks like you could do something like this without requiring any changes to types (as it works for v7), and then hook into the property on the handler in the vdom to deal with it?

https://codesandbox.io/s/wonderful-sound-kijth?file=/src/main.tsx

matt-gadd · 2020-09-27T12:59:27Z

Does this change work with the test renderer?

agubler · 2020-09-29T09:08:20Z

src/core/vdom.ts

@@ -1609,7 +1648,14 @@ export function renderer(renderer: () => RenderResult): Renderer {
 					setValue(domNode, propValue, previousValue);
 				} else if (propName !== 'key' && propValue !== previousValue) {
 					const type = typeof propValue;
-					if (type === 'function' && propName.lastIndexOf('on', 0) === 0 && includesEventsAndAttributes) {
+					if (type === 'object' && propValue && 'handler' in propValue) {


Would this also need the includesEventsAndAttributes to ensure that events don't get registered twice? Another thing would be that we probably need to add support for this new style of event handlers when using dom() function (https://github.com/dojo/framework/blob/master/src/core/interfaces.d.ts#L171)

When support has been added for that then it might make sense to put all the logic handling either a function or object with a handler inside the updateEvent function.

samends · 2020-09-29T14:38:40Z

Hmm @matt-gadd maybe we shouldn't offer once as an option then

matt-gadd · 2020-09-29T15:42:26Z

src/core/vdom.ts

@@ -1296,7 +1335,7 @@ export function renderer(renderer: () => RenderResult): Renderer {
 			};
 		}

-		domNode.addEventListener(eventName, callback);
+		domNode.addEventListener(eventName, callback, options);


I think you will have to do a check here to support IE11, as it doesn't take an options bag so passing anything truthy to it will mean it does a capture.

So IE doesn't seem to have an equivalent here, could we still implement this with a note saying IE doesn't support event options?

Yeah none of the options bag is implemented in IE other than capture and it’s a Boolean as the third parameter I believe.

Yup that’s correct, it doesn’t error passing an object bag but it does consider it “truthy” so capture is enabled. We’ll need to sniff IE11 and and not pass the options bag at all I think. That can be documented that the “passive” helper isn’t supported in IE11

matt-gadd · 2020-11-18T14:25:42Z

Closing as resolved in #856

samends added 4 commits September 4, 2020 12:22

Started passive event listeners

dd93d61

Updated test to pass event options

71e6073

Updated event types for all events

683916e

Updated types on interface for event options

c4ee80a

samends changed the title ~~Passive event listeners~~ Adds Event Handler Options Sep 25, 2020

Removed unused interface

478ddcb

matt-gadd reviewed Sep 26, 2020

View reviewed changes

agubler reviewed Sep 29, 2020

View reviewed changes

matt-gadd reviewed Sep 29, 2020

View reviewed changes

matt-gadd closed this Nov 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Event Handler Options #847

Adds Event Handler Options #847

samends commented Sep 25, 2020

codesandbox-ci bot commented Sep 25, 2020 •

edited

codecov bot commented Sep 25, 2020 •

edited

matt-gadd commented Sep 26, 2020

matt-gadd Sep 26, 2020

samends Sep 29, 2020

matt-gadd Sep 29, 2020

matt-gadd Sep 29, 2020

matt-gadd Sep 29, 2020

samends Sep 29, 2020

samends Sep 29, 2020

samends Oct 1, 2020 •

edited

samends Oct 16, 2020

agubler Oct 19, 2020

matt-gadd commented Sep 27, 2020

agubler Sep 29, 2020

samends commented Sep 29, 2020

matt-gadd Sep 29, 2020

samends Sep 30, 2020

matt-gadd Sep 30, 2020

agubler Sep 30, 2020

matt-gadd commented Nov 18, 2020

Adds Event Handler Options #847

Adds Event Handler Options #847

Conversation

samends commented Sep 25, 2020

codesandbox-ci bot commented Sep 25, 2020 • edited

codecov bot commented Sep 25, 2020 • edited

Codecov Report

matt-gadd commented Sep 26, 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

samends Oct 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-gadd commented Sep 27, 2020

Choose a reason for hiding this comment

samends commented Sep 29, 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

matt-gadd commented Nov 18, 2020

codesandbox-ci bot commented Sep 25, 2020 •

edited

codecov bot commented Sep 25, 2020 •

edited

samends Oct 1, 2020 •

edited