Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(event-manager): shadow dom event bubbling #758

Merged

Conversation

@michaelw85
Copy link
Contributor

commented Jul 16, 2019

Delegate events should be able to pass shadow dom boundry similar to native
events.

Closes #755

@CLAassistant

This comment has been minimized.

Copy link

commented Jul 16, 2019

CLA assistant check
All committers have signed the CLA.

fix(event-manager): shadow dom event bubbling
Delegate events should be able to pass shadow dom boundry similar to native
events.

Closes #755

@michaelw85 michaelw85 force-pushed the michaelw85:755_fix_shadow_dom_event_bubbling branch from 8d25fac to 7563566 Jul 16, 2019

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@bigopon Here's a small PR to fix the shadow dom boundry event bubbling issue

src/event-manager.js Outdated Show resolved Hide resolved
src/event-manager.js Outdated Show resolved Hide resolved
@EisenbergEffect

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

We definitely need this in some form. I had to implement some custom stuff for my own vNext app because this doesn't work quite right with link clicks and routing, for example. So, I think we need a fix in the core and also some fix in the router....and we need to port it all to vNext. Two thoughts:

  • For vCurrent, I do think we should have a setting to turn this on, just to be safe. For vNext, it should just work this way by default.
  • Is there any other way to detect the shadow root other than toString? I thought there was an API for that.

/cc @fkleuver for vNext core and @jwx for vNext router.

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@EisenbergEffect Where would such a flag be implemented? Would this be something global?

On the alternatives, lets brainstorm:

  1. I could use instanceof ShadowRoot
  2. I could check element.shadowRoot but this fails for closed mode.
  3. I could use getRootNode() to get the shadow root but not sure how to determine it's not returning the root dom I guess I would still end up using toString or instanceof. Another drawback I doubt this works cross browser.
  4. I could attempt to find the element in the light dom, if it's not in light it's in the shadow dom. (does not sound performant and wonder if it might end up giving false results)
  5. Maybe it would be possible to match host with the dom root if it doesn't match we have successfully detected a shadow root host and should continue up from there.
  6. ....

I think instanceof might be the only feasible and more elegant option .

@EisenbergEffect

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

It could be implemented as a boolean property on the EventManager class itself. I guess doing an instanceof check is the best, though we would need to guard against that not existing, in the case someone is using a browser without native support.

@bigopon

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

I think the combination of .getRootNode() and .ownerDocument would be sufficient. Though this increases the amount of DOM APIs we use in core framework, not that it matters for vCurrent since we only have 1 runtime for web. But it could be worth it to be aware of.

cc @fkleuver for awareness

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

.getRootNode() has been around since the beginning of shadow dom, so wherever shadow dom is safe to use, that piece of the api is safe to use as well. DOM api surface used in and of itself is really not an issue, as long as no globals are used

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

I've changed the string check to an instanceof check. This seems like the easiest most readable change to me.

I need some assistance for the boolean/setting.
How would the handleDelegatedEvent function get access to this bool property?
Should it be on the proto chain of the EventManager class? Or would this bool have to be passed through multiple classes/functions to end up in the handleDelegatedEvent function as a param? Is there a singleton I could get and should this prop just be a public property I can access?

How would a dev set this bool?
If the EventManager is a singleton you could gain access using dep injection and than set the value of the bool. If its a public prop or on the proto chain this would be very fragile. Any piece of code (or plugin for that matter) could affect this value causing unexpected behavior.

I'm a bit confused how to properly pass the setting from an app all the way down to the code handling the event bubbling.

@bigopon

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

I think instanceof check combined with a existance check is pretty ok. About the bool check, let me have a look tonight

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@bigopon Did you have time to have a look at how I could introduce a flag? It would be awesome if I could wrap up this PR.

src/event-manager.js Outdated Show resolved Hide resolved
@bigopon

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

We have

class DelegateHandlerEntry {
  constructor(eventName) {
    this.eventName = eventName;
    this.count = 0;
  }

  increment() {
    this.count++;

    if (this.count === 1) {
      DOM.addEventListener(this.eventName, handleDelegatedEvent, false);
    }
  }

  decrement() {
    if (this.count === 0) {
      emLogger.warn('The same EventListener was disposed multiple times.');
    } else if (--this.count === 0) {
      DOM.removeEventListener(this.eventName, handleDelegatedEvent, false);
    }
  }
}

We can change the constructor to accept 1 more parameter, that it will determine if it should check for shadow root, and then we can change handler from a loose function, to event listener object:

class DelegateHandlerEntry {
  constructor(eventName, escapeShadowRoot) {
    this.eventName = eventName;
    this.count = 0;
    this.escapeShadowRoot = escapeShadowRoot;
  }

  handleEvent(event) {
    // this will be this instance
    // so we can get the escapeShadowRoot value
  }

  increment() {
    this.count++;

    if (this.count === 1) {
      // use this as handler, instead
      DOM.addEventListener(this.eventName, this, false);
    }
  }

  decrement() {
    if (this.count === 0) {
      emLogger.warn('The same EventListener was disposed multiple times.');
    } else if (--this.count === 0) {
      // use this to remove handler instead
      DOM.removeEventListener(this.eventName, this, false);
    }
  }
}

Though will need some more tests for these changes. Are you comfortable with writing tests for those changes?

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@bigopon Thanks for the input but I'm still a bit confused how an Aurelia app would pass this bool to the constructor. How does the consuming app set this to true?

@bigopon

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@michaelw85 During start up, one would do this:

aurelia.container.get(EventManager).escapeShadowRoot = true;

then we change the code here:

this.defaultEventStrategy = new DefaultEventStrategy();

to

    this.defaultEventStrategy = new DefaultEventStrategy(this);

So later, we could change here

handlerEntry = delegatedHandlers[targetEvent] || (delegatedHandlers[targetEvent] = new DelegateHandlerEntry(targetEvent));
and here
handlerEntry = capturedHandlers[targetEvent] || (capturedHandlers[targetEvent] = new CapturedHandlerEntry(targetEvent));
to something like this:

-      handlerEntry = capturedHandlers[targetEvent] || (capturedHandlers[targetEvent] = new CapturedHandlerEntry(targetEvent));
+      handlerEntry = capturedHandlers[targetEvent] || (capturedHandlers[targetEvent] = new CapturedHandlerEntry(targetEvent, this.eventManager.escapeShadowRoot));
@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

ok thanks I could give this a try.
Not a fan of getting the instance and changing the prop tbh, this could be done from anywhere by any code causing unexpected side effects/breaking the code. What if for instance a plugin would do this?

@bigopon

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@michaelw85 after aurelia.start() call, all plugins have been run, you can set it there. About your Q, there's always possibility of something messing with this, which I don't see anything we can do about, but I'm not sure anyone would do that unless for fun. Beside that, as you said, it matches native behavior, pretty ok to me

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Only option I could think of as an alternative is to only read the option once at startup from the aurelia config. This would prevent any changes during run time.

@bigopon

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

@michaelw85 we can do it that way by having the setter ignore 2nd and after assignment.

Cc @EisenbergEffect @fkleuver for making a call with us

@EisenbergEffect

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

I'm pretty ok with keeping it simple and letting people do this:

aurelia.container.get(EventManager).escapeShadowRoot = true;

While there is technically a possibility for things to stomp on each other, it seems unlikely.

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@bigopon Sorry it took a while I had to finish some other work first but here it is.
Implemented the flag, struggled a bit with unit tests but got it up and running after disposing the registered events after each test 🌈

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

@EisenbergEffect This PR is still waiting for review, could someone be assigned?

@bigopon

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Oops, seems like its my turn to slack. Ill review it tonight. Sorry for that. First glance is it needs some modification

src/event-manager.js Outdated Show resolved Hide resolved
michaelw85 added 2 commits Aug 30, 2019
src/event-manager.js Outdated Show resolved Hide resolved
src/event-manager.js Outdated Show resolved Hide resolved
@bigopon

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@michaelw85 nice 👍 . Can you help give CapturedHandlerEntry the same treatment like DelegatedHandlerEntry? Also please remove the dist files

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@michaelw85 nice 👍 . Can you help give CapturedHandlerEntry the same treatment like DelegatedHandlerEntry? Also please remove the dist files

Changing CapturedHandlerEntry seems unrelated to this PR. I would not mind changing it but I would rather create a new PR for that with a proper description why it is changed.

The dist files are not ignored at the moment should they be? Should I revert the changed of the dist in this PR or do you really mean delete? But I guess same question applies here is this related to this PR else I would prefer keeping it out of this one.

@bigopon

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Well, i just realized the title of this PR, so I think your argument is fair, and in agile spirit. We probably can go ahead with this. Just need to make @EisenbergEffect @fkleuver aware of this.
For dist files, we have been going with that for a long time, I'm not sure why.

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Ok great lets finish this PR. If you create tasks/placeholders for the other changes just mention me and I will create PR's.

@EisenbergEffect

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

The dist files are needed for Bower, since it uses the repo itself as the installation source. We'll be dropping this for Aurelia 2, but it's not something we can drop for the current version, due to people being dependent on it. It could be solved with an imporved CI process and different branch strategy, but we've had it this way so long, and it's only a minor inconvenience, so we haven't allotted the time to make the change.

@michaelw85 If you can remove the dist folder from this PR, that would be great.
@bigopon Are we tracking this for Aurelia 2 as well? We will need to make the same improvements or we're going to hit these problems again right out of the gate.

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@EisenbergEffect @bigopon I've reverted the dist changes as requested.

@bigopon

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@EisenbergEffect

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Thanks @michaelw85 and @bigopon

Doing a quick review now. This is technically a breaking change, but I somehow doubt that it's going to affect anyone.

Does anyone here have concerns? Do we need to put this behind a flag?

@EisenbergEffect

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Doh. I see we did put a flag there. Just ignore me. Doing final review now...turning on my thinking cap.

@EisenbergEffect EisenbergEffect merged commit ae58f99 into aurelia:master Sep 5, 2019

4 checks passed

WIP Ready for review
Details
ci/circleci: build_test Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
main Workflow: main
Details
michaelw85 added a commit to michaelw85/binding that referenced this pull request Sep 5, 2019
fix(event-manager): update d.ts
Add escapeShadowRoot to d.ts

Related: aurelia#758 aurelia#755
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.