Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2512323002:
MD Settings: In /syncSetup clicking links in checkbox label should not check/uncheck. (Closed)
|
Created:
4 years, 1 month ago by scottchen Modified:
4 years ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: In /syncSetup clicking links in checkbox label should not change checkbox value.
There was a bug in which the clicks on anchor tags within the checkbox label will unintentionally alter the checkbox value. This patch fixes this issue by adding a listener with event.stopPropagate.
BUG=655069
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/37cd8d80895f447c7b34da5178d5ac71edca9656
Cr-Commit-Position: refs/heads/master@{#433629}
Patch Set 1 #Patch Set 2 : remove outdated comment #
Total comments: 8
Patch Set 3 : update styling per cr comments #
Total comments: 2
Patch Set 4 : add @private annotation to sync page function #
Messages
Total messages: 25 (16 generated)
Description was changed from ========== clicking on links within checkbox label should not change checkbox value BUG=655069 ========== to ========== clicking on links within checkbox label should not change checkbox value BUG=655069 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@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...
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
scottchen@chromium.org changed reviewers: + dbeam@chromium.org
Description was changed from ========== clicking on links within checkbox label should not change checkbox value BUG=655069 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: In /syncSetup clicking links in checkbox label should not check/uncheck. There was a bug in which the clicks on anchor tags within the checkbox label will unintentionally alter the checkbox value. This patch fixes this issue by adding a listener with event.stopPropagate. BUG=655069 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: In /syncSetup clicking links in checkbox label should not check/uncheck. There was a bug in which the clicks on anchor tags within the checkbox label will unintentionally alter the checkbox value. This patch fixes this issue by adding a listener with event.stopPropagate. BUG=655069 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: In /syncSetup clicking links in checkbox label should not change checkbox value. There was a bug in which the clicks on anchor tags within the checkbox label will unintentionally alter the checkbox value. This patch fixes this issue by adding a listener with event.stopPropagate. BUG=655069 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2512323002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2512323002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:145: id="paymentLearnMore" on-tap="stopPropagation_"> Nit: on-tap="onLearnMoreTap_" https://codereview.chromium.org/2512323002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2512323002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:429: stopPropagation_: function(event) { Nit: onLearnMoreTap_: function() { // Move comment here instead. // Stop the propagation of events ... ... } This naming is more consistent with listeners across the codebase. https://codereview.chromium.org/2512323002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/people_page_sync_page_test.js (right): https://codereview.chromium.org/2512323002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/people_page_sync_page_test.js:250: var link = syncPage.$.paymentLearnMore; Probably worth ensuring that the parentElement/parentNode is what you expect it to be. assertEquals('PAPER-CHECKBOX', link.parentNode.nodeName); https://codereview.chromium.org/2512323002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/people_page_sync_page_test.js:256: // The checkbox value should be unchanged after clicking on the link Comments should have proper punctuation per style guide.
https://codereview.chromium.org/2512323002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.html (right): https://codereview.chromium.org/2512323002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.html:145: id="paymentLearnMore" on-tap="stopPropagation_"> On 2016/11/19 02:04:38, dpapad wrote: > Nit: on-tap="onLearnMoreTap_" Done. https://codereview.chromium.org/2512323002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2512323002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:429: stopPropagation_: function(event) { On 2016/11/19 02:04:38, dpapad wrote: > Nit: > onLearnMoreTap_: function() { > // Move comment here instead. > // Stop the propagation of events ... > ... > } > > This naming is more consistent with listeners across the codebase. Done. https://codereview.chromium.org/2512323002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/people_page_sync_page_test.js (right): https://codereview.chromium.org/2512323002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/people_page_sync_page_test.js:250: var link = syncPage.$.paymentLearnMore; On 2016/11/19 02:04:38, dpapad wrote: > Probably worth ensuring that the parentElement/parentNode is what you expect it > to be. > > assertEquals('PAPER-CHECKBOX', link.parentNode.nodeName); Done. https://codereview.chromium.org/2512323002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/people_page_sync_page_test.js:256: // The checkbox value should be unchanged after clicking on the link On 2016/11/19 02:04:38, dpapad wrote: > Comments should have proper punctuation per style guide. Done.
lgtm https://codereview.chromium.org/2512323002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2512323002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:425: * @param {!Event} event @private
https://codereview.chromium.org/2512323002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2512323002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:425: * @param {!Event} event On 2016/11/21 18:53:29, dpapad wrote: > @private Done.
The CQ bit was checked by scottchen@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...
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 scottchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2512323002/#ps60001 (title: "add @private annotation to sync page function")
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": 60001, "attempt_start_ts": 1479759962183520,
"parent_rev": "0396ea72fac03c1ee0706917198c60130df3feeb", "commit_rev":
"44e8efa934df99b98b5d1e0c2627ea72c7926376"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: In /syncSetup clicking links in checkbox label should not change checkbox value. There was a bug in which the clicks on anchor tags within the checkbox label will unintentionally alter the checkbox value. This patch fixes this issue by adding a listener with event.stopPropagate. BUG=655069 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: In /syncSetup clicking links in checkbox label should not change checkbox value. There was a bug in which the clicks on anchor tags within the checkbox label will unintentionally alter the checkbox value. This patch fixes this issue by adding a listener with event.stopPropagate. BUG=655069 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: In /syncSetup clicking links in checkbox label should not change checkbox value. There was a bug in which the clicks on anchor tags within the checkbox label will unintentionally alter the checkbox value. This patch fixes this issue by adding a listener with event.stopPropagate. BUG=655069 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: In /syncSetup clicking links in checkbox label should not change checkbox value. There was a bug in which the clicks on anchor tags within the checkbox label will unintentionally alter the checkbox value. This patch fixes this issue by adding a listener with event.stopPropagate. BUG=655069 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/37cd8d80895f447c7b34da5178d5ac71edca9656 Cr-Commit-Position: refs/heads/master@{#433629} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/37cd8d80895f447c7b34da5178d5ac71edca9656 Cr-Commit-Position: refs/heads/master@{#433629} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
