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

tst-dhudlow · 2016-05-03T11:42:53Z

See #288

May not be exactly the right solution, but it does seem to work.

thexs-dev · 2016-05-09T11:25:00Z

Any idea when this fix will be released?
Or how to use it within a project (bower path, version)?
Thanks

jesusabarca · 2016-06-05T23:47:38Z

This fixed my issue :)

ebidel · 2016-06-06T01:15:31Z

google-map-marker.html

      });

-      var content = this.innerHTML.trim();
+      var content = this.children[0];


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.

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

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

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

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.

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)

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

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>?

@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 +

after clicking paper-icon-button

Demo Code
ink-demo-pull-294.zip

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

ebidel · 2016-06-12T17:18:15Z

@tst-dhudlow can you take a look at my comments? Interested in getting this merged.

thexs-dev · 2016-06-22T22:12:18Z

I have been using the modified code and it's working fine for my purpose of having binding and interaction within the info-window
I have found though, a couple of issues as follow
Apologies in advance for the format I am using, but I don't know how to it better

When using a paper-dropdown-menu element within the info-window, if you pan/zoom, then click on a market and open the dropdown, the map applies back the fit-to-markers (resize, center). This happens with Polymer versions 1.4.0 and 1.5.0
To avoid this issue I moved from a paper-dropdown-menu to a paper-dialog with a paper-dropdown-menu. With that the map stays where it was when opening the dialog
The second issue only happens with version 1.5.0. In either case, using a paper-dropdown-menu or a paper-dialog, after I click on a marker I would get errors in the console as follow, when opening the info-window, Uncaught TypeError: Cannot read property 'addEventListener' of null (iron-a11y-keys-behavior.html:404), and when closing the info-window, Uncaught TypeError: Cannot read property 'removeEventListener' of null (iron-a11y-keys-behavior.html:421).
Other than the error messages, the app seems to be working fine.

This is the code I am using, in case you want to reproduce these issues.
https://jsfiddle.net/9s9yx8j1/
Just replace the google-map with the modified version of google-map-marker, and try with Polymer version 1.5.0

mlisook · 2016-06-23T21:32:24Z

@tst-dhudlow I'm sorry, but the concept of the wrapping div around the content tag is just not going to work. Its not just plain text nodes, either, as the earlier dialog implied.

I'll admit that when I first saw this I thought it was clever, brilliant even. Now, however, I know more than I ever thought I wanted to about shadow.

See this JS Bin with the chrome browser to illustrate.

The div around the content tag becomes part of the shadow (or shady) dom. Inside the content are the light dom. The children of a web component element are its light dom, not its shadow + light dom. When this works under shady it is because the polyfill assembles all the elements in the regular dom and it (the polyfill) has no way to prevent a native function like children from operating as it always does in regular dom. In native shadow dom that div is just not in the source tree of the regular dom, so, children[0] refers to the first element inside the content tag.

With chrome's support for shadow dom pretty robust, the Polymer team is recommending to run with shadow as the default (e.g. see what's generated by the polymer cli) and just fall back to shady if the browser doesn't support shadow. We're just not going to be able to have a fix that doesn't work in shadow.

ebidel · 2016-06-23T21:47:12Z

I think my comments in #294 (comment) still need to be addressed.

mlisook · 2016-06-24T00:33:24Z

@ebidel The method in your comment from last week will work to allow the content of a child, like the google-map-marker, to be moved into a parent's shadow dom (google-map).

I set up a JS Bin with an ultra simplified demo (ultra, like not even a google-map). It is analogous though. Infowindow creates a div inside the map div for its styled contents, then translates that into place within the map div. The JS Bin works on both shadow and shady.

What I don't know, however, is the inner workings of Infowindow. Based on the last comment by @Fausto-theXS and my comment 11 days ago about ink bearing / ripple elements, it seems like the Infowindow has its own content / mutation observer. It may not be compatible with the types of action that happen in material design elements. In the sample I attached last week, it can be seen that the content of the infowindow has its height and width set to 0 when the ripple fires. I have no idea why, but the elements inside do go to 0 x 0 on ripple. Its also unclear what effect styling of the content would have on moving the elements from light dom to shadow/shady. Especially in shadow, the classes from outside wouldn't apply after moving, right?

This will be extremely difficult to test.

mlisook · 2016-06-24T15:02:17Z

I modified that last JS Bin to include a style on one of the elements moving. The style is quashed under shadow dom, but shows under shady, as I suspected.

tst-dhudlow · 2016-06-29T00:38:06Z

@mlisook I can try your method to move the content and test it in shadow to see if it works, but if you know what it should look like already, feel free to PR into my branch and I'll merge it so it goes into this PR.

mlisook · 2016-06-30T13:06:34Z

@tst-dhudlow I can do that (PR into your branch), but I'm in an area with only sketchy internet access until after the holiday.

With that it should be possible to support everything but styles. As far as I can tell there's no way the Infowindow will support style classes under shadow dom. To have full support we'd have to go big - not using infowindow at all. That would involve making google-map-marker a visual component that floats its content over the map on demand (like the paper-map-info, (or see demo), I did to get my current project working), but styling as close as possible to infowindow and probably duck typing the infowindow public interface. Not sure if management would entertain that, @ebidel your thoughts?

ebidel · 2016-06-30T15:32:01Z

I'm hesitate to support custom info windows. It's not a common case. Could
you not add custom info windows yourself with the underlying api calls?

On Thu, Jun 30, 2016, 8:06 AM Murray Lisook notifications@github.com
wrote:

@tst-dhudlow https://github.com/tst-dhudlow I can do that (PR into your
branch), but I'm in an area with only sketchy internet access until after
the holiday.

With that it should be possible to support everything but styles. As far
as I can tell there's no way the Infowindow will support style classes
under shadow dom. To have full support we'd have to go big - not using
infowindow at all. That would involve making google-map-marker a visual
component that floats its content over the map on demand (like the
paper-map-info https://github.com/mlisook/paper-map-info, (or see demo
https://mlisook.github.io/paper-map-info/), I did to get my current
project working), but styling as close as possible to infowindow and
probably duck typing the infowindow public interface. Not sure if
management would entertain that, @ebidel https://github.com/ebidel your
thoughts?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#294 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAOigBBEucGJ_UYaCOyFtecEytzcfNT-ks5qQ79agaJpZM4IWJF9
.

thexs-dev · 2016-06-30T16:09:35Z

@ebidel Is there a "Polymeric" way of doing that?
I mean a single reusable Infowindow object (new this.api.InfoWindow()), that I can invoke on-google-map-marker-click event and open it with a paper-card element as content?

I'd rather do that than having thousands of infowindows in the DOM, when the user would click on just a few of them -- a huge performance improvement

mlisook · 2016-07-05T13:16:45Z

@Fausto-theXS the reusable window that is invoked on-google-map-marker-click is how the paper-map-info (demo) works.

mlisook · 2016-07-05T13:29:42Z

@ebidel I understand and agree completely.

I don't think the styles thing is as important as events either, although it is outside developer's expectations, since something as straightforward as:

<style>
    foo {
      color: red;
    }
</style>
<google-map latitude="37.779" longitude="-122.3892" >
  <google-map-marker latitude="37.779" longitude="-122.3892">
    <span class="foo">Go Giants!</span>
  </google-map-marker>
</google-map>

Will never work to style the text under shadow dom with the native infowindow (but does under shady).

ebidel · 2016-07-05T13:37:55Z

google-map-marker.html


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


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

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

The idea is that users write:

<google-map-marker> <div>...</div>  </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.

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

IIRC, it was traversing the existing DOM children.

…ogle-map#294 PR

jonadeline · 2017-09-20T10:39:07Z

Hi guys,
I developed custom-gmap-infowindow element to customize the infowindow. It's based on @mlisook plastic-map-info. Do not hesitate to contribute to enhance the thing.

Use DOM for marker content (allows listeners to work)

2d608c9

googlebot added the cla: yes label May 3, 2016

Must manage content when moving DOM

2ab7d4c

thexs-dev mentioned this pull request Jun 3, 2016

google-map-marker content does not update in dom-repeat #263

Open

ebidel reviewed Jun 6, 2016
View reviewed changes

Use better test for content + comments

1dc0774

tst-dhudlow mentioned this pull request Jun 29, 2016

Fix for marker content update bug #295

Open

ebidel reviewed Jul 5, 2016
View reviewed changes

klarkc added a commit to klarkc/ondetrabalhar that referenced this pull request Aug 18, 2016

change google-map dependency to a version with GoogleWebComponents/go…

c79019d

…ogle-map#294 PR

klarkc added a commit to klarkc/ondetrabalhar that referenced this pull request Aug 18, 2016

change google-map dependency to a version with GoogleWebComponents/go…

3f9b823

…ogle-map#294 PR

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

tst-dhudlow commented May 3, 2016

Uh oh!

thexs-dev commented May 9, 2016

Uh oh!

jesusabarca commented Jun 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebidel commented Jun 12, 2016

Uh oh!

thexs-dev commented Jun 22, 2016

Uh oh!

mlisook commented Jun 23, 2016

Uh oh!

ebidel commented Jun 23, 2016

Uh oh!

mlisook commented Jun 24, 2016

Uh oh!

mlisook commented Jun 24, 2016

Uh oh!

tst-dhudlow commented Jun 29, 2016

Uh oh!

mlisook commented Jun 30, 2016

Uh oh!

ebidel commented Jun 30, 2016

Uh oh!

thexs-dev commented Jun 30, 2016

Uh oh!

mlisook commented Jul 5, 2016

Uh oh!

mlisook commented Jul 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonadeline commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants