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

Add Shadow DOM feature to isAttached #3996

Merged
merged 7 commits into from Nov 9, 2018

Conversation

Projects
None yet
4 participants
@SaptakS
Contributor

SaptakS commented Mar 2, 2018

Summary

This PR needs to be merged only after #3977

This change allows isAttached() function to address shadowDOMElements even if the browser doesn't support getRootNode().

Ref gh-3504, gh-3977

Checklist

  • All authors have signed the CLA at https://cla.js.foundation/jquery/jquery
  • New tests have been added to show the fix or feature works (Will add once this method is reviewed and confirmed)
  • Grunt build and unit tests pass locally with these changes

@SaptakS SaptakS changed the title from Feature/shadow dom/is attached to Add Shadow DOM feature to isAttached Mar 5, 2018

@mgol

This comment has been minimized.

Member

mgol commented Mar 5, 2018

Can you rebase this PR now that your other one has landed?

@SaptakS

This comment has been minimized.

Contributor

SaptakS commented Mar 5, 2018

@mgol done.

@mgol mgol added the Needs review label Mar 18, 2018

@gibson042

This is a good start, but we'll definitely need to add test cases before landing it.

Show resolved Hide resolved src/var/isAttached.js Outdated
Show resolved Hide resolved src/var/getShadowRoot.js Outdated
Show resolved Hide resolved src/var/isAttached.js Outdated
return obj.getRootNode( { composed: true } );
} else {
var doc = obj.ownerDocument;
return doc && doc.host ? getShadowRoot( doc.host ) : doc;

This comment has been minimized.

@gibson042

gibson042 Mar 19, 2018

Member

This will return the outer document for detached elements, which is incorrect. And we should probably have a loop instead of recursive calls anyway, but be mindful to return empty at all levels (e.g., including a shadow DOM inside a detached element).

@mgol

This will need unit tests, all new functionality has to be tested.

Show resolved Hide resolved src/var/getShadowRoot.js Outdated

@timmywil timmywil removed the Needs review label Jun 18, 2018

@timmywil

This comment has been minimized.

Member

timmywil commented Jun 18, 2018

This has been reviewed, but needs tests.

@mgol

This comment has been minimized.

Member

mgol commented Jun 27, 2018

@SaptakS will you have time to finish this PR soon?

@SaptakS

This comment has been minimized.

Contributor

SaptakS commented Jun 27, 2018

@mgol I will work on it this weekend. Sorry for the delay.

@timmywil timmywil added this to the 3.4.0 milestone Sep 3, 2018

@mgol

This comment has been minimized.

Member

mgol commented Sep 24, 2018

@SaptakS We're trying to finish stuff planned for 3.4.0. Will you have time to finish this today or tomorrow? If not, we may have to take over.

@SaptakS

This comment has been minimized.

Contributor

SaptakS commented Sep 24, 2018

I will give it a try today. Sorry for the delay.

Core: Allow shadow DOM in attachment check
Allow isAttached to check shadow DOM for attachment. This change allows
isAttached() function to address shadowDOM elements even if the browser doesn't support getRootNode()

Ref gh-3504, gh-3977
@mgol

This is looking good! I left some minor remarks. Of the major ones, we still need to:

  • test the usage in defaultPrefilter i.e. add some tests to the effects module.
  • there's some usage of isAttached in buildFragment to make sure attached scripts are marked as already evaluated to prevent double evaluation on manipulation; it'd be good to check that, e.g. via clone or append
  • check that elem.css(...) returns the computed value for shadown-inserted elements and not the inline value which happens for detached ones

Let me know if you need help with that.

Show resolved Hide resolved test/unit/css.js Outdated
Show resolved Hide resolved test/unit/css.js Outdated
@mgol

I left a few minor comments. We still need more tests as discussed offline. Nice progress!

Show resolved Hide resolved src/core/isAttached.js Outdated
Show resolved Hide resolved src/var/getShadowRoot.js Outdated
Show resolved Hide resolved src/var/getShadowRoot.js Outdated
Show resolved Hide resolved test/unit/css.js Outdated

@mgol mgol added the Needs review label Oct 22, 2018

@gibson042

The core of this looks solid, but I have some minor suggestions.

Show resolved Hide resolved test/unit/effects.js Outdated
var shadowChild = shadowRoot.querySelector( "#shadowChild" );
var x = jQuery( shadowChild );
x.fadeIn( 100, function() {

This comment has been minimized.

@gibson042

gibson042 Oct 22, 2018

Member

This whole block deserves its own test rather than being wedged into "Persist correct display value - {inline hidden,cascade hidden}", and I'd rather avoid using fadeIn etc. for tests that can be independent of them. Let's just simplify down to a verification that toggle works correctly with cascade-applied styles in shadow DOM.

Alternatively, we can switch some effects tests like this one from single-element to multi-element collections and make assertion across all of them (e.g., $span = jQuery( "#show-tests span" )$elems = jQuery( "#show-tests span" ).add( shadowChild ), with corresponding consequences). But that seems like overkill.

Show resolved Hide resolved test/unit/css.js Outdated
Show resolved Hide resolved test/unit/css.js Outdated
Show resolved Hide resolved src/core/isAttached.js Outdated
Tests: Check getShadowRoot works for isAttached function
- Checked with isHiddenWithinTree in toggle
- Checked with isHiddenWithinTree in show
- Checked with isHiddenWithinTree in fadeIn and fadeOut
@mgol

mgol requested changes Oct 29, 2018 edited

I've noticed only now we've been running some checks on Shadow DOM v0 and some on v1 where all should run on v1. I added comments to that effect.

With my suggestions, we're going from +80 bytes gzipped to +52 bytes. I'll have a look if we can gain anything more than that.

EDIT: I wrote v0 where I meant v1 🤦🏼‍♂️

Show resolved Hide resolved test/unit/effects.js
Show resolved Hide resolved src/core/isAttached.js Outdated
Show resolved Hide resolved src/var/getShadowRoot.js Outdated

mgol added some commits Oct 29, 2018

var $shadowChild = jQuery( shadowChild );
var displayNone = $shadowChild.css( "display" );
var display = "block";

This comment has been minimized.

@mgol

mgol Oct 29, 2018

Member

@gibson042 Note that this hardcodes "block" as opposed to the non-shadow test above this one. But, as @SaptakS made me realize - the check for $span.css( "display" ) in the non-shadow test requires to show the element and later hide it which changes how the element is hidden so the test doesn't really seem to test what it should. Also, in both cases, the expected value is "block" so it doesn't make sense to compute it during the test.

What do you think about changing the non-shadow test to avoid this check? (outside of this PR, of course).

@mgol

mgol approved these changes Oct 29, 2018

This looks good to me now. @gibson042, since you requested changes could you also have a look if it's OK now for you?

At this point, it's +52 minified. Suggestions on how to decrease that are welcome, I tried some things but I couldn't find anything obvious.

@mgol

This comment has been minimized.

Member

mgol commented Nov 2, 2018

There was one test failure which I fixed. This is ready for a final review.

@gibson042 do you want to have a look?

@mgol mgol added the Core label Nov 2, 2018

@timmywil timmywil removed the Needs review label Nov 5, 2018

@gibson042

At this point, it's +52 minified. Suggestions on how to decrease that are welcome, I tried some things but I couldn't find anything obvious.

This shaves off 11 more bytes, getting down to +41. It's surprisingly resilient against further compression, but I'm still in favor of merging.

"use strict";

var isAttached = function( elem ) {
		return jQuery.contains( elem.ownerDocument, elem );
	},
	composed = { composed: true };

// Check attachment across shadow DOM boundaries when possible (gh-3504)
if ( documentElement.attachShadow ) {
	isAttached = function( elem ) {
		return jQuery.contains( elem.ownerDocument, elem ) ||
			elem.getRootNode( composed ) === elem.ownerDocument;
	};
}

return isAttached;
@SaptakS

This comment has been minimized.

Contributor

SaptakS commented Nov 9, 2018

@gibson042 @mgol updated to put size optimization.

@mgol

mgol approved these changes Nov 9, 2018

Looks good, thank you!

@mgol mgol merged commit 9b77def into jquery:master Nov 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@GulajavaMinistudio GulajavaMinistudio referenced this pull request Nov 9, 2018

Merged

Unable to install #64

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment