Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2504483002:
MD Settings: Fix popstate behavior from section route to section route. (Closed)
|
Created:
4 years, 1 month ago by tommycli Modified:
4 years, 1 month ago Reviewers:
Dan Beam 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Fix popstate behavior from section route to section route.
Fixes this case:
1. Navigate to a section using the left menu.
2. Navigate to a different section using the left menu.
3. Press Back.
4. Observe nothing happens.
BUG=664070
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7f225c70775269836739cbd735f7d84919b505d2
Cr-Commit-Position: refs/heads/master@{#432298}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #Patch Set 3 : fix closure compile #
Dependent Patchsets: Messages
Total messages: 19 (13 generated)
Description was changed from ========== MD Settings: Fix popstate behavior from section route to section route. Fixes this case: 1. Navigate to a section using the left menu. 2. Navigate to a different section using the left menu. 3. Press Back. 4. Observe nothing happens. BUG=664070 ========== to ========== MD Settings: Fix popstate behavior from section route to section route. Fixes this case: 1. Navigate to a section using the left menu. 2. Navigate to a different section using the left menu. 3. Press Back. 4. Observe nothing happens. BUG=664070 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tommycli@chromium.org changed reviewers: + dbeam@chromium.org
dbeam: PTAL - this fixes the popstates that occur from one section to another. i.e. from /appearance => /people. This does not fix the back-to-root-route case of /appearance => /. That will require a separate fix, since currently don't do any scrolling for the root route at all.
lgtm worth writing a test? https://codereview.chromium.org/2504483002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2504483002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:29: // any popstate navigations that are in-page. this is kinda confusing. "Scroll to the section except for back/foward." or "ignoring popstate" maybe? https://codereview.chromium.org/2504483002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:33: !settings.lastRouteChangeWasPopstate() || oldRouteWasSection; nit: 2 more spaces
thanks! The test strikes me as too hard to write to be worth it because: 1. Scroll happens at some setTimeout later, so we would probably need to add an event and listener. 2. Testing that an element is scrolled to the top isn't super well defined. Maybe we would test the container.scrollTop against the section.offsetTop with some margin and tolerance? Just doesn't seem worth to me. https://codereview.chromium.org/2504483002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2504483002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:29: // any popstate navigations that are in-page. On 2016/11/15 01:24:22, Dan Beam wrote: > this is kinda confusing. "Scroll to the section except for back/foward." or > "ignoring popstate" maybe? Done. https://codereview.chromium.org/2504483002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:33: !settings.lastRouteChangeWasPopstate() || oldRouteWasSection; On 2016/11/15 01:24:21, Dan Beam wrote: > nit: 2 more spaces Done.
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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
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: This issue passed the CQ dry run.
The CQ bit was checked by tommycli@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/2504483002/#ps40001 (title: "fix closure compile")
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Fix popstate behavior from section route to section route. Fixes this case: 1. Navigate to a section using the left menu. 2. Navigate to a different section using the left menu. 3. Press Back. 4. Observe nothing happens. BUG=664070 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix popstate behavior from section route to section route. Fixes this case: 1. Navigate to a section using the left menu. 2. Navigate to a different section using the left menu. 3. Press Back. 4. Observe nothing happens. BUG=664070 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7f225c70775269836739cbd735f7d84919b505d2 Cr-Commit-Position: refs/heads/master@{#432298} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7f225c70775269836739cbd735f7d84919b505d2 Cr-Commit-Position: refs/heads/master@{#432298} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
