search

guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Fix/329 chrome shadow dom errors #338

Closed shaunnetherby closed 9 years ago

shaunnetherby commented 9 years ago

This is to fix issue #329. Scribe does not work in shadow DOM because it grabs the selection from the document rather then the parent document fragment (shadow root in this case). I updated Selection to grab the appropriate document fragment to use to get the selection.

There is also 2x chrome bugs encountered when scribing in shadow root. The first causes the selection.getRangeAt() method to return an incorrect range. I checked Chrome Canary and found that the getRangeAt() issue is fixed in that version but I applied the workaround so scribe can be used for now (https://code.google.com/p/chromium/issues/detail?id=380690).

The second bug is not fixed in Canary and it causes selection.isCollapsed to be incorrect. I found a workaround by calling range.collapsed (https://code.google.com/p/chromium/issues/detail?id=447523)

I tried to create some tests for these cases but was having trouble. I think the scribe-test-harness will have to be modified to support testing scribe in a shadow root, and I'm not just not sure how to appropriately do that. All of the current tests did pass in FF/Chrome.

rrees commented 9 years ago

@shaunnetherby As long as the tests pass I'd be happy to merge this, could you add the issues you're working around to BROWSERINCONSISTENCIES.md as well so we remember to review them later.

rrees commented 9 years ago

Second opinion @cutandpastey ?

cutandpastey commented 9 years ago

Looks good. Tests pass in both Chrome and FF here. I don't have a lot of experience with shadow DOM so maybe @theefer should cast a glance. :+1: from me tho.

theefer commented 9 years ago

The code seems sane but I don't really have enough context from working on Scribe recently to know what this is really doing, so would be good to confirm with someone QA'ing it and also even some tests if possible?

@shaunnetherby Are you building a Scribe Web Component? Is it available to see anywhere? Would be really interested to see how well it works and the kind of HTML interface it exposes!

cutandpastey commented 9 years ago

I think there are a few people doing this? https://github.com/guardian/scribe/issues/329

shaunnetherby commented 9 years ago

@theefer Good catch on using the Node constants, I updated the codes accordingly. I think tests would be great but I'm not sure how to introduce shadow dom testing into the current test framework. I played around with it a bit but I think I would need to do some serious tweaking in the scribe-test-harness helpers.js file, and I'm a little hesitant to play with that file since it affects so many of the tests. Ideally there would be a way that this could be tweaked such that all of the tests could be run against a light dom scribe.el and a shadow dom scribe.el to root out any other shadow inconsistencies.

@rrees I updated the BROWSERINCONSISTENCIES.md in a way that I think makes sense, so take a look at that.

I'll try to post a link here by this afternoon so you can check out how I'm using scribe (need to get some stuff in order first).

shaunnetherby commented 9 years ago

I added a missing null check for currentElement (was causing issues in some of my tests).

@theefer Here is a link to the component I am working on, should be able to start making more progress now that scribe is behaving :). http://infusionsoft.github.io/inf-rich-text-editor/bower_components/inf-rich-text-editor/demo.html

rrees commented 9 years ago

@shaunnetherby Awesome work I (or someone else) will merge and release as soon as we can. Good luck with the Web Component!

rrees commented 9 years ago

@shaunnetherby Scribe 1.2.7