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

Conversation

joheriks
Copy link
Contributor

Fixes #8256.

@joheriks joheriks added this to Iteration Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) via automation Sep 17, 2020
@joheriks joheriks force-pushed the joheriks/8256-template-visibility-in-shadow-root branch from a3eedc8 to b55fe75 Compare September 17, 2020 21:25
Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

There are some GWT unit tests for visibility.

Please review them whether they need to be extended.
Some tests are for the visibility feature (bind/unbind) mostly.

But other tests are for hidden attribute behavior.

E.g.
testBindHiddenElement_stateNodeIsVisible_elementStaysHidden
testBindHiddenElement_stateNodeChangesVisibility_elementStaysHidden,
testBindNotHiddenElement_stateNodeChangesVisibility_elementIsNotHidden, ....

@joheriks
Copy link
Contributor Author

There are some GWT unit tests for visibility.

Please review them whether they need to be extended.
Some tests are for the visibility feature (bind/unbind) mostly.

But other tests are for hidden attribute behavior.

E.g.
testBindHiddenElement_stateNodeIsVisible_elementStaysHidden
testBindHiddenElement_stateNodeChangesVisibility_elementStaysHidden,
testBindNotHiddenElement_stateNodeChangesVisibility_elementIsNotHidden, ....

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.

* 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.

@denis-anisimov denis-anisimov merged commit deab152 into master Sep 22, 2020
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Done - pending release Sep 22, 2020
@denis-anisimov denis-anisimov deleted the joheriks/8256-template-visibility-in-shadow-root branch September 22, 2020 07:57
joheriks pushed a commit that referenced this pull request Sep 23, 2020
…ents inside a shadow root (#9026)

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

Fixes #8256
joheriks pushed a commit that referenced this pull request Sep 23, 2020
…ents inside a shadow root (#9026)

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

Fixes #8256
joheriks pushed a commit that referenced this pull request Sep 23, 2020
…ents inside a shadow root (#9026)

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

Fixes #8256
joheriks pushed a commit that referenced this pull request Sep 23, 2020
…ents inside a shadow root (#9026)

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

Fixes #8256
joheriks pushed a commit that referenced this pull request Sep 28, 2020
…ents inside a shadow root (#9026)

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

Fixes #8256
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
4 participants