Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731403005:
MD Settings: Privacy: Show dialog when changing do-not-track (Closed)
|
Created:
3 years, 9 months ago by stevenjb Modified:
3 years, 9 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-closure_chromium.org, dbeam+watch-settings_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, srahim+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, vitalyp+closure_chromium.org, dpapad Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Privacy: Show dialog when changing do-not-track
BUG=691843
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2731403005
Cr-Commit-Position: refs/heads/master@{#457128}
Committed: https://chromium.googlesource.com/chromium/src/+/aaaf9b612ae3aa0a8d148feecd950009d9433fe5
Patch Set 1 #
Total comments: 5
Patch Set 2 : dom-if dialog and move near toggle #Patch Set 3 : . #
Total comments: 5
Patch Set 4 : Test event.path in dom-change handler #Patch Set 5 : Add learn more link #
Total comments: 14
Patch Set 6 : Fix. Learn more #
Total comments: 2
Patch Set 7 : Make dom-change handler id specific #
Total comments: 4
Patch Set 8 : Move type from @param to cast #Patch Set 9 : Rebase Use on-settings-boolean-control-change #
Messages
Total messages: 40 (13 generated)
Description was changed from ========== MD Settings: Privacy: Show dialog when changing do-not-track BUG=691843 ========== to ========== MD Settings: Privacy: Show dialog when changing do-not-track BUG=691843 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + tommycli@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:477: </dialog> please put in a dom-if, and possibly closer to the original toggle button so focus works more correctly
https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:477: </dialog> On 2017/03/07 23:08:46, Dan Beam wrote: > please put in a dom-if, and possibly closer to the original toggle button so > focus works more correctly Could you explain "focus works more correctly"? I wasn't aware that the location of the dialog was significant. (I have another similar use case that I may want to fix also).
https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:477: </dialog> On 2017/03/07 23:19:02, stevenjb wrote: > On 2017/03/07 23:08:46, Dan Beam wrote: > > please put in a dom-if, and possibly closer to the original toggle button so > > focus works more correctly > > Could you explain "focus works more correctly"? I wasn't aware that the location > of the dialog was significant. (I have another similar use case that I may want > to fix also). I just noticed a clear-browsing-data-dialog at the top of the file (outside of settings-animated-pages). Is that better, or should that be moved also?
https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:477: </dialog> On 2017/03/07 23:24:53, stevenjb wrote: > On 2017/03/07 23:19:02, stevenjb wrote: > > On 2017/03/07 23:08:46, Dan Beam wrote: > > > please put in a dom-if, and possibly closer to the original toggle button so > > > focus works more correctly > > > > Could you explain "focus works more correctly"? I wasn't aware that the > location > > of the dialog was significant. (I have another similar use case that I may > want > > to fix also). > > I just noticed a clear-browsing-data-dialog at the top of the file (outside of > settings-animated-pages). Is that better, or should that be moved also? when the dialog hides, focus is kept where it was in the DOM. we haven't yet fully audited this, but you get weird focus results the first time you press tab after dialogs close because they're not placed right by the component that triggers them. ultimately the focus stuff will have to be cleaned up globally. i'm more interested in dom-if because this is an uncommon dialog.
https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:477: </dialog> On 2017/03/08 00:00:50, Dan Beam wrote: > On 2017/03/07 23:24:53, stevenjb wrote: > > On 2017/03/07 23:19:02, stevenjb wrote: > > > On 2017/03/07 23:08:46, Dan Beam wrote: > > > > please put in a dom-if, and possibly closer to the original toggle button > so > > > > focus works more correctly > > > > > > Could you explain "focus works more correctly"? I wasn't aware that the > > location > > > of the dialog was significant. (I have another similar use case that I may > > want > > > to fix also). > > > > I just noticed a clear-browsing-data-dialog at the top of the file (outside of > > settings-animated-pages). Is that better, or should that be moved also? > > when the dialog hides, focus is kept where it was in the DOM. we haven't yet > fully audited this, but you get weird focus results the first time you press tab > after dialogs close because they're not placed right by the component that > triggers them. > > ultimately the focus stuff will have to be cleaned up globally. > > i'm more interested in dom-if because this is an uncommon dialog. Understood WRT dom-if. So, it sounds like the dialog should ideally be placed before the toggle, so that the toggle receives focus when 'tab' is pressed after closing the dialog? That seems to more-or-less work, although the focus outline after closing the dialog is a bit odd. Still, the behavior is more important, so done.
Thanks for the feedback / direction. PTAL.
https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:113: <template is="dom-if" if="[[showDoNotTrackDialog_]]"> id="do-not-track-dialog-if" https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:99: 'dom-change': 'onDomChange_', do-not-track-dialog-if.dom-change should only target the element with an ID of do-not-track-dialog
PTAL https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:113: <template is="dom-if" if="[[showDoNotTrackDialog_]]"> On 2017/03/09 02:08:33, Dan Beam wrote: > id="do-not-track-dialog-if" Acknowledged. https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:99: 'dom-change': 'onDomChange_', On 2017/03/09 02:08:33, Dan Beam wrote: > do-not-track-dialog-if.dom-change > > should only target the element with an ID of do-not-track-dialog Done.
looks pretty good except Material Design only wants to remove periods from 1 sentence (as in: if a sentence stops and THEN there's a learn more link, there should be a period in the middle) https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_chr... File chrome/app/settings_chromium_strings.grdp (left): https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_chr... chrome/app/settings_chromium_strings.grdp:57: Chromium may use web services to improve your browsing experience. You may optionally disable these services. <ph name="BEGIN_LINK"><a target="_blank" href="$1"></ph>Learn more<ph name="END_LINK"></a><ex></a></ex></ph> don't remove this . https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_goo... File chrome/app/settings_google_chrome_strings.grdp (left): https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_goo... chrome/app/settings_google_chrome_strings.grdp:57: Google Chrome may use web services to improve your browsing experience. You may optionally disable these services. <ph name="BEGIN_LINK"><a target="_blank" href="$1"></ph>Learn more<ph name="END_LINK"></a><ex></a></ex></ph> nor this one https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1604: Enabling "Do Not Track" means that a request will be included with your browsing traffic. Any effect depends on whether a website responds to the request, and how the request is interpreted. For example, some websites may respond to this request by showing you ads that aren't based on other websites you've visited. Many websites will still collect and use your browsing data - for example to improve security, to provide content, services, ads and recommendations on their websites, and to generate reporting statistics <ph name="BEGIN_LINK"><a target="_blank" href="$1"></ph>Learn more<ph name="END_LINK"></a><ex></a></ex></ph> period before Learn More link https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:130: </template> nit: this is BEFORE the control, we talked this morning about AFTER the control https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:129: var elementId = event.path[0].id; i assume you added this variable to use once because it was easier to understand than event.path[0].id? (as in: I'd probably just inline it) https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:131: if (this.showDoNotTrackDialog_) nit: combine (and maybe inline?) if (e.path[0].id == 'doNotTrackDialogIf' && this.showDoNotTrackDialog_)
https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_chr... File chrome/app/settings_chromium_strings.grdp (left): https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_chr... chrome/app/settings_chromium_strings.grdp:57: Chromium may use web services to improve your browsing experience. You may optionally disable these services. <ph name="BEGIN_LINK"><a target="_blank" href="$1"></ph>Learn more<ph name="END_LINK"></a><ex></a></ex></ph> On 2017/03/10 23:30:50, Dan Beam wrote: > don't remove this . Discussed offline (UX requested this). https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_goo... File chrome/app/settings_google_chrome_strings.grdp (left): https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_goo... chrome/app/settings_google_chrome_strings.grdp:57: Google Chrome may use web services to improve your browsing experience. You may optionally disable these services. <ph name="BEGIN_LINK"><a target="_blank" href="$1"></ph>Learn more<ph name="END_LINK"></a><ex></a></ex></ph> On 2017/03/10 23:30:50, Dan Beam wrote: > nor this one ditto https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2731403005/diff/80001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1604: Enabling "Do Not Track" means that a request will be included with your browsing traffic. Any effect depends on whether a website responds to the request, and how the request is interpreted. For example, some websites may respond to this request by showing you ads that aren't based on other websites you've visited. Many websites will still collect and use your browsing data - for example to improve security, to provide content, services, ads and recommendations on their websites, and to generate reporting statistics <ph name="BEGIN_LINK"><a target="_blank" href="$1"></ph>Learn more<ph name="END_LINK"></a><ex></a></ex></ph> On 2017/03/10 23:30:50, Dan Beam wrote: > period before Learn More link ditto https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:130: </template> On 2017/03/10 23:30:50, Dan Beam wrote: > nit: this is BEFORE the control, we talked this morning about AFTER the control I guess I was confused in the discussion, I thought I mentioned that I put the dialog before the control because I felt that was more intuitive (tab returns to the same control), sorry if I misunderstood, you think after the control is better? https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:129: var elementId = event.path[0].id; On 2017/03/10 23:30:50, Dan Beam wrote: > i assume you added this variable to use once because it was easier to understand > than event.path[0].id? (as in: I'd probably just inline it) Yes. https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:131: if (this.showDoNotTrackDialog_) On 2017/03/10 23:30:50, Dan Beam wrote: > nit: combine (and maybe inline?) > > if (e.path[0].id == 'doNotTrackDialogIf' && this.showDoNotTrackDialog_) I also think this is more clear: First check for which dom change, then do the logic. Otherwise if we were to add another dom change in an else, it would continue to do more tests. (minor preference; I can change it if you feel strongly).
not lgtm until we resolve puncutation
PTAL
looking better! https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:99: 'dom-change': 'onDomChange_', On 2017/03/09 20:06:26, stevenjb wrote: > On 2017/03/09 02:08:33, Dan Beam wrote: > > do-not-track-dialog-if.dom-change > > > > should only target the element with an ID of do-not-track-dialog > > Done. not done https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:130: </template> On 2017/03/10 23:58:20, stevenjb wrote: > On 2017/03/10 23:30:50, Dan Beam wrote: > > nit: this is BEFORE the control, we talked this morning about AFTER the > control > > I guess I was confused in the discussion, I thought I mentioned that I put the > dialog before the control because I felt that was more intuitive (tab returns to > the same control), sorry if I misunderstood, you think after the control is > better? let's just do whatever until somebody sweeps the whole UI consistently https://codereview.chromium.org/2731403005/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.js:99: 'dom-change': 'onDomChange_', listeners: { 'doNotTrackDialogIf.dom-change': 'onDomChange_', }, specifically only calls onDomChange_ when it occurs on an element with id of doNotTrackDialogIf, which is what we want example: https://jsfiddle.net/x16joap9/
PTAL https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2731403005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:130: </template> On 2017/03/13 21:57:51, Dan Beam wrote: > On 2017/03/10 23:58:20, stevenjb wrote: > > On 2017/03/10 23:30:50, Dan Beam wrote: > > > nit: this is BEFORE the control, we talked this morning about AFTER the > > control > > > > I guess I was confused in the discussion, I thought I mentioned that I put the > > dialog before the control because I felt that was more intuitive (tab returns > to > > the same control), sorry if I misunderstood, you think after the control is > > better? > > let's just do whatever until somebody sweeps the whole UI consistently Acknowledged. https://codereview.chromium.org/2731403005/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.js:99: 'dom-change': 'onDomChange_', On 2017/03/13 21:57:51, Dan Beam wrote: > listeners: { > 'doNotTrackDialogIf.dom-change': 'onDomChange_', > }, > > specifically only calls onDomChange_ when it occurs on an element with id of > doNotTrackDialogIf, which is what we want > > example: https://jsfiddle.net/x16joap9/ Ah, I was not aware we could do that and didn't parse your first comment, thanks for explaining.
lgtm https://codereview.chromium.org/2731403005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.js:136: * @param {{target: !SettingsToggleButtonElement}} event I think this type is weird
https://codereview.chromium.org/2731403005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.js:136: * @param {{target: !SettingsToggleButtonElement}} event On 2017/03/14 04:59:19, Dan Beam wrote: > I think this type is weird 'weird' is not very helpful, please elaborate.
https://codereview.chromium.org/2731403005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.js:136: * @param {{target: !SettingsToggleButtonElement}} event On 2017/03/14 18:35:56, stevenjb wrote: > On 2017/03/14 04:59:19, Dan Beam wrote: > > I think this type is weird > > 'weird' is not very helpful, please elaborate. @param {Event} event var target = /** !SettingsToggleButtonElement */(event.target); i think is more correcter
https://codereview.chromium.org/2731403005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2731403005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.js:136: * @param {{target: !SettingsToggleButtonElement}} event On 2017/03/14 18:38:21, Dan Beam wrote: > On 2017/03/14 18:35:56, stevenjb wrote: > > On 2017/03/14 04:59:19, Dan Beam wrote: > > > I think this type is weird > > > > 'weird' is not very helpful, please elaborate. > > @param {Event} event > > var target = /** !SettingsToggleButtonElement */(event.target); > > i think is more correcter Done.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2731403005/#ps140001 (title: "Move type from @param to cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/14 at 19:19:06, commit-bot wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... I am seeing a bug where the dialog only shows up if you click directly on the toggle, but if you click on the rest of the row, no dialog shows up, even if the toggle changes state.
The CQ bit was unchecked by dpapad@chromium.org
On 2017/03/14 20:34:35, dpapad wrote: > On 2017/03/14 at 19:19:06, commit-bot wrote: > > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > I am seeing a bug where the dialog only shows up if you click directly on the > toggle, but if you click on the rest of the row, no dialog shows up, even if the > toggle changes state. Hmm, this is probably affected by the same problem affecting the toggles in https://codereview.chromium.org/2746143004/, I'll test this and probably apply the same fix.
PTAL
The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm
On 2017/03/14 23:28:49, Dan Beam wrote: > still lgtm rslgtm. I don't have any objections if dan's reviewed it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489599371225840,
"parent_rev": "c6d234a7184e24e2def0900efaa4c1ba18069165", "commit_rev":
"aaaf9b612ae3aa0a8d148feecd950009d9433fe5"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Privacy: Show dialog when changing do-not-track BUG=691843 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Privacy: Show dialog when changing do-not-track BUG=691843 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2731403005 Cr-Commit-Position: refs/heads/master@{#457128} Committed: https://chromium.googlesource.com/chromium/src/+/aaaf9b612ae3aa0a8d148feecd95... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/aaaf9b612ae3aa0a8d148feecd95... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
