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

Date binding notification events are broken if the bound element has children #5308

Closed
4 tasks done
tomalec opened this issue Jul 30, 2018 · 6 comments
Closed
4 tasks done

Comments

@tomalec
Copy link
Contributor

tomalec commented Jul 30, 2018

Description

Notification events do not work if the bound element has children. They seem to read the value from the descendant node instead of the bound one.

<div onclick="++this.value" value="{{value::click}}">
  works
  <b>fails</b>
</div>

Live Demo

https://glitch.com/edit/#!/small-duke?path=index.html:13:41

Steps to Reproduce

  1. Create div with property bound with notification event, like value="{{value::click}}"
  2. Append an element to the div
  3. Click (or trigger a bubbling event) on div

Expected Results

The property (value) should be changed.

Actual Results

The property is set to undefined or any value of descendant.property.

Browsers Affected

  • Chrome
  • Firefox
  • Edge
  • [?] Safari 11
  • [?] Safari 10
  • IE 11

Versions

  • Polymer: v2.6.0
  • webcomponents: v1.2.3

Possible fix

Seems that handleNotification should check event.currentTarget not event.target at

value = event.target[fromProp];

@TimvdLippe
Copy link
Contributor

@tomalec Seems like a valid case. Do you mind filing a PR with the fix + test cases? Please submit them to both the 2.x and the 3.x branch. Thanks! 😄

tomalec added a commit to tomalec/polymer that referenced this issue Jul 30, 2018
instead of target which may not be bound to any data.

Fixes Polymer#5308
@tomalec
Copy link
Contributor Author

tomalec commented Jul 30, 2018

@TimvdLippe Here you have 2.x #5309 :)

@tomalec
Copy link
Contributor Author

tomalec commented Jul 30, 2018

@jsilvermist That's the point I use native onclick to trigger event listener given in {{prop::event}} syntax

@TimvdLippe
Copy link
Contributor

@tomalec Awesome thanks. Once that is merged, please also open a PR on master for 3.x

tomalec added a commit to tomalec/polymer that referenced this issue Jul 30, 2018
instead of target which may not be bound to any data.

Fixes Polymer#5308
TimvdLippe pushed a commit that referenced this issue Aug 2, 2018
…5309)

instead of target which may not be bound to any data.

Fixes #5308
tomalec added a commit to tomalec/polymer that referenced this issue Aug 2, 2018
instead of target which may not be bound to any data.

Fixes Polymer#5308
TimvdLippe pushed a commit that referenced this issue Aug 2, 2018
…5313)

instead of target which may not be bound to any data.

Fixes #5308
@tomalec
Copy link
Contributor Author

tomalec commented Aug 2, 2018

Thanks a lot for the quick feedback :)

@TimvdLippe
Copy link
Contributor

TimvdLippe commented Aug 2, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants