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

Make Shadow DOM optional for components with slots #654

Closed
rluba opened this issue Oct 3, 2019 · 20 comments
Closed

Make Shadow DOM optional for components with slots #654

rluba opened this issue Oct 3, 2019 · 20 comments
Labels
Projects
Milestone

Comments

@rluba
Copy link
Contributor

rluba commented Oct 3, 2019

🙋 Feature Request

I’ve tried to convert one of our internal apps to vNext in the past few days. One thing that causes problems for us is that components with <slot>s now use the Shadow DOM, which…

  1. prevents our style sheet from affecting the component.
  2. forces us to include another polyfill (for IE11 and MS Edge), even though we gain no benefit from the Shadow DOM.

🤔 Expected Behavior

I’d be really great if the Shadow DOM was Opt-In. Especially because it has other side-effects like #653. Slots worked great in Aurelia v1 without the Shadow DOM (although I expect this was a lot of work…)

🔦 Context

The <slot> of Aurelia v1 was a perfect equivalent of AngularJS’s <transclude>. They filled an important role without causing all the trouble that the Shadow DOM does – in a homogenous environment.

I do understand that the Shadow DOM might be useful for heterogenous environments (i.e. an Aurelia component inside a CMS). But for "Aurelia owns the whole page" scenarios, its benefits are dwarfed by its side-effects.

@3cp
Copy link
Member

3cp commented Oct 3, 2019

This is a very good argument to bring back non-shadowdom slot implementation from vcurrent.

@fkleuver
Copy link
Member

fkleuver commented Oct 3, 2019

prevents our style sheet from affecting the component.

With shadydom polyfill you can force the polyfill to override native behavior, so you'll keep your styles.

forces us to include another polyfill (for IE11 and MS Edge), even though we gain no benefit from the Shadow DOM.

v1 slot emulation is quite a lot of code, so v1 is as much a polyfill in that sense as any other polyfill. Shadydom is just a separate install so it doesn't "feel" like "installing aurelia"

This is a very good argument to bring back non-shadowdom slot implementation from vcurrent.

Let me give you some arguments against it:

  • The costs of v1 slot emulation are immense for maintainability and it wasn't even spec compliant.
  • Other frameworks who provide slot emulation have numerous longstanding open issues because of this as well. Many of which never get addressed.
  • @EisenbergEffect did slot emulation for v1 as well as he could given the time constraints. V2 is meant to be much more robust than v1, and he was already pretty clear on having no intent to redo it. So that means I'm going to have to do it, and do it better than @EisenbergEffect did. Well, good luck to me on that with my more limited understanding of ShadowDOM (compared to him) as well as my current todo list. Shall we drop AOT then, or configured router API, or the still necessary last runtime refactor? I don't think so. I'm sure I can deliver much more value to the framework by working on other things that have no pre-existing solutions (shadydom) out there.

Installing the polyfill is something we could make a bit easier, perhaps with a @aurelia/plugin-slot-emulation package that drags in shadydom and enables its override mode.

Seems like a relatively small price to pay (an extra dependency in your tree) to save an enormous expense (redoing slot emulation).

Besides, in most cases you can use replaceable instead of slots to do the same thing, and probably should.
This is one of the reasons we tried to make replaceable a bit more ergonomic for common scenarios.

Given all of the above, I firmly vote against, but I welcome counter-arguments.

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Oct 3, 2019

I think there's one thing we could do. We've discussed supporting emulation of default slot content only as something that might be not too much work. When you get into named slots and various types of slot to slot project, it get's very complicated.

So, we could do something like this:

  1. Components with no useShadowDOM decorator and no slots, don't use shadow dom. Their view just get added as children of the host element. (Done)
  2. Components with no useShadowDOM decorator and a single default slot don't use shadow DOM. The content from the usage site gets hoisted directly into the location of the slot inside the element's view (while maintaining its binding context to the content site origin). If the slot is inside another custom element, we need to keep projecting the content down until it bottoms out. This is a complexity but might be doable. (Not Done)
  3. Components with no useShadowDOM decorator and named slots throw an error. (Not Done)
  4. Components with useShadowDOM always use shadow dom. (Done)
  5. We configure the default projects with shady dom and shady styles polyfills so that they can use shadow dom as if natively supported. We've got no external dependencies in Aurelia 2 at present. I think adding one that is a polyfill for a web standard is ok, esp. since it would take months and months of work to replicate it for no other benefit than it would be ours to own (and maintain..yikes!) (Not Done)

But I think with the above and the polyfill, we can match v1 while also providing a way forward for those of us that want to just use shadow dom. Even if we can't achieve item 2, the pollyfill will handle that for us as well. So, we can save item 2 for later if we work out the rest (which should be relatively trivial).

Of course, this needs to be documented. I've got a placeholder article dedicated to styling components that is intended to go over most of this.

@EisenbergEffect EisenbergEffect added this to the Alpha milestone Oct 3, 2019
@rluba
Copy link
Contributor Author

rluba commented Oct 3, 2019

Thank you for sharing your reasons and considerations. I fully agree that bringing back v1-behavior might not be a good trade-off.

Besides, in most cases you can use replaceable instead of slots to do the same thing, and probably should.

There are use-cases where this is not equivalent. (Apart from creating more boilerplate and adding complexity to every usage site due to the scope-change inside the template element. I’ve worked around the Shadow DOM in v2 for now by converting the <slot>s to <template replaceable>s and it wasn’t pretty. The added boilerplate made some components much less useful because it turned a concise component that saves markup into an mess that’s sometimes worse than not extracting the component at all.)

I think adding @useShadowDOM and implementing items 1, 3, and 4 sounds really easy and would already help to avoid unexpected behavior.
Item 2 would be really great, but I understand that it’s more involved. (But it would solve all of our needs – unless I forgot something 95% of our needs.)

I’m not so sure about item 5. If Aurelia has no external dependencies yet, I wouldn’t break that pattern for this feature. I think having @useShadowDOM (and the error if it’s missing) already offers a good place to educate people about what to do if they want to use it. (And what possible side-effects it may have.)

@fkleuver
Copy link
Member

fkleuver commented Oct 3, 2019

I’ve worked around the Shadow DOM in v2 for now by converting the <slot>s to <template replaceable>s and it wasn’t pretty.

I understand. It's 14 characters vs 34 characters, which is a huge difference in small pieces of template.

<template replaceable></template>
<slot></slot>

What @EisenbergEffect 's plans seem to boil down to is making slots like a shortcut for replaceable, and without the context targeting.
Why don't we then just add an au-slot as a shortcut for it instead? Then we don't need to tamper with html's default behavior either. You could even combine the two if for whatever reason you needed to.

@rluba
Copy link
Contributor Author

rluba commented Oct 3, 2019

@fkleuver You misunderstood me. I meant what it does to the usage site not the component’s template.
I meant:

<tag>Innercontent with ${interpolation}</tag>

vs.

<tag context.bind="interpolation"><template replace-part="content">Innercontent with ${context}</tag>

(And it gets worse if the context parameter requires multiple unrelated properties)

@fkleuver
Copy link
Member

fkleuver commented Oct 3, 2019

@rluba We can easily add a nice shortcut for that though. For example, placing replace-part on an element would turn its content into the template for the inner (unnamed) replaceable.

So custom element template:

<div replaceable></div>

With custom element declaration:

<tag replace-part>Innercontent with ${interpolation}</tag>

Would yield:

<tag>Innercontent with ${interpolation}</tag>

@fkleuver
Copy link
Member

fkleuver commented Oct 3, 2019

Random thought. <au></au> as a shortcut for <template></template>? @EisenbergEffect

@rluba
Copy link
Contributor Author

rluba commented Oct 3, 2019

<tag replace-part> would be an improvement, but you’d still need to pass the context.

@fkleuver
Copy link
Member

fkleuver commented Oct 3, 2019

Yeah that's the only functional difference between replaceable and slots in that sense. You get the context targeting. We could make that configurable though via an alias I suppose. Then we'll more or less have v1-like slot emulation.
It just doesn't seem sensible to me to write new code for that when we already have the replaceable infrastructure to do almost the same thing.

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Oct 3, 2019

@fkleuver Slots and replacelables are not quite the same because of this:

component-one.html

<div>
  <slot></slot>
</div>

component-two.html

<component-one>
  <slot></slot>
</component-one>

my-app.html

<component-two>
  Content here...
</component-two>

In the end the content gets projected into the slot in component one. This is what I meant above when I said "...we need to keep projecting the content down until it bottoms out." It's hard to explain but hopefully the example above makes it clearer.

At first I wasn't a fan of an au-slot element, but maybe we do need something like that so that you can more explicitly choose when you want projection with or without shadow-dom. For that au-slot seems too generic. Maybe we want a different name. Not sure what that would be though. Perhaps something like <inline-slot> ? Just tossing out ideas...

@akircher
Copy link

akircher commented Oct 4, 2019

If we decide to support both, can they both be <slot> elements but we use an attribute to differentiate between the two types of behavior?

For me there is high-cognitive cost of having two different elements that do very similar things (footgun?). I feel like it fits better as an option within one element (whatever the name).

@EisenbergEffect
Copy link
Contributor

My concern with using <slot> is that we'd be altering the standard browser behavior, hijacking it for something different. We'd need to make it clear that something like <slot inline> was aurelia-specific and not part of the standard. We'd also have to make sure the compiler removed the element from the dom otherwise the browser would kick in with its slot behavior. Nothing insurmountable; just something to keep in mind.

Anyone else have opinions? I think I could be convinced either way.

@rluba
Copy link
Contributor Author

rluba commented Oct 4, 2019

I’m with @EisenbergEffect here. Using <slot> for this is a bad idea for the same reason that hijacking <a href> for the router configuration was a bad idea. It breaks standard behavior and works against browsers, leading to unpredictable behavior in the future.

Regarding the name: "inline" is a very overloaded word and I wouldn’t understand which meaning really applies in this context. I think <au-slot> is fine, as it conveys "special kind of slot that’s particular to Aurelia" and then you can read up on what its particularities are and why you might want to use it.

You could call it <light-slot> since it is both lighter (from an effect/consequences standpoint) than a normal <slot> and it’s "light" in the sense of "no shadow". (I’m usually not a fan of such "clever" wordplays, but I kind of like this one due to the former meaning.)

@fkleuver
Copy link
Member

fkleuver commented Oct 4, 2019

I think is fine, as it conveys "special kind of slot that’s particular to Aurelia" and then you can read up on what its particularities are and why you might want to use it.

Good point. Also, <au-slot> would be consistent with other built-in custom elements provided by the framework: <au-compose> and <au-viewport>.

@fkleuver fkleuver added this to Triage in Work queue Oct 7, 2019
@fkleuver fkleuver moved this from Triage to Backlog in Work queue Oct 7, 2019
@fkleuver fkleuver added API In discussion Suggestion Topic: Templating Topic: v1 Issues related to v1 backwards compatibility labels Oct 7, 2019
@3cp
Copy link
Member

3cp commented Oct 15, 2019

Actually we could also support the other way around to smooth user experience.

Users just use <au-slot> if

  1. it is hard for some users to understand the details.
  2. some users might want an option to turn on shadowDOM in future, but not happy to touch so many html template files when they decide to switch.

Our conventions plugin can rewrite <au-slot> to <slot> but only when shadowDOM is enabled in configuration.

@EisenbergEffect
Copy link
Contributor

@3cp That makes me a bit uncomfortable as it muddies what is standard and what is aurelia-specific. I'm not sure there's a ton of value in that, because in practice, one doesn't simply turn on shadow dom :) You generally have to re-work your CSS as well, which could be a large task. So, I think the work involved in changing a slot definition is trivial compared to the CSS re-work.

@3cp
Copy link
Member

3cp commented Oct 15, 2019

Nvm, I doubted myself if this is a bad idea. :)

@3cp
Copy link
Member

3cp commented Jun 12, 2020

Any plan for this before alpha release?

@Sayan751
Copy link
Contributor

Sayan751 commented Oct 3, 2020

The PR for au-slot is merged. That mimics the slot behavior without shadow DOM. The usage of native <slot> brings shadow DOM as expected. Primary examples of au-slots can be found in the PR, but the docs PR will follow.

Closing this in favor of au-slot. Feel free to open and/or continue discussion.

@Sayan751 Sayan751 closed this as completed Oct 3, 2020
Work queue automation moved this from Backlog to Done Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Work queue
  
Done
Development

No branches or pull requests

6 participants