Skip to content

Use DOM for marker content (allows listeners to work) #294

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

Open
tst-dhudlow wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use DOM for marker content (allows listeners to work) #294

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d608c9
Use DOM for marker content (allows listeners to work)
tst-dhudlow Apr 27, 2016
2ab7d4c
Must manage content when moving DOM
tst-dhudlow May 6, 2016
1dc0774
Use better test for content + comments
tst-dhudlow Jun 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions google-map-marker.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
display: none;
}
</style>
<template><content></content></template>
<template><div><content></content></div></template>
</dom-module>

<script>
Expand Down Expand Up @@ -366,8 +366,10 @@
subtree: true
});

var content = this.innerHTML.trim();
if (content) {
// Use this element's content DOM node for the info window content.
Copy link
Contributor

@ebidel ebidel Jul 5, 2016

Choose a reason for hiding this comment

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

element's content DOM -> element's light dom.

Copy link
Contributor

@ebidel ebidel Jul 5, 2016

Choose a reason for hiding this comment

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

I'd still like Line 41: <template><div><content></content></div></template> changed.

The idea is that users write:

<google-map-marker>
  <div>...</div> <!-- wrapper -->
</google-map-marker>

But you already have a wrapper inside shadow dom. Can we remove the one in shadow dom and update the docs to explain to users that they need to use a single wrapper in their light dom. We'll also need to rev a minor version b/c this is a breaking change.

Copy link
Contributor Author

@tst-dhudlow tst-dhudlow Jul 5, 2016

Choose a reason for hiding this comment

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

I thought @mlisook had a solution that didn't require the user to wrap in a single element via getContentChildNodes ?

Copy link
Contributor

@ebidel ebidel Jul 7, 2016

Choose a reason for hiding this comment

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

IIRC, it was traversing the existing DOM children.

// Note that this will move the contents, not copy them.
Copy link
Contributor

@ebidel ebidel Jul 5, 2016

Choose a reason for hiding this comment

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

Not really move, just preserve/reuse the existing DOM.

var content = this.children[0];
Copy link
Contributor

@ebidel ebidel Jun 6, 2016

Choose a reason for hiding this comment

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

This is selecting the first light dom child. are you meaning to pull the <div> in this element's shadow dom? I think this will work b/c of shady dom but fail under native shadow dom.

Copy link
Contributor

@mlisook mlisook Jun 12, 2016

Choose a reason for hiding this comment

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

(I'm not the author of this pull request, just interested in getting the issue fixed)

I've tried this line (369) with all four combinations of shady dom, shadow dom, webcomponents and webcomponents-lite and it certainly seems to work. Why might it fail under shadow dom?

On the other hand, issues:

The next line, 370 if (content) { ... }, fails because content always contains the new div element now so content itself is always true. This leads to opening an empty infowindow. Maybe 370 if (content.innerHTML.trim()) { ... }

More serious, Polymer ink bearing elements crash the infowindow. For example, when a paper-button or paper-icon-button is clicked inside the infowindow, elements are added inside the button to show the ink effect. This triggers _contentChanged() in the middle of the ink actions, and the infowindow content winds up empty. Non ink elements don’t seem to have this problem, for example an iron-icon with an on-tap event works fine.

Copy link
Contributor

@ebidel ebidel Jun 12, 2016

Choose a reason for hiding this comment

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

I'm trying to figure out why the <div> was added to the element's shadow dom. Also, var content = this.children[0]; won't select marker content if it's just text content (<google-marker>text content</google-marker>).

Copy link
Contributor

@ebidel ebidel Jun 12, 2016

Choose a reason for hiding this comment

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

@mlisook can you file a separate issue on the ink elements not working? That'll need investigation.

Copy link
Contributor

@mlisook mlisook Jun 12, 2016

Choose a reason for hiding this comment

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

Ahh, right you are, this.children[0] will not pick up content that is just text in shadow dom.

Well I’m fairly certain the idea behind adding the div was to be able to get a single element that could be moved into the infowindow content in one step, minimizing content change events. The way the infowindow content is currently populated with this.innerHTML.trim() leaves event bindings behind, of course.

The ink issue applies only to this pull request, not to the released code base, so I don’t think a new issue is appropriate.

While it may be possible to loop through this.childNodes to set the infowindow content and control for the content change events during the assembly, I’m now doubtful that issue #288 can be resolved with infowindow markup as content of the marker. Rapid mutation, such as the ink example, is just one reason.. There are alternatives to using the infowindow to display information on top of a map.

Copy link

@thexs-dev thexs-dev Jun 15, 2016

Choose a reason for hiding this comment

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

No please, don't abandon it, it's working fine and we need actions from within the infowindow
I am using a all the changes (in lines 41, 369, 384, plus new line 399) and it works in both scenarios:

  • with data binding and actions (code) within the infowindow element
  • and also working fine with just text as you suggested (text content)

Copy link
Contributor Author

@tst-dhudlow tst-dhudlow Jun 15, 2016

Choose a reason for hiding this comment

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

@ebidel Sorry, I've been busy with other projects and haven't been able to get back to these until now.

@mlisook is correct that the div was added so that you could depend on this.children[0] containing all of the contents. I can look into whether there's a way to make it work with just a text node and fix the if (content) predicate.

@mlisook, Not sure I understand what your theory is on why the ink elements don't work in the InfoWindow. What's the fundamental problem you see with moving the DOM node?

Is there another possible way to preserve the events handlers attached to the contents of the marker besides this idea? Realistically, the google-maps-marker element is radically less useful without this ability.

Copy link
Contributor

@ebidel ebidel Jun 15, 2016

Choose a reason for hiding this comment

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

My original question of whether this will work under native shadow dom is still bothering me. this.children[0]will give you the marker's light dom, but you've added the <div> wrapper to the element's shadow dom. This will require users to use a single wrapper for their marker content, so I'm not sure what the internal wrapper does.

<google-map-marker>
  <div>...</div>
</google-map-marker>

One thing you could do is use (this.$.contentwrapper), and the id: <template><content id="contentwrapper"></content></template>, and pull the distributed nodes: this.$.content.getDistributedNodes() to see what users have put inside the marker element. Then, wrap all that in a single <div>?

Copy link
Contributor

@mlisook mlisook Jun 15, 2016

Choose a reason for hiding this comment

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

@tst-dhudlow fundamentally, moving the DOM is absolutely the right thing to do. I also agree that preserving event handlers is vitally important.

I don't really understand what's happening during the ripple/ink effect, but it's not (as I had thought) that the infowindow content is destroyed - it's still there, but the display of the content in the browser is gone (see pics).

before clicking paper-icon-button +
p294-ink-a
after clicking paper-icon-button
p294-ink-b
Demo Code
ink-demo-pull-294.zip

Copy link
Contributor Author

@tst-dhudlow tst-dhudlow Jun 17, 2016

Choose a reason for hiding this comment

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

@ebidel In my use case, I'm using <google-map-marker><h2>...</h2><custom-element>...</custom-element>...</google-map-marker> not using a single wrapper. Using just text works as well for me.

I'm afraid I'm a bit fuzzy on the shadow dom / light dom distinction or what use cases the method I'm using breaks.

if (content && content.innerHTML.trim()) {
if (!this.info) {
// Create a new infowindow
this.info = new google.maps.InfoWindow();
Expand All @@ -381,7 +383,7 @@
}
this.info.setContent(content);
} else {
if (this.info) {
if (this.info && !this.open) {
// Destroy the existing infowindow. It doesn't make sense to have an empty one.
google.maps.event.removeListener(this.openInfoHandler_);
google.maps.event.removeListener(this.closeInfoHandler_);
Expand All @@ -396,6 +398,9 @@
this.info.open(this.map, this.marker);
this.fire('google-map-marker-open');
} else {
// When closing the info window, make sure the content is still available under this.content.
// This is necessary because we allow the info window to take the DOM node
this.content = this.info.getContent();
Copy link
Contributor

@ebidel ebidel Jun 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

can you add a comment why this is needed?

Copy link
Contributor Author

@tst-dhudlow tst-dhudlow Jun 15, 2016

Choose a reason for hiding this comment

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

When you use the DOM node instead of a copy of the HTML, the info window moves the node and is no longer under this element. When the InfoWindow is closed, you have to ensure that the contents are stored here, otherwise they're overwritten and lost whenever the InfoWindow is used to display some other marker's content.

Copy link
Contributor Author

@tst-dhudlow tst-dhudlow Jun 15, 2016

Choose a reason for hiding this comment

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

I'll add a comment to the code if you think I'm still on the right track and this remains applicable.

Copy link
Contributor

@ebidel ebidel Jun 15, 2016

Choose a reason for hiding this comment

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

Sg. Please add the comment :)

this.info.close();
this.fire('google-map-marker-close');
}
Expand Down