Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688073002:
MD Settings: For rows containing paper-toggle-buttons, make entire row clickable. (Closed)
|
Created:
3 years, 10 months ago by scottchen Modified:
3 years, 10 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: For rows containing paper-toggle-buttons, make entire row clickable.
BUG=680406
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Closing based on off-line discussion with dbeam@ and dpapad@, we're going to go with another approach with pref plus some twist (to be implemented by dbeam).
Patch Set 1 #
Total comments: 1
Patch Set 2 : convert a more complicated row #
Total comments: 8
Patch Set 3 : prevent clicking toggle from firing handler twice #
Total comments: 7
Messages
Total messages: 13 (3 generated)
Description was changed from ========== MD Settings: For rows containing paper-toggle-buttons, make entire row clickable. BUG=680406 ========== to ========== MD Settings: For rows containing paper-toggle-buttons, make entire row clickable. BUG=680406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Here's the demo showing the first changed toggle working: https://drive.google.com/open?id=0ByIc0PEad7DmWndyR3dUOVAxRFU For the second changed toggle, it's policy-controlled on my machine so I'm still trying to hack the browser-proxy/c++ code to bypass the policy control, but I suspect it'll work just as the first one did since they're pretty similar (just that one is bound to a boolean and one to an object). I'll post testing screen cap of the second one once I get rid of the policy control. https://codereview.chromium.org/2688073002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2688073002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:88: on-change="onSafeBrowsingExtendedReportingControlTap_" This is needed to handle dragging, since on-tap doesn't fire when the toggle is dragged.
scottchen@chromium.org changed reviewers: + dbeam@chromium.org, dpapad@chromium.org
+ dbeam@ dpapad@ as reviewers.
https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:108: <div class="settings-box" on-tap="onMetricsReportingControlTap_" this changes the clickable area (compared to other rows implemented with <settings-toggle-button>) https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:108: <div class="settings-box" on-tap="onMetricsReportingControlTap_" when you tap the box, you want to invert the value https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:124: on-change="onMetricsReportingControlTap_" this doesn't work when you drag the toggle https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:124: on-change="onMetricsReportingControlTap_" but when this changes, you DON'T want to invert the value (it just changed)
it's almost like i've tried this before! https://codereview.chromium.org/2684853003/#ps1
On 2017/02/10 06:51:40, scottchen wrote: > + dbeam@ dpapad@ as reviewers. Sorry, I take back what I said - having both on-change and on-tap does trigger it twice when you tap the toggle, it's just that the C++ executes slower for THAT specific toggle, so the javascript fired twice before the value changed and made it look like it's working correctly. Well, it luckily always works out for this toggle but for other toggles it might not be the case.. so the issue at hand is 1. clicking on the row triggers only on-tap 2. dragging the toggle triggers only on-change 3. tapping the toggle triggers both on-tap and on-change. So the two possible solution to make this approach work would be: 1. if we could make dragging work without having to have the on-change toggle, OR 2. make tapping the toggle trigger only one of on-tap or on-change. Will try to work one of these out.
https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:124: on-change="onMetricsReportingControlTap_" allllso, when you tap on the toggle, you get double events first, you get this change event THEN, you get the on-tap of the parent.
On 2017/02/10 07:13:47, scottchen wrote: > On 2017/02/10 06:51:40, scottchen wrote: > > + dbeam@ dpapad@ as reviewers. > > Sorry, I take back what I said - having both on-change and on-tap does trigger > it twice when you tap the toggle, it's just that the C++ executes slower for > THAT specific toggle, so the javascript fired twice before the value changed and > made it look like it's working correctly. Well, it luckily always works out for > this toggle but for other toggles it might not be the case.. > > so the issue at hand is > 1. clicking on the row triggers only on-tap > 2. dragging the toggle triggers only on-change > 3. tapping the toggle triggers both on-tap and on-change. > > So the two possible solution to make this approach work would be: > 1. if we could make dragging work without having to have the on-change toggle, > OR > 2. make tapping the toggle trigger only one of on-tap or on-change. > > Will try to work one of these out. https://codereview.chromium.org/2684853003/diff/110001/chrome/browser/resourc...
Just made one more patch, concluding at a workable solution. https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:191: doNothing_: function(e) { This is dipping into the "slightly gross" zone, but not too horrible. And I think we can reasonably make this a reusable thing - maybe [is="contained-toggle"], or make the parent a [is="toggle-container"] and have its child paper-toggle-button always stop propagating tap, or something. I think we can achieve this without creating shadow-dom I believe, much like is=action-link.
https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:88: on-change="onSafeBrowsingExtendedReportingControlTap_" while your mouse is down dragging, other toggles change their value at each edge. starting here: 0== grabbing the knob and moving to here without letting go ==0 should change the effective value. without letting go, moving between 0== ==0 should continue to change the effective value. does that work with your CL? i'd guess not, because 'change' is only dispatched at the end of this madness, whereas iron-change occurs the whole time. https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:89: on-tap="doNothing_" this is very similar to https://codereview.chromium.org/2684853003/diff/20001/chrome/browser/resource... which has a stopPropagation() branch for taps it's a little nicer but of on-tap was ever changed to on-click on the parent or on the child, this would potentially still have double events. which is why i pushed them both through the same method and asserted they're the same e.type (event type). https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:150: var enabled = !this.safeBrowsingExtendedReportingEnabled_; i still don't really understand how this works. but on-change="onSafeBrowsingExtendedReportingControlTap_" means "hey, the value just changed. the new value is what you want". then you invert it and send it to C++. how is this correct?
https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:124: on-change="onMetricsReportingControlTap_" On 2017/02/10 07:11:36, Dan Beam wrote: > but when this changes, you DON'T want to invert the value (it just changed) depends on which value. You don't wanna invert .checked, but you do wanna invert metricsReporting_.enabled; based on my experiment, changing the checkbox value doesn't change metricsReporting_.enabled unless the handler says to do so (assuming checked= is one-way bounded). This is also why if you just have on-tap, dragging doesn't work in two windows. https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:124: on-change="onMetricsReportingControlTap_" On 2017/02/10 07:11:36, Dan Beam wrote: > this doesn't work when you drag the toggle on-tap doesn't fire, but on-change seems to fire when dragged. https://codereview.chromium.org/2688073002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:124: on-change="onMetricsReportingControlTap_" On 2017/02/10 07:14:51, Dan Beam wrote: > allllso, when you tap on the toggle, you get double events > > first, you get this change event > > THEN, you get the on-tap of the parent. Acknowledged. Fixed with a later patch. https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:88: on-change="onSafeBrowsingExtendedReportingControlTap_" On 2017/02/10 17:20:36, Dan Beam wrote: > while your mouse is down dragging, other toggles change their value at each > edge. > > starting here: > > 0== > > grabbing the knob and moving to here without letting go > > ==0 > > should change the effective value. without letting go, moving between > > 0== > ==0 > > should continue to change the effective value. > > does that work with your CL? i'd guess not, because 'change' is only dispatched > at the end of this madness, whereas iron-change occurs the whole time. it does - see https://drive.google.com/open?id=0ByIc0PEad7DmWndyR3dUOVAxRFU https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:89: on-tap="doNothing_" On 2017/02/10 17:20:36, Dan Beam wrote: > this is very similar to > https://codereview.chromium.org/2684853003/diff/20001/chrome/browser/resource... > > which has a stopPropagation() branch for taps > > it's a little nicer but of on-tap was ever changed to on-click on the parent or > on the child, this would potentially still have double events. which is why i > pushed them both through the same method and asserted they're the same e.type > (event type). I think this can be handled if we encapsulate it in a is="contained-toggle" and just do something like... html: <div is=contained-toggle handler="[[onThisTriggered_]]"> <paper-toggle-button checked="[[value]]" ...(other attr EXCEPT any on-x handler)/> </div> contained-toggle.js (pseudo-code): var eventName = "tap"; //if you ever think we would listen to click instead of tap this.root.addEventListener(eventName, handler); var toggle = this.root.querySelector("paper-button-toggle"); toggle.addEventListener("change",handler); toggle.addEventListener(eventName, doNothing_); I believe the above solution would solve your concern, and this'll also help us reduce the boilerplate code we need to type up each time we want to use a "custom toggle element". https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2688073002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:150: var enabled = !this.safeBrowsingExtendedReportingEnabled_; On 2017/02/10 17:20:36, Dan Beam wrote: > i still don't really understand how this works. > > but on-change="onSafeBrowsingExtendedReportingControlTap_" means "hey, the value > just changed. the new value is what you want". then you invert it and send it > to C++. how is this correct? You're thinking of the toggle control's "checked" value, which does get updated before on-change fires, but this.SBER_Enabled_ doesn't, since it's bound one-way on the checkbox (check="[[SBER_Enabled_]]"). In other words, if there were no handlers, then when you click on the toggle, toggle.checked would switch back and forth, but this.SBER_Enabled_ doesn't actually change until you tell it to. And then when it does change, the toggle will conform to it's value. This is why I'm saying it's easier to just use that value as the source of truth as opposed to the checkbox value which might or might not have de-synced from the value due to weird user interaction (such as dragging not triggering event handler, etc.)
Description was changed from ========== MD Settings: For rows containing paper-toggle-buttons, make entire row clickable. BUG=680406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: For rows containing paper-toggle-buttons, make entire row clickable. BUG=680406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Closing based on off-line discussion with dbeam@ and dpapad@, we're going to go with another approach with pref plus some twist (to be implemented by dbeam). ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
