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.

Sign up for GitHub

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

Jump to bottom

Adds support for activeElement to Polymer.dom. #2717

Merged
merged 3 commits into from
Jan 8, 2016
Merged

Adds support for activeElement to Polymer.dom. #2717

merged 3 commits into from
Jan 8, 2016

Conversation

bicknellr
Copy link
Member

For document and shadow root nodes, if the active element is a (possibly deep) descendant of the given node, Polymer.dom(node).activeElement will return the deepest element within the light tree of the node which is or deeply contains the active element. For all other nodes, the getter and setter will forward to the activeElement property on the node.

@bicknellr bicknellr mentioned this pull request Nov 11, 2015
Merged
@bicknellr
Copy link
Member Author

@sorvell ping

sorvell
sorvell reviewed Nov 30, 2015
View reviewed changes
src/lib/dom-api.html
@@ -593,6 +593,55 @@

};

Object.defineProperties(DomApi.prototype, {
activeElement: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's separate the implementations of Shadow/Shady. Should make this more understandable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this might be a little better now.

@bicknellr bicknellr force-pushed the bicknellr/activeElement branch 3 times, most recently from 7dfc4d1 to 90e062d Compare December 5, 2015 00:52
@bicknellr
Copy link
Member Author

@sorvell This is ready for review again.

sorvell
sorvell reviewed Dec 9, 2015
View reviewed changes
src/lib/dom-api.html
var ownerDocument = this.node.ownerDocument;

// Shadow DOM
if (Polymer.Settings.useShadow) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This section should be its own getter in the useShadow section of this module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved.

@bicknellr
Copy link
Member Author

@sorvell ping again

@bicknellr bicknellr mentioned this pull request Dec 10, 2015
Merged
@valdrinkoshi valdrinkoshi mentioned this pull request Dec 10, 2015
Merged
@bicknellr
Copy link
Member Author

So, since Polymer runs the tests on Chrome's native version of Shadow DOM, it looks like I didn't correctly interpret the retargeting algorithm for activeElement on ShadowRoots. Specifically, it seems like if the active element is within the light DOM of a ShadowRoot's host, activeElement of that ShadowRoot should be the active element rather than null. I need to fix this in the web components polyfills and then come back here. :/

sorvell
sorvell reviewed Dec 12, 2015
View reviewed changes
src/lib/dom-api.html
if (ownerDocument || !window.ShadowDOMPolyfill) {
return this.node.activeElement;
} else {
return window.ShadowDOMPolyfill.wrapIfNeeded(this.node).activeElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

This file has a wrap implementation so I think this can just be:

return DomApi.wrap(this.node).activeElement;

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@bicknellr
Copy link
Member Author

(PR for activeElement fix in web components polyfill.)

@bicknellr
Copy link
Member Author

Updated with the expectation that the web components polyfill PR will make it in, tests will fail until then.

sorvell
sorvell reviewed Dec 12, 2015
View reviewed changes
src/lib/dom-api.html
activeElement: {
configurable: true,
get: function() {
return wrap(this.node).activeElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be DomApi.wrap

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean Polymer.DomApi.wrap? The DomApi constructor defined in this file doesn't have a wrap property - the object returned / assigned to Polymer.DomApi does though. However, the other function calling wrap calls it directly since it's defined in this scope. Thoughts?

@sorvell
Copy link
Contributor

sorvell commented Dec 12, 2015

Only 1 little nit left, otherwise LGTM after we get the webcomponents change merge. Thanks!

@bicknellr
Copy link
Member Author

I had to add additional tests and make an update to the host-finding loop for this new case - might want to double check that.

bicknellr
bicknellr reviewed Dec 14, 2015
View reviewed changes
src/lib/dom-api.html
// active element is a descendant of its host; iterate upwards to
// find the active element's most shallow host.
var activeRoot = Polymer.dom(active).getOwnerRoot();
while (activeRoot && activeRoot !== this.node && !(this.node._isShadyRoot && this._contains(this.node.host, active))) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the new change; is there a preferred way to format really long conditionals like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, 80 character line limit + indenting.

@valdrinkoshi
Copy link
Member

@sorvell ping for this one, needed by PolymerElements/iron-overlay-behavior#68

@bicknellr
Copy link
Member Author

Depends on webcomponents/webcomponentsjs#475.

sorvell
sorvell reviewed Jan 8, 2016
View reviewed changes
src/lib/dom-api-shadow.html
get: function() {
var node = Polymer.DomApi.wrap(this.node);
var activeElement = node.activeElement;
return node.contains(activeElement) ? activeElement : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment here that explains the contains check. I believe this is so that we ignore distributed nodes. We should also link to the spec bug where the native behavior is changing.

@bicknellr
Copy link
Member Author

@sorvell: fixed stuff from your comments

@sorvell
Copy link
Contributor

sorvell commented Jan 8, 2016

LGTM

sorvell pushed a commit that referenced this pull request Jan 8, 2016
Merge pull request #2717 from Polymer/bicknellr/activeElement
44232bd 
Adds support for `activeElement` to `Polymer.dom`.
@sorvell sorvell merged commit 44232bd into master Jan 8, 2016
@sorvell sorvell deleted the bicknellr/activeElement branch January 8, 2016 23:38
@cdata cdata mentioned this pull request Jan 23, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sorvell sorvell

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bicknellr @sorvell @valdrinkoshi @googlebot