Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681143004:
MD Settings: in autofill section, change google-payment related entries to outlinks instead. (Closed)
|
Created:
3 years, 10 months ago by scottchen Modified:
3 years, 10 months ago Reviewers:
dpapad 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 autofill section, change google-payment related entries to outlinks instead.
If an entry in the address or credit card autofill settings is connected to Google Payment, this CL makes it use a outlink-icon button that directly links to the google payment page, as opposed to using a dropdown that conditionally decides what to do based on the entry type.
BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2681143004
Cr-Commit-Position: refs/heads/master@{#450123}
Committed: https://chromium.googlesource.com/chromium/src/+/8cc82777e38c6e39cfcbbdfccce6216a222251e1
Patch Set 1 #Patch Set 2 : Add tests for button display conditions in autofill #
Total comments: 6
Patch Set 3 : use dom-if to alternate buttons #
Total comments: 13
Patch Set 4 : fixes based on comments #Patch Set 5 : fix unit tests #
Messages
Total messages: 23 (14 generated)
Description was changed from ========== MD Settings: in autofill section, change google-payment related entries to outlinks instead. If an entry in the address or credit card autofill settings is connected to Google Payment, this CL makes it use a outlink-icon button that directly links to the google payment page, as opposed to using a dropdown that conditionally decides what to do based on the entry type. BUG=684152 ========== to ========== MD Settings: in autofill section, change google-payment related entries to outlinks instead. If an entry in the address or credit card autofill settings is connected to Google Payment, this CL makes it use a outlink-icon button that directly links to the google payment page, as opposed to using a dropdown that conditionally decides what to do based on the entry type. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: in autofill section, change google-payment related entries to outlinks instead. If an entry in the address or credit card autofill settings is connected to Google Payment, this CL makes it use a outlink-icon button that directly links to the google payment page, as opposed to using a dropdown that conditionally decides what to do based on the entry type. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: in autofill section, change google-payment related entries to outlinks instead. If an entry in the address or credit card autofill settings is connected to Google Payment, this CL makes it use a outlink-icon button that directly links to the google payment page, as opposed to using a dropdown that conditionally decides what to do based on the entry type. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
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...
https://codereview.chromium.org/2681143004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html (right): https://codereview.chromium.org/2681143004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:84: <button is="paper-icon-button-light" class="icon-external" Instead of adding two mutually exclusive buttons (one of the two is always hidden), can we just have one button that is customized based on |item|? <button is="paper-icon-button-light" class="[[getIconCssClass_(item)]]" on-tap="onButtonTap_"></button> ... onButtonTap_: function(e) { // you can refer to |e.model.item.metadata.isLocal| and act accordingly.. } https://codereview.chromium.org/2681143004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:85: on-tap="onRemoteEditAddressTap_" actionable Do <button>s need the actionable class? Don't they already show the correct cursor by default? https://codereview.chromium.org/2681143004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:139: <paper-icon-button id="creditCardMenu" icon="cr:more-vert" Same question here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2681143004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html (right): https://codereview.chromium.org/2681143004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:85: on-tap="onRemoteEditAddressTap_" actionable On 2017/02/09 01:30:41, dpapad wrote: > Do <button>s need the actionable class? Don't they already show the correct > cursor by default? paper-icon-button-light has a "display:none" unless parent or self is actionable. I think this is one of those things that was done to hide the icon button if parent is "disabled" (as in, actionable=false), which is one of the slightly confusing things I mentioned in the actionable-disabled debate. In any case, in order to make it visible, it has to have the "actionable" attribute.
Oops, sorry left out comments. They're Addressed by new patch. https://codereview.chromium.org/2681143004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html (right): https://codereview.chromium.org/2681143004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:84: <button is="paper-icon-button-light" class="icon-external" On 2017/02/09 01:30:41, dpapad wrote: > Instead of adding two mutually exclusive buttons (one of the two is always > hidden), can we just have one button that is customized based on |item|? > > <button is="paper-icon-button-light" class="[[getIconCssClass_(item)]]" > on-tap="onButtonTap_"></button> > > ... > onButtonTap_: function(e) { > // you can refer to |e.model.item.metadata.isLocal| and act accordingly.. > } Done. https://codereview.chromium.org/2681143004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:139: <paper-icon-button id="creditCardMenu" icon="cr:more-vert" On 2017/02/09 01:30:41, dpapad wrote: > Same question here. Done.
https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html (right): https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:84: <button is="paper-icon-button-light" class="icon-external" Indentation off. https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:85: on-tap="onRemoteEditAddressTap_" actionable></button> Is there a point of applying actionable to a <button>? Isn't the cursor already the correct cursor? (same question for the other <button> in this CL). https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:136: </div> I noticed in the screenshot that the "Expiration Date" header is not aligned with the expiration date anymore. Is it because of this div? I don't see the misalignment in ToT. The creditCardMenu and creditCardExpiration used to be siblings but they are not anymore. https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:107: onRemoteEditAddressTap_: function() { Does this need a preventDefault() call also? https://codereview.chromium.org/2681143004/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2681143004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:230: assertTrue(outlinkButton.hidden); Since the paper-icon-button-light is in a dom-if it should not be even found in the DOM. How come it exists here? https://codereview.chromium.org/2681143004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:240: var menuButton = row.querySelector('#creditCardMenu'); Same question here.
https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html (right): https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:84: <button is="paper-icon-button-light" class="icon-external" On 2017/02/10 20:24:08, dpapad wrote: > Indentation off. Done. https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:85: on-tap="onRemoteEditAddressTap_" actionable></button> On 2017/02/10 20:24:08, dpapad wrote: > Is there a point of applying actionable to a <button>? Isn't the cursor already > the correct cursor? (same question for the other <button> in this CL). This was addressed in the reply to your comment from the last patch: "paper-icon-button-light has a "display:none" unless parent or self is actionable. I think this is one of those things that was done to hide the icon button if parent is "disabled" (as in, actionable=false), which is one of the slightly confusing things I mentioned in the actionable-disabled debate. In any case, in order to make it visible, it has to have the "actionable" attribute." https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:136: </div> On 2017/02/10 20:24:08, dpapad wrote: > I noticed in the screenshot that the "Expiration Date" header is not aligned > with the expiration date anymore. Is it because of this div? I don't see the > misalignment in ToT. > > The creditCardMenu and creditCardExpiration used to be siblings but they are not > anymore. Oops, fixed in newest patch: http://imgur.com/a/T4gWq https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:107: onRemoteEditAddressTap_: function() { On 2017/02/10 20:24:08, dpapad wrote: > Does this need a preventDefault() call also? I don't think so. The menu items handlers has it because there's something under the menu that might get mistakenly focused (https://chromium.googlesource.com/chromium/src/+/ddbf37a7628ba691536fd980951f...), but in this case the target button is not going anywhere, so we don't need to worry about the touch focusing onto something else. https://codereview.chromium.org/2681143004/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2681143004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:230: assertTrue(outlinkButton.hidden); On 2017/02/10 20:24:08, dpapad wrote: > Since the paper-icon-button-light is in a dom-if it should not be even found in > the DOM. How come it exists here? ..I forgot to update the test after changing it to dom-if, thanks for catching. https://codereview.chromium.org/2681143004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:240: var menuButton = row.querySelector('#creditCardMenu'); On 2017/02/10 20:24:08, dpapad wrote: > Same question here. same answer as above ;)
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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
LGTM https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html (right): https://codereview.chromium.org/2681143004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:85: on-tap="onRemoteEditAddressTap_" actionable></button> On 2017/02/10 at 23:48:02, scottchen wrote: > On 2017/02/10 20:24:08, dpapad wrote: > > Is there a point of applying actionable to a <button>? Isn't the cursor already > > the correct cursor? (same question for the other <button> in this CL). > > This was addressed in the reply to your comment from the last patch: > > "paper-icon-button-light has a "display:none" unless parent or self is > actionable. I think this is one of those things that was done to hide the icon > button if parent is "disabled" (as in, actionable=false), which is one of the > slightly confusing things I mentioned in the actionable-disabled debate. > > In any case, in order to make it visible, it has to have the "actionable" > attribute." I see (must have missed your previous response). Thanks for the explanation. I agree that this is confusing.
The CQ bit was checked by scottchen@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": 80001, "attempt_start_ts": 1487016394720810,
"parent_rev": "4c66d9c05a0e1ad7d4f32d7ebacb5d49ef2573e3", "commit_rev":
"8cc82777e38c6e39cfcbbdfccce6216a222251e1"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: in autofill section, change google-payment related entries to outlinks instead. If an entry in the address or credit card autofill settings is connected to Google Payment, this CL makes it use a outlink-icon button that directly links to the google payment page, as opposed to using a dropdown that conditionally decides what to do based on the entry type. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: in autofill section, change google-payment related entries to outlinks instead. If an entry in the address or credit card autofill settings is connected to Google Payment, this CL makes it use a outlink-icon button that directly links to the google payment page, as opposed to using a dropdown that conditionally decides what to do based on the entry type. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2681143004 Cr-Commit-Position: refs/heads/master@{#450123} Committed: https://chromium.googlesource.com/chromium/src/+/8cc82777e38c6e39cfcbbdfccce6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8cc82777e38c6e39cfcbbdfccce6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
