Cleanup protectEditing and ignore shadow roots. Fixes #17 #18
Conversation
madeleineostoja
force-pushed
the
cleanup-protect
branch
from
a34356b to
c82ecc4
Compare
There was a problem hiding this comment.
Weirdness on checking for shadow root. Also this feature really needs to have tests written, if we break it, it can mean our users cannot edit anything. And it's complex what's going on (no thanks to my initial ultra confusing naming) so it's hard to know that new changes aren't breaking things.
| function editableElementsIn(list) { | ||
| return Array.prototype.filter.call(list, isEditableElement); | ||
| function ignoredElementsIn(elements) { | ||
| const shouldIgnore = element => isSimplaElement(element) || |
There was a problem hiding this comment.
@seaneking root here isn't declared anywhere? Also I'm a bit concerned about the perf of this. We're querying for simpla-admin every time, and then also doing a contains op. The contains I'm not sure we can get around, but can just use window.SimplaAdmin to find the simpla-admin instance.
There was a problem hiding this comment.
Whoops! Should just check for truthiness, was previously doing a strict check but realised it wasn't necessary.
There was a problem hiding this comment.
And querying for a single element is performant, we're talking ridiculously miniscule gains there. But yeah sure, cleaner to use the singleton instance anyway.
There was a problem hiding this comment.
el.contains() is perfectly performant for our needs as well, again, talking crazy small gains otherwise. Very much premature optimization.
There was a problem hiding this comment.
Yeah fair on premature optimisation, if it becomes sluggish, then we can look at them.
|
Also there is a fair chunk changing here at once, so I'm not really confident in merging unless it's been manually tested pretty thoroughly. |
|
Tested it a fair bit on simpla.io, and it's less broken than it was. Previous behavior was pretty borked on simpla.io, largely because it didn't ignore shadow roots. I'd be confident that this has fewer edge cases than mainline, and can always push further patches if things are still broken. |
|
This adds in a regression where you can't login (can't click on login button in simpla-admin), or at least in my tests that's whats happening - I'm on a converted agency bootstrap template. Might be worth making a separate commit / PR that just ignores Shadow Roots. Don't think we should merge this unless we add some proper tests |
|
Can't repro the login button issue - can you provide more deets? |
|
Yeah sure, sorry should have included steps. This is minimum repro: <!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<!-- Simpla -->
<!-- Include Simpla and cross-browser polyfill, then init project -->
<script src="https://unpkg.com/webcomponents.js@^0.7.24/webcomponents-lite.min.js"></script>
<script src="https://unpkg.com/simpla@^2.0.0"></script>
<script>
Simpla.init('random');
</script>
<link rel="import" href="/bower_components/simpla-admin/simpla-admin.html">
</head>
<body>
</body>
</html>and just For me, I can't click 'lets go' and hitting return doesn't work either. All clicks are disabled - can't click 'x' to exit either, though blurring / hitting escape work fine. |
|
Hmm okay, will try to repro later. If that is the case, with WC v1 around the corner maybe we should just leave this until we can properly observe shadow roots with the element registry. Flat out ignoring shadow roots is a little hokey anyway. |
|
Okay issue is that Easy fix would be to use similar hack as master has now, or do some tree walking with |
madeleineostoja
force-pushed
the
master
branch
2 times, most recently
from
97e5e80 to
587b55e
Compare
No description provided.