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

Set display:none in addition to hidden attribute when hiding elements inside a shadow root #9026

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions flow-client/src/main/java/com/vaadin/client/PolymerUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -646,4 +646,20 @@ public static native void setProperty(Element element, String path,
element.set(path, value);
}-*/;

/**
* Returns true iff the element has a shadow root ancestor.
joheriks marked this conversation as resolved.
Show resolved Hide resolved
*
* @param element
* the element to test
* @return whether the element is in a shadow root
*/
public static native boolean isInShadowRoot(Element element)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an additional assertion that style={display:none} is not added in existing tests checking attribute hidden. This protects against regressions where it would be added unncessarily (for elements not in shadow root). Not sure how to set up a GWT test for the shadow root case.

I think we need more GWT tests for this.
It's possible to emulate code with shadow root with this code or change one.
You may create Elements hierarchy so that one element in the hierarchy returns '[object ShadowRoot]'.
Just fake it: create a fake toString method and return this string from it.

Another way could be reimplement this method a bit so that it doesn't use toString but check whether an element has a parent whose shadowRoot property is set to the element.
Then it's also possible to fake shadowRoot.

We have quite many tests like that which fakes elements.
See e.g. GwtBasicElementBinderTest: initPolymer , addShadowRootElement, etc.

With mocking/faking it should be possible to write the same tests for display style as we already have for hidden.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for the hints. Added two unit tests.

/*-{
while (element.parentNode && (element = element.parentNode)) {
if (element.toString() === '[object ShadowRoot]') {
return true;
}
}
return false;
}-*/;
}
Original file line number Diff line number Diff line change
Expand Up @@ -624,39 +624,55 @@ private void updateVisibility(JsArray<EventRemover> listeners,
.setValue(true);
restoreInitialHiddenAttribute(element, visibilityData);
} else {
updateVisibility(element, visibilityData, Boolean.TRUE);
setElementInvisible(element, visibilityData);
}
}

private void updateVisibility(Element element, NodeMap visibilityData,
Boolean visibility) {
private void setElementInvisible(Element element, NodeMap visibilityData) {
storeInitialHiddenAttribute(element, visibilityData);
updateAttributeValue(
visibilityData.getNode().getTree().getRegistry()
.getApplicationConfiguration(),
element, HIDDEN_ATTRIBUTE, visibility);
element, HIDDEN_ATTRIBUTE, Boolean.TRUE);
if (PolymerUtils.isInShadowRoot(element)) {
element.getStyle().setDisplay("none");
}
}

private void restoreInitialHiddenAttribute(Element element,
NodeMap visibilityData) {
MapProperty initialVisibility = storeInitialHiddenAttribute(element,
visibilityData);
storeInitialHiddenAttribute(element, visibilityData);
MapProperty initialVisibility = visibilityData
.getProperty(NodeProperties.VISIBILITY_HIDDEN_PROPERTY);
if (initialVisibility.hasValue()) {
updateAttributeValue(
visibilityData.getNode().getTree().getRegistry()
.getApplicationConfiguration(),
element, HIDDEN_ATTRIBUTE, initialVisibility.getValue());
}

MapProperty initialDisplay = visibilityData
.getProperty(NodeProperties.VISIBILITY_STYLE_DISPLAY_PROPERTY);
if (initialDisplay.hasValue()) {
final String initialValue = initialDisplay.getValue().toString();
element.getStyle().setDisplay(initialValue);
}
}

private MapProperty storeInitialHiddenAttribute(Element element,
private void storeInitialHiddenAttribute(Element element,
NodeMap visibilityData) {
MapProperty initialVisibility = visibilityData
.getProperty(NodeProperties.VISIBILITY_HIDDEN_PROPERTY);
if (!initialVisibility.hasValue()) {
initialVisibility.setValue(element.getAttribute(HIDDEN_ATTRIBUTE));
}
return initialVisibility;

MapProperty initialDisplay = visibilityData
.getProperty(NodeProperties.VISIBILITY_STYLE_DISPLAY_PROPERTY);
if (PolymerUtils.isInShadowRoot(element) && !initialDisplay.hasValue()
&& element.getStyle() != null) {
initialDisplay.setValue(element.getStyle().getDisplay());
}
}

private void doBind(StateNode node, BinderContext nodeFactory) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ public final class NodeProperties {
*/
public static final String VISIBILITY_HIDDEN_PROPERTY = "hidden";

/**
* The property used on the client side only in addition to
* {@link #VISIBLE}. It stores the client side value of the CSS "display"
* property to be able to restore when making a hidden element visible
joheriks marked this conversation as resolved.
Show resolved Hide resolved
* again.
*/
public static final String VISIBILITY_STYLE_DISPLAY_PROPERTY = "styleDisplay";

/**
* The property in Json object which marks the object as special value
* transmitting URI (not just any string).
Expand Down
12 changes: 12 additions & 0 deletions flow-tests/test-root-context/frontend/lit/lit-template-inner.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import {html, LitElement} from 'lit-element';

class TemplateInner extends LitElement {

render() {
return html`
<div>Hello template inner</div>
`;
}
}

customElements.define('lit-template-inner', TemplateInner);
joheriks marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 14 additions & 0 deletions flow-tests/test-root-context/frontend/lit/lit-template-outer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import {html, LitElement} from 'lit-element';
import './lit-template-inner.js';

class TemplateOuter extends LitElement {

render() {
return html`
<div>Hello template outer</div>
<lit-template-inner id="inner" style="display: block"></lit-template-inner>
`;
}
}

customElements.define('lit-template-outer', TemplateOuter);
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package com.vaadin.flow.uitest.ui.littemplate;

import com.vaadin.flow.component.Tag;
import com.vaadin.flow.component.dependency.JsModule;
import com.vaadin.flow.component.html.NativeButton;
import com.vaadin.flow.component.littemplate.LitTemplate;
import com.vaadin.flow.component.polymertemplate.Id;
import com.vaadin.flow.router.Route;
import com.vaadin.flow.uitest.servlet.ViewTestLayout;
import com.vaadin.flow.uitest.ui.AbstractDivView;

@Route(value = "com.vaadin.flow.uitest.ui.littemplate.InnerTemplateVisibilityView", layout = ViewTestLayout.class)
public class InnerTemplateVisibilityView extends AbstractDivView {

public static final String TOGGLE_INNER_VISIBILITY_BUTTON_ID = "toggleInnerVisibility";
public static final String TOGGLE_OUTER_VISIBILITY_BUTTON_ID = "toggleOuterVisibility";
public static final String INNER_ID = "inner";
public static final String OUTER_ID = "outer";

@Tag("lit-template-inner")
@JsModule("./lit/lit-template-inner.js")
public static class Inner extends LitTemplate {
public Inner() {
}
}

@Tag("lit-template-outer")
@JsModule("./lit/lit-template-outer.js")
public static class Outer extends LitTemplate {
@Id("inner")
Inner inner;

public Outer() {
}
}

public InnerTemplateVisibilityView() {
Outer outer = new Outer();
outer.setId(OUTER_ID);
outer.inner.setId(INNER_ID);

NativeButton toggleOuterVisibilityButton = new NativeButton(
"Toggle visibility of outer",
e -> outer.setVisible(!outer.isVisible()));
toggleOuterVisibilityButton.setId(TOGGLE_OUTER_VISIBILITY_BUTTON_ID);

NativeButton toggleInnerVisibility = new NativeButton(
"Toggle visibility of inner",
e -> outer.inner.setVisible(!outer.inner.isVisible()));
toggleInnerVisibility.setId(TOGGLE_INNER_VISIBILITY_BUTTON_ID);

add(toggleOuterVisibilityButton, toggleInnerVisibility, outer);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package com.vaadin.flow.uitest.ui.littemplate;

import org.junit.Assert;
import org.junit.Test;
import org.openqa.selenium.By;
import org.openqa.selenium.WebElement;

import com.vaadin.flow.component.html.testbench.NativeButtonElement;
import com.vaadin.flow.testutil.ChromeBrowserTest;

public class InnerTemplateVisibilityIT extends ChromeBrowserTest {

@Test
public void innerTemplateIsHiddenWithDisplayNone() {
open();

// when inner is hidden
NativeButtonElement toggleButton = $(NativeButtonElement.class)
.id(InnerTemplateVisibilityView.TOGGLE_INNER_VISIBILITY_BUTTON_ID);
toggleButton.click();

// then: element is not visible, attribute 'hidden' and 'display: none'
// set
WebElement outer = findElement(
By.id(InnerTemplateVisibilityView.OUTER_ID));
WebElement inner = findInShadowRoot(outer,
By.id(InnerTemplateVisibilityView.INNER_ID)).get(0);
Assert.assertFalse("expected inner to be hidden", inner.isDisplayed());
Assert.assertNotNull("expected attribute hidden on inner",
inner.getAttribute("hidden"));
Assert.assertEquals("expected 'display: none' on inner", "none",
inner.getCssValue("display"));
}

@Test
public void innerTemplateDisplayStyleRestored() {
open();

// when inner is hidden and unhidden
NativeButtonElement toggleButton = $(NativeButtonElement.class)
.id(InnerTemplateVisibilityView.TOGGLE_INNER_VISIBILITY_BUTTON_ID);
toggleButton.click();
toggleButton.click();

// then: element is visible, attribute and 'display: none' are no longer
// present
WebElement outer = findElement(
By.id(InnerTemplateVisibilityView.OUTER_ID));
WebElement inner = findInShadowRoot(outer,
By.id(InnerTemplateVisibilityView.INNER_ID)).get(0);
Assert.assertTrue("expected inner to be visible", inner.isDisplayed());
Assert.assertNull("inner should not have attribute hidden",
inner.getAttribute("hidden"));
Assert.assertEquals("expected 'display: block' on inner", "block",
inner.getCssValue("display"));
}

@Test
public void outerTemplateIsHiddenWithAttributeOnly() {
open();

// when hidden
NativeButtonElement toggleButton = $(NativeButtonElement.class)
.id(InnerTemplateVisibilityView.TOGGLE_OUTER_VISIBILITY_BUTTON_ID);
toggleButton.click();

// then: element is not visible, attribute 'hidden' and 'display: none'
// set
WebElement outer = findElement(
By.id(InnerTemplateVisibilityView.OUTER_ID));
Assert.assertFalse("expected outer to be hidden", outer.isDisplayed());
Assert.assertNotNull("expected attribute hidden on outer",
outer.getAttribute("hidden"));
Assert.assertEquals("expected no style attribute", "",
outer.getAttribute("style"));
}
}