Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2231153002:
MD Settings: Checks the event target in the language tap event handler (Closed)
|
Created:
4 years, 4 months ago by Moe Modified:
4 years, 4 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Checks the event target in the language tap event handler
BUG=636223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b03be7848c607d001520cc6f721a793ff630cc3f
Cr-Commit-Position: refs/heads/master@{#411716}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comment, added test #
Total comments: 6
Patch Set 3 : Addressed comments #
Total comments: 3
Patch Set 4 : Addressed comments #
Total comments: 4
Patch Set 5 : Addressed comments #Patch Set 6 : updated comment #
Messages
Total messages: 52 (38 generated)
Description was changed from ========== MD Settings: Checks the event target in the language tap event handler BUG=636223 ========== to ========== MD Settings: Checks the event target in the language tap event handler BUG=636223 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run
mahmadi@chromium.org changed reviewers: + michaelpg@chromium.org
Hi Michael, Please take a look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
can you update the test to tap the dropdown-trigger first, and check that setUILanguage doesn't get called? maybe here: https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/language... https://codereview.chromium.org/2231153002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2231153002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/languages_page.js:64: if (Polymer.dom(e).localTarget.classList.toString().includes('dropdown-')) className for a string?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mahmadi@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...
Added a test, but I don't think it's exactly what we're looking for. This test only makes sense for cros and windows. And we should probably be selecting '.list-button:first-of-type .dropdown-trigger'. https://codereview.chromium.org/2231153002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2231153002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/languages_page.js:64: if (Polymer.dom(e).localTarget.classList.toString().includes('dropdown-')) On 2016/08/10 18:24:16, michaelpg wrote: > className for a string? Done.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:82: active="{{item.optionsMenuOpened}}"> can't we just make an on-tap=methodThatCallsEventStopPropagation? https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.js:64: if (Polymer.dom(e).localTarget.className.includes('dropdown-')) why are you just checking that dropdown- exists in the class name? this doesn't seem right. my-dropdown-thing would match this. can we just use something other than class here to do the filtering?
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 mahmadi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:82: active="{{item.optionsMenuOpened}}"> On 2016/08/10 20:45:26, Dan Beam wrote: > can't we just make an on-tap=methodThatCallsEventStopPropagation? the event could originate from either of dropdown-(trigger|content|item) depending on where the user taps. We would need three on-tap handlers. https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.js:64: if (Polymer.dom(e).localTarget.className.includes('dropdown-')) On 2016/08/10 20:45:26, Dan Beam wrote: > why are you just checking that dropdown- exists in the class name? this doesn't > seem right. my-dropdown-thing would match this. > > can we just use something other than class here to do the filtering? Made this a regex. It should be a more precise matching now. what do you suggest using instead of class names?
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/2231153002/diff/10003/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2231153002/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:65: <div class="start" title="[[item.language.nativeDisplayName]]"> can we just move the on-tap to here? https://codereview.chromium.org/2231153002/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:78: </div> i don't reallly care that much if tapping/clicking the checkbox works. if you reallly care, you can wrap with a <div on-tap> around both .middle and .title
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 mahmadi@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 checked by mahmadi@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...
Patchset #4 (id:50001) has been deleted
https://codereview.chromium.org/2231153002/diff/10003/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2231153002/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:78: </div> On 2016/08/11 00:09:51, Dan Beam wrote: > i don't reallly care that much if tapping/clicking the checkbox works. if you > reallly care, you can wrap with a <div on-tap> around both .middle and .title Let's have a wrapper around title and .middle. Otherwise the language list item looks entirely actionable but clicks are handled on only a small part of it. I think that's a bad experience.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:70001) has been deleted
The CQ bit was checked by mahmadi@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.
i'd like our solution to: not add any extra wrapper elements keep basically the same functionality and presentation if possible if we move the actionable style to only the start/middle <div>s, we lose the hand cursor for much of the space so I think the best idea is still to try to stop the propagation from places where possible. we could go back to referring to class names if realllly necessary, I just don't love it https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:82: active="{{item.optionsMenuOpened}}"> On 2016/08/10 23:58:32, moe wrote: > On 2016/08/10 20:45:26, Dan Beam wrote: > > can't we just make an on-tap=methodThatCallsEventStopPropagation? > > the event could originate from either of dropdown-(trigger|content|item) > depending on where the user taps. We would need three on-tap handlers. could you put the <iron-dropdown> in the <paper-icon-button> to fix this? https://codereview.chromium.org/2231153002/diff/90001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2231153002/diff/90001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:64: <div class$="list-item [[getLanguageItemClass_( why are these both .list-item https://codereview.chromium.org/2231153002/diff/90001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:67: <div class="list-item language-title-wrapper" why is this a list-item?
Patchset #5 (id:110001) has been deleted
The CQ bit was checked by mahmadi@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 checked by mahmadi@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: Exceeded global retry quota
Patchset #5 (id:130001) has been deleted
Hopefully third time's the charm. https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2231153002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:82: active="{{item.optionsMenuOpened}}"> On 2016/08/12 01:43:43, Dan Beam wrote: > On 2016/08/10 23:58:32, moe wrote: > > On 2016/08/10 20:45:26, Dan Beam wrote: > > > can't we just make an on-tap=methodThatCallsEventStopPropagation? > > > > the event could originate from either of dropdown-(trigger|content|item) > > depending on where the user taps. We would need three on-tap handlers. > > could you put the <iron-dropdown> in the <paper-icon-button> to fix this? I tried, but couldn't get the dropdown to show :( https://codereview.chromium.org/2231153002/diff/90001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.html (right): https://codereview.chromium.org/2231153002/diff/90001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:64: <div class$="list-item [[getLanguageItemClass_( On 2016/08/12 01:43:43, Dan Beam wrote: > why are these both .list-item again to maintain appearance. https://codereview.chromium.org/2231153002/diff/90001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.html:67: <div class="list-item language-title-wrapper" On 2016/08/12 01:43:43, Dan Beam wrote: > why is this a list-item? to maintain the appearance. '.list-item' gives the wrapper div all the good flex stuff.
if this works, awesome lgtm
On 2016/08/12 16:48:06, Dan Beam wrote: > if this works, awesome > > lgtm yep it works. Thanks!
The CQ bit was checked by mahmadi@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/2231153002/#ps170001 (title: "updated comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Checks the event target in the language tap event handler BUG=636223 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Checks the event target in the language tap event handler BUG=636223 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b03be7848c607d001520cc6f721a793ff630cc3f Cr-Commit-Position: refs/heads/master@{#411716} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b03be7848c607d001520cc6f721a793ff630cc3f Cr-Commit-Position: refs/heads/master@{#411716} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
