New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 665979 link

Starred by 1 user
Status: WontFix
Owner: ----
Closed: Jul 21
Cc:
dbeam@chromium.org
█ dschuyler@chromium.org
steve...@chromium.org
dpa...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

MD Settings: implement a settings-checkbox-change event.

Project Member Reported by dpa...@chromium.org, Nov 16 2016 
At https://codereview.chromium.org/2501783003, I use an iron-change event to detect changes to a settings-checkbox. While writing tests a subtle bug was revealed.

'iron-change' is fired by the paper-checkbox inside the settings-checkbox. Listening to iron-change outside of the settings-checkbox (with bubbling), is not safe, because the surrounding settings-checkbox has not updated itself yet, so it does not have the "checked" HTML attribute yet.

In other words, the 'iron-change' listener executes before the settings-checkbox#checkedChanged_ observer has fired, and before settings-checkbox has updated its internal state to reflect the 'checked' Polymer property as an HTML attribute.

Comment 1 by dbeam@chromium.org, Nov 16 2016

Status: WontFix (was: Untriaged)
<settings-checkbox> does not guarantee that it dispatches an iron-change event / is implemented with a <paper-checkbox>.  yes, dash-form CustomEvents pierce shadow DOM, but I don't think's a good reason to depend on them.

<settings-checkbox> does notify outer scope when its checked member changes:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js?q=SettingsBooleanControlBehavior&sq=package:chromium&dr=C&l=24
so i'd recommend using that.

if we need an event, we should do something like fire('settings-checkbox-change') instead, IMO.

Comment 2 by dpa...@chromium.org, Nov 16 2016

Labels: -Pri-2 Pri-3
Status: Available (was: WontFix)
Summary: MD Settings: implement a settings-checkbox-change event. (was: MD Settings: Can't rely on iron-change event to detect changes in settings-checkbox.)
At the very least we should not be letting the iron-change event propagate through settings-checkbox to parents, because this just allows a future developer to get confused, the same way I was confused.

Firing a 'settings-checkbox-change' at the right time, when both the Polymer property and the HTML attribute 'checked', have been updated seems the best solution.

I'll reopen for now.

Comment 3 by dpa...@chromium.org, Nov 28 2016

Cc: steve...@chromium.org

Comment 4 by tbuck...@chromium.org, Jan 7 2017

Labels: Hotlist-MD-Settings-General
Project Member

Comment 5 by sheriffbot@chromium.org, Feb 21 2018

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by dpa...@chromium.org, Jul 21

Status: WontFix (was: Untriaged)
Closing this bug. This element no longer uses paper-checkbox internally, and #2 makes a good point. If we want to be extra careful, we could call stopPropagation() on any events that come from internal elements and are not meant to be bubbled to clients.
►

Sign in to add a comment