Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552393002:
MD Settings: Scroll correctly for navigations that exit search mode. (Closed)
|
Created:
4 years ago by tommycli Modified:
4 years 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: Scroll correctly for navigations that exit search mode.
Previously, exiting search mode by clicking on a section link did not
work. This was because we tried to scroll to the section before the
search mode exited.
This CL adds another attempt to scroll AFTER search mode exits.
BUG=667958
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/73b5f1096d7fb361d7e6797ede8100de39279749
Cr-Commit-Position: refs/heads/master@{#437165}
Patch Set 1 #Patch Set 2 : add test #Patch Set 3 : merge #Patch Set 4 : fix cl to make review easier #
Total comments: 2
Patch Set 5 : improve browsertest #
Total comments: 5
Patch Set 6 : update #
Messages
Total messages: 28 (19 generated)
Description was changed from ========== MD Settings: Scroll correctly for navigations that exit search mode. Previously, exiting search mode by clicking on a section link did not work. This was because we tried to scroll to the section before the search mode exited. This CL adds another attempt to scroll AFTER search mode exits. BUG=667958 ========== to ========== MD Settings: Scroll correctly for navigations that exit search mode. Previously, exiting search mode by clicking on a section link did not work. This was because we tried to scroll to the section before the search mode exited. This CL adds another attempt to scroll AFTER search mode exits. BUG=667958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tommycli@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tommycli@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 tommycli@chromium.org to run a CQ dry run
tommycli@chromium.org changed reviewers: + dpapad@chromium.org
dpapad: PTAL, thanks for the consult earlier. This CL is a result of our discussion there.
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.
https://codereview.chromium.org/2552393002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2552393002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:96: sections[i].hiddenBySearch = true; Lines 87 to 96, redo what the actual code does, seems a bit fragile and accesses private member vars. Can we invoke search properly (by calling settings-main#searchContents() and have all that done automatically?
dpapad: Thanks! See if this looks any better. https://codereview.chromium.org/2552393002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2552393002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:96: sections[i].hiddenBySearch = true; On 2016/12/07 00:57:10, dpapad wrote: > Lines 87 to 96, redo what the actual code does, seems a bit fragile and accesses > private member vars. Can we invoke search properly (by calling > settings-main#searchContents() and have all that done automatically? Done. This was so nasty I spent all day trying cleaning this up.
LGTM, with nits. Summarizing my discussion with Tommy. This CL fixes a bug, but none of us is super satisfied with the solution. The fact that in main_page_behavior.js we have to issue commands speculatively is unfortunate. We don't have a clear view on how to simplify, but at a high level, if every async operation had a clear end signal on when it is done, no speculative calls would be needed (example tryTransitionToSection_). Perhaps something we should fix in the near future, if scrolling related bugs keep happening. https://codereview.chromium.org/2552393002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2552393002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:64: // SearchManager. Otherwise, the race condition doesn't manifest. Perhaps update the comment to mention that the sections also need to be hidden for the race condition to manifest. Otherwise it looks odd that we are using a "test" SearchManager, but we are replicating part of its functionality. https://codereview.chromium.org/2552393002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:152: } If this test was to fail, it would only manifest that via a timeout. Can you add an else { reject() }, such that it actually rejects if scrolling did not happen? I think similar issue would happen on the previous test in this file. Also need to add reject in the function passed to the Promise constructor.
dpapad: Thanks! I wasn't able to address one of your comments. See below. https://codereview.chromium.org/2552393002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2552393002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:64: // SearchManager. Otherwise, the race condition doesn't manifest. On 2016/12/08 01:33:54, dpapad wrote: > Perhaps update the comment to mention that the sections also need to be hidden > for the race condition to manifest. Otherwise it looks odd that we are using a > "test" SearchManager, but we are replicating part of its functionality. Done. https://codereview.chromium.org/2552393002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:152: } On 2016/12/08 01:33:54, dpapad wrote: > If this test was to fail, it would only manifest that via a timeout. Can you add > an > else { reject() }, such that it actually rejects if scrolling did not happen? I > think similar issue would happen on the previous test in this file. > > Also need to add reject in the function passed to the Promise constructor. This test requires the setInterval because sometimes after 55ms, the scroll position has still not updated, and we need to wait for the next interval. The only failure mode is a timeout. For the same reason, the above test also needs this. We could try hooking into the inSearchMode observer or something... blah... this test should be equally flaky as the one right above it... Maybe both should be deleted :D
The CQ bit was checked by tommycli@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/2552393002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2552393002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:152: } On 2016/12/08 at 01:54:46, tommycli wrote: > On 2016/12/08 01:33:54, dpapad wrote: > > If this test was to fail, it would only manifest that via a timeout. Can you add > > an > > else { reject() }, such that it actually rejects if scrolling did not happen? I > > think similar issue would happen on the previous test in this file. > > > > Also need to add reject in the function passed to the Promise constructor. > > This test requires the setInterval because sometimes after 55ms, the scroll position has still not updated, and we need to wait for the next interval. The only failure mode is a timeout. For the same reason, the above test also needs this. > > We could try hooking into the inSearchMode observer or something... blah... this test should be equally flaky as the one right above it... Maybe both should be deleted :D Ah, I had not noticed that it calls setInterval and not setTimeout. Thanks for pointing this out. Per my original comment in the CL, this is a side-effect of the actual code being tested, making speculative calls, which trickles down to the testing code too.
The CQ bit was unchecked by tommycli@chromium.org
The CQ bit was checked by tommycli@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/2552393002/#ps100001 (title: "update")
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": 100001, "attempt_start_ts": 1481164367242290,
"parent_rev": "036635e2b61c32785dbd843158537fb69e57f9df", "commit_rev":
"f95cb894926f6b40aab96ba49c8509c0e4131e6c"}
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Scroll correctly for navigations that exit search mode. Previously, exiting search mode by clicking on a section link did not work. This was because we tried to scroll to the section before the search mode exited. This CL adds another attempt to scroll AFTER search mode exits. BUG=667958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Scroll correctly for navigations that exit search mode. Previously, exiting search mode by clicking on a section link did not work. This was because we tried to scroll to the section before the search mode exited. This CL adds another attempt to scroll AFTER search mode exits. BUG=667958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/73b5f1096d7fb361d7e6797ede8100de39279749 Cr-Commit-Position: refs/heads/master@{#437165} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/73b5f1096d7fb361d7e6797ede8100de39279749 Cr-Commit-Position: refs/heads/master@{#437165} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
