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

V 14.2.0: Component with styles exposed via WebComponentExporter #8543

Closed
darmro opened this issue Jun 10, 2020 · 27 comments
Closed

V 14.2.0: Component with styles exposed via WebComponentExporter #8543

darmro opened this issue Jun 10, 2020 · 27 comments

Comments

@darmro
Copy link

darmro commented Jun 10, 2020

Description of the bug

I have a component (simple div) annotated by @CssImport or @JsModule (contains styles for the component). I'm embedding it in another page as exposed web component via WebComponentExporter.

In V 14.1.x (till 14.1.28) everything was fine, styles was applied when component was embedded.

The same in V 14.2.0 stoped working ie the component doesn't see any of its styles.
The same component added to MainView (so not as a embedded component) of course looks fine (styles are applied). They are not applied only when component is embedded via WebComponentExporter.

Minimal reproducible example

  1. I took the Vaadin starter project
  2. I added few classes from the Creating an Embedded Vaadin Application Tutorial
  3. Then I added styles to LoginForm component by @JsModule. I just set a background of div (LoginForm) to red.

Run it (/example)
For V 14.1.28 the result:
image

And for V 14.2.0 the result:
image

The code (project)

starter-spring-war.zip

Expected behavior

Styles should be applied to the embedded component in V 14.2.0 as they were in V 14.1.28.

Summary

I'm not sure whether it's a bug. However everything was ok in V 14.1.x. Maybe I should add styles to the embedded component in some other way, but the documentation (link above) says nothing about it.

@mshabarov
Copy link
Contributor

Hello @darmro , thank you for ticket! Could you please re-upload the .zip file, I'm not able to open it.

@jouni
Copy link
Member

jouni commented Jun 12, 2020

Could this be related to #5363? A fix for style scoping was included in 14.2.

This looks like the fix: #7074
It introduces a shadow root for the embedded component, which could be an explanation why styles no longer apply to the components inside it, assuming the corresponding styles for it are not applied inside the shadow root as well.

@denis-anisimov
Copy link
Contributor

Could this be related to #5363? A fix for style scoping was included in 14.2.

#5363 has not been fixed directly anyhow.
I've just checked that I can't reproduce it anymore and closed it.

@mshabarov mshabarov added Severity: Blocker embedding flow Issues for embedding Flow applications and removed Severity: Major labels Jun 12, 2020
@mshabarov mshabarov moved this from Needs triage to P1 - High priority in OLD Vaadin Flow bugs & maintenance (Vaadin 10+) Jun 12, 2020
@darmro
Copy link
Author

darmro commented Jun 12, 2020

Hello @darmro , thank you for ticket! Could you please re-upload the .zip file, I'm not able to open it.

Hi, I've changed the zip in the conntent of this issue. Previous file was packed as rar (sorry :) New one is standard zip.

@denis-anisimov denis-anisimov self-assigned this Jun 15, 2020
@denis-anisimov
Copy link
Contributor

I'm not able to reproduce this with 16.0.0 Platform version.
But I indeed can reproduce this with 14.2.0.

Unfortunately the example in the ticket contains a lot of extra info:

  • Spring is totally unrelated and complicates the project set-up. Extra.
  • All programmatic logic code is extra.
  • Additional scripts are extra.

Below the steps to reproduce without all extra unrelated info:

  • checkout https://github.com/vaadin/skeleton-starter-flow.
  • Either use V14 or change the vaadin.version in the pom to 14.2.0.
  • Remove all Java classes.
  • Add the classes below.
  • Add login-form.js resource to the frontend/styles folder in the project (the content is below).
  • Run project (mvn jetty:run)
  • Open browser with /example URI.
  • The form is not styled: the background-color is not red

The same steps with the master branch for skeleton-starter-flow gives background-color red .

package pl.op.dar;

import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;

import com.vaadin.flow.component.button.Button;
import com.vaadin.flow.component.dependency.JsModule;
import com.vaadin.flow.component.formlayout.FormLayout;
import com.vaadin.flow.component.html.Div;
import com.vaadin.flow.component.textfield.PasswordField;
import com.vaadin.flow.component.textfield.TextField;
import com.vaadin.flow.function.SerializableRunnable;

@JsModule(value = "./styles/login-form.js") // added ==============
public class LoginForm extends Div {
    private TextField userName = new TextField();
    private PasswordField password = new PasswordField();
    private Div errorMsg = new Div();
    private String userLabel;
    private String pwdLabel;
    private FormLayout layout = new FormLayout();
    private List<SerializableRunnable> loginListeners = new CopyOnWriteArrayList<>();

    public LoginForm() {
        updateForm();

        add(layout);

        Button login = new Button("Login", event -> login());
        add(login, errorMsg);

        // added ==============
        addClassName("login-form");
    }

    public void setUserNameLabel(String userNameLabelString) {
        userLabel = userNameLabelString;
        updateForm();
    }

    public void setPasswordLabel(String pwd) {
        pwdLabel = pwd;
        updateForm();
    }

    public void updateForm() {
        layout.removeAll();

        layout.addFormItem(userName, userLabel);
        layout.addFormItem(password, pwdLabel);
    }

    public void addLoginListener(SerializableRunnable loginListener) {
        loginListeners.add(loginListener);
    }

    private void login() {
    }

    private void fireLoginEvent() {
        loginListeners.forEach(SerializableRunnable::run);
    }
}
package pl.op.dar;

import com.vaadin.flow.component.WebComponentExporter;
import com.vaadin.flow.component.webcomponent.WebComponent;

public class LoginFormExporter
		extends WebComponentExporter<LoginForm> {
	public LoginFormExporter() {
		super("login-form");
		addProperty("userlbl", "")
				.onChange(LoginForm::setUserNameLabel);
		addProperty("pwdlbl", "")
				.onChange(LoginForm::setPasswordLabel);
	}

	@Override
	protected void configureInstance(
			WebComponent<LoginForm> webComponent,
			LoginForm form) {
		form.addLoginListener(() -> webComponent.fireEvent("logged-in"));
	}
}
package pl.op.dar;

import javax.servlet.ServletException;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import java.io.IOException;
import java.io.PrintWriter;

@WebServlet(urlPatterns = { "/example" })
public class MainAppServlet extends HttpServlet {

    @Override
    protected void doGet(HttpServletRequest req, HttpServletResponse response)
            throws ServletException, IOException {
        response.setContentType("text/html;charset=UTF-8");

        try (PrintWriter out = response.getWriter()) {
            out.println("<!DOCTYPE html>");
            out.println("<html><head>");
            out.println("<meta http-equiv='Content-Type' content='text/html; "
                    + "charset=UTF-8'>");

            out.println("<script type='text/javascript' "
                    + "src='VAADIN/build/webcomponentsjs/"
                    + "webcomponents-loader.js'></script>");
            out.println("<script type='module' src='web-component"
                    + "/login-form.js'></script>");
            out.println("</head><body>");
            out.println("<login-form userlbl='Username' pwdlbl='Password'>"
                    + "</login-form>");
            out.println("</body>");
            out.println("</html>");
        }
    }
}

login-form.js :

import '@polymer/polymer/lib/elements/custom-style.js';

const $_documentContainer = document.createElement('template');

$_documentContainer.innerHTML = `<custom-style>
<style>
	.login-form {
		background-color: red;
	}
</style>
</custom-style>`;

document.head.appendChild($_documentContainer.content);

@denis-anisimov
Copy link
Contributor

OK, now I see the same behavior in 16.0.0 . It's weird.

@denis-anisimov
Copy link
Contributor

But it seems this is not related to embedding in fact.

What happens here:

<custom-style>
<style>
	.login-form {
		background-color: red;
	}
</style>
</custom-style>

the custom-style is added into the head tag but it doesn't affect anyhow login-form element .
If you apply background-color: red; directly to the login-form element via browser inspector then it's applied without problems.
So the effect is out of the embedding fucntionality.

@denis-anisimov
Copy link
Contributor

Ah , sorry.
the selector is .login-form , not login-form.
So it refers to the element inside shadow root.

@denis-anisimov
Copy link
Contributor

So in fact it works as designed.
Even though it worked in 14.1.28 we have changed the way how embedded components work now to prevent leaking styles from embedded web component to global styles.

See Jouni comment about this : #8543 (comment).
This is the main ticket for that : #7005.

To be able to style the web component you may use style directly in the web component:

or do the same thing as you did in login-form.js but it should
add styles to the dom-module :
$_documentContainer.innerHTML =<dom-module id="login-form"><template><style>.....

I will check the documentation about embedded components. It might be that we forgot to update it.

@darmro
Copy link
Author

darmro commented Jun 15, 2020

Thank you Denis for your investigation. In fact I expected such an answer (I've read before that you're plannig changes how styles and modules are applied) but for me this is bad news :/
Please note that the LoginForm is NOT a web component, it is a simple div with some global styling (by unique class name selector). In my app I have a lot (dozens) of such pseudo web components- simple divs with other components (other such pseudo components or vaadin web components) inside it, with some extra styling via @JsModule / @CssImport. For example I implemented whole generic crud functionality in this way, many usefull layouts, other complex "components". I'm using these components everythere in my app (normal vaadin app). And it works great, styles for these components are applied. But I have to expose some functionality to external CMS so I used perfect mechanism for this- WebComponentExporter. I created few widgets (extending WebComponentExporter) which can be embedded as web components in this external CMS (totaly different framework). Such a widget (web component) just contains these pseudo components with their own styling encapsulated within each component.
Up to 14.1.28 even such widget (web component created by WebComponentExporter) worked fine, styles were applied. Now they are not.
Your advice, although completely correct, means for me, that I have to add extra JsModule tags in each such widget (or create one module and include all related styles). So eg. when a widget uses 8 such pseudo components, each pseudo component already have styles added to it (JsModule annotation), despite this I have to check, which style module is used in each used pseudo component and I have to add it explicitly the second time in scope of the widget, is this correct? It's not very elegant, but if I don't have a choice, I will do it :)

@darmro
Copy link
Author

darmro commented Jun 15, 2020

Sorry, but I was wrong. Just adding to a widget (web component created by WebComponentExporter) each style module (via @JsModule annotation) used by each pseudo component used by this widget doesn't work, which is obvious, because whole widget is a web component, so every component inside it resides in shaddow dom).
This means that I have to style each widget like any other web component. And this means that I have to create another js module for each such widget with all styles inside dom-module just copied from each single file with styles used by pseudo component (so that styles were applied to components in shaddow dom). And in case of any change in pseudo component styling I have to remember to change component style file and widget style file. Am I right? :/

@denis-anisimov
Copy link
Contributor

Please note that the LoginForm is NOT a web component, it is a simple div with some global styling (by unique class name selector).

Right. But embedding makes a web component wrapper.
And yes, you have no control over this web component. It's created out of the box.
And now I see that this is complicated in fact.

Well, we have no other way to deal with this at the moment.
Please create a ticket to request a simpler way to style an embedded we component.
May be we should provide a way in WebComponentExporter which add web component styles to the generated web component element.
But please note that we not going to change the behavior back : allow applying global styles to the embedded web components. Otherwise it will allow to leak styles for embedded elements to global page which is bad and we may not allow this.
So the embedded component will be in shadow root of the generate element (or somehow hidden there any other way).

@denis-anisimov
Copy link
Contributor

I've made a ticket in docs how to style the embedded component : https://github.com/vaadin/flow-and-components-documentation/issues/1300

For now the easiest way I see is: make you own web component (which will be wrapped into a generated web component).
Then you may incapsulate the styles into this web component and embed it.
This way allows you to avoid global page modification and keep all styles inside the embedded web component itself.

@darmro
Copy link
Author

darmro commented Jun 15, 2020

Thank you Denis. I realize that you are not going to change the behavior back- to be honest I was sure you would say that :) But this means that using WebComponentExporter is now difficult to use when you expose some component (not web component) which has its own styling (his styling is now visible for web component exposed via WebComponentExporter).
One question: does web component generated by WebComponentExporter uses vaadin ThemableMixin utility? I've just tryed to style my widgets by dom-module, as I do to style all vaadin web components, and it seems that it doesn't work (styles are not applied in my widget).
So the only idea is your idea to create another custom web component which will be wrapped into a WebComponentExporter web component. And I have to remember to change styling in two places when styling of this pseudo component (div with styling) changes.
My quick idea is that mayby WebComponentExporter could parse all classes (components) used in exposed component for all @JsModules/@CssStyles annotations and include its content in local style of the web component (I don't know if it's possible).

@denis-anisimov denis-anisimov moved this from P1 - High priority to WIP in OLD Vaadin Flow bugs & maintenance (Vaadin 10+) Jun 15, 2020
@darmro
Copy link
Author

darmro commented Jun 15, 2020

Denis, what did you mean by:

or do the same thing as you did in login-form.js but it should add styles to the dom-module :
$_documentContainer.innerHTML =<dom-module id="login-form"><template><style>

I've changed login-form.js to use $_documentContainer.innerHTML =<dom-module id="login-form"><template><style>..... and tried different selectors but I couldn't make login-form to be red. Maybe because LoginForm is not a web component but simple div (can I style simple div using dom-module?).

@denis-anisimov
Copy link
Contributor

WebComponentExporter uses vaadin ThemableMixin utility?

No.
I've just realized that the generated wrapper is not PolymerElement at all.
It used to be but it has been changed.
That's why dom-module id="login-form" doesn't work as well: it works only for PolymerElements.

So the only way to style the embedded component is to wrap it into a web component with its own styles. Or set styles inline (hardcode via Element API "style" attribute).

@denis-anisimov
Copy link
Contributor

So, in the end:

  • this works as designed.
  • I've made a PR for docs which describes the design
  • I've made a ticket for docs to provide a tutorial for styling embedded components
  • Here is a ticket for making easier styling of embedded components: Use application theme for exported web components #8564

I'm closing this one since as I said: the behavior is expected.
Other issues are addressed via other tickets.

@jouni
Copy link
Member

jouni commented Jun 16, 2020

My quick idea is that mayby WebComponentExporter could parse all classes (components) used in exposed component for all @JsModules/@CssStyles annotations and include its content in local style of the web component (I don't know if it's possible).

Conceptually, this is what should be done. The exported component should “inherit” all the styles, which would normally be in the global style scope, to its local style scope.

@jouni
Copy link
Member

jouni commented Jun 16, 2020

Oh, didn’t notice Denis’s later comments.

I don’t agree that this works as intended/expected. Maybe it was technically designed to work like this, but I don’t think there’s justification why it should work like this. Good that you created #8564

To clarify: we will most likely need better support for this for design system documentation needs, when customers have done theme customizations for components. The Java component examples in the documentation are done using Flow embedding.

@denis-anisimov
Copy link
Contributor

Well, shadow root has been added to the generated wrapper by design.

There is no any justification that it should work in any other way in fact.

I also don't agree with the fact that global styles should be applied to the exported web component.
In fact I pretty sure: they should not.
The same way as styles for the embedded web component should not affect the page anyhow.

Embedded web component is a different world which is embedded to some universe.
I personally expect that web component should behave (including styling) the same way regardless which universe I embedded it.
Normally the web component is developed by different people.

But: I'm not an expert in this area. There was no exact requirements for this all all our judgment is done on our personal preference.

If you think this is a bug then we can close #8564 since the current ticket makes it useless.
But then I have no idea whether/when and how this specific ticket will be fixed: as I said I'm not an expert in this area and it won't be me who will work on this.

@jouni
Copy link
Member

jouni commented Jun 16, 2020

Well, shadow root has been added to the generated wrapper by design.

Yes, that’s indeed the correct way to do it, and for that the justification is sound, that the component should be isolated from the parent page. That wasn’t the thing I was arguing against 😄

With “global styles” I was referring to the styles that are applied in Flow using @CssImport. For example, if there’s a style sheet like this:

html {
  --lumo-primary-color: red;
}

I would want that to be applied in the exported component as well. I don’t know how that should be done, if the developer needs to explicitly do something else in addition to having a @CssImport annotation, which works in a regular Flow app.

@jouni
Copy link
Member

jouni commented Jun 16, 2020

Now, after writing the above example, I realize that doesn’t really make sense, as the embedded component won’t have any <html> element. So Flow would need to rewrite the style sheet for it to apply inside the shadow root of the exported component.

The :root selector would be great for this, if it still worked for shadow roots as well…

@jouni
Copy link
Member

jouni commented Jun 16, 2020

There was no exact requirements for this all all our judgment is done on our personal preference.

I know, and I apologize if I came off as accusatory. My intention was not to shame or point fingers. Just point out that I think we should still work on making this better 🤗

@denis-anisimov
Copy link
Contributor

it's fine.

@darmro
Copy link
Author

darmro commented Jun 16, 2020

I don'w want to complain but for me it is a very big problem :| As I said I have many java components (not web components / polymer components), which extends Div or Composite. Each one has its own styling in js file (mainly margins, paddings, flex attributes, size, sometimes colors or font properties). They are simple and primitive but I use them as lego bricks and put them in many other layouts, divs etc. In Flow app everything is nice and works. But when such layout (coponent) which contains these simple java components (with their own styling) I have to expose to be embedded somewhere else (WebComponentExporter was perfect for that) then it seems impossible (or very difficult or in rather ugly way). I think my scenario is rather common and it turns out that WebComponentExporter is not very usefull to generate embeddable web components :|

@denis-anisimov
Copy link
Contributor

There is a ticket about this: #8564

@darmro
Copy link
Author

darmro commented Jun 16, 2020

Thank You denis and jouni. I am looking forward to any improvements :) For now I have to stay on V 14.1.28.

denis-anisimov pushed a commit to vaadin/flow-and-components-documentation that referenced this issue Jun 16, 2020
Part of vaadin/flow#8543

Co-authored-by: Johannes Eriksson <joheriks@vaadin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug embedding flow Issues for embedding Flow applications Impact: High Severity: Blocker
Development

No branches or pull requests

4 participants