Skip to content
This repository has been archived by the owner on Nov 16, 2021. It is now read-only.

Fix shadow dom handling #1

Closed
wants to merge 2 commits into from
Closed

Fix shadow dom handling #1

wants to merge 2 commits into from

Conversation

kleinfreund
Copy link

Since ccampbell#438 wasn’t merged upstream, I’m trying to get this into Discourse’s fork as per Mousetrap.js doesn’t properly stop callbacks for events originating from a shadow DOM (comment).

Prerequisites:


(Original notes)

This pull request:

  • adds tests for open and closed shadow trees.
  • adds a sixth options parameter to KeyEvent.simulate. This was required to re-target the event target with closed shadow trees. The related code contains further explanation.

Important note: The tests pass when opening tests/mousetrap.html in a browser, but not when running npm test. This is due to the test environment not understanding Element.attachShadow; hence, it is throwing on the lines with the call to attachShadow in the test file. I’m not quite sure how to make these tests work with mocha.

@eviltrout
Copy link

I would really prefer if this was merged upstream. The problem was lack of review from the project?

@kleinfreund
Copy link
Author

@eviltrout Absolutely, that would be preferable. Unfortunately, as opposed to my previous PR, there was zero feedback/response on this one. I bumped and got nothing so I decided to file it here instead.

@SamSaffron
Copy link
Member

This is so tricky... we have been sitting on, merging this into our master is not going to help cause discourse uses firefox-alt-key branch. We have been stuck on this branch for 2 years now. I am not sure we will ever get away from it.

ccampbell#402

@ccampbell appears to be growing in open issues on mousetrap and it appears to be just too hard to manage the oncoming requests here. This is the nature of open source, sometimes this happens.

My general strategy from Discourse is going to be to just move away from mousetrap altogether and have our own micro library specific to discourse.

@ccampbell
Copy link

Hi there. As you have pointed out it is not an ideal situation. The truth is I just don’t have a lot of time/resources to devote to working on Mousetrap.

That said, I’m happy to merge whatever changes are necessary into master (provided they are minor). I know from experience that it’s quite annoying to have to maintain your own fork because the upstream project won’t merge changes.

I commented on the other PR. If you decide to go with the micro library approach there are no hard feelings here.

Wishing you all a happy holidays!

@SamSaffron
Copy link
Member

SamSaffron commented Dec 24, 2018

@ccampbell wishing you a happy holiday. I totally understand your predicament. Thank you so much for mousetrap!

@kleinfreund we are going to take our time here deciding what the best path is forward, ping us in 14 days if we do not reply here or on meta, it is going to take us time key decision makers are on a break at the moment.

@kleinfreund
Copy link
Author

I’m closing this as I’m currently trying to get the original pull request on @ccampbell’s repository merged by updating the test suite. For that, I need to fork the original repository and can’t keep this. I’ll get back to you once there is any important news.

@kleinfreund kleinfreund closed this Jan 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants