Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2039603002:
MD Settings: Fix scroll to section (Closed)
|
Created:
4 years, 6 months ago by michaelpg Modified:
4 years, 5 months 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Fix scroll to section
Scrolling to sections broke in crrev.com/2018753002 with errors about
host being undefined. The host is just "this", anyway.
BUG=569761
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Patch Set 1 #Patch Set 2 : test coverage #
Total comments: 9
Messages
Total messages: 19 (6 generated)
Description was changed from ========== MD Settings: Fix scroll to section Scrolling to sections broke in crrev.com/2018753002 with errors about host being undefined. The host is just "this", anyway. BUG=569761 R=dpapad@chromium.org ========== to ========== MD Settings: Fix scroll to section Scrolling to sections broke in crrev.com/2018753002 with errors about host being undefined. The host is just "this", anyway. BUG=569761 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
PTAL.
LGTM, with some questions. Should probably CC @dschuyler, since he is more familiar with navigation. https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/navigation_browsertest.js (right): https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/navigation_browsertest.js:20: 'navigation_tests.js', Can we not put this test under one of our existing _browsertest.js files? https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/navigation_tests.js (right): https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/navigation_tests.js:30: assertLT(0, basicPage.scroller.scrollTop); I am guessing that this is necessary because the code being tested also uses setTimeout? Is there a way we could avoid this (either in the test or both in the test and prod code)?
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
sure does look like this conflicts with: https://codereview.chromium.org/2022893003/
On 2016/06/03 23:02:20, Dan Beam wrote: > sure does look like this conflicts with: > https://codereview.chromium.org/2022893003/ thanks, didn't see that and didn't get a response from dschuyler (since he's out today). that said... this solution is simpler. (the tests are more complex, but see response to dpapad)
https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/navigation_browsertest.js (right): https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/navigation_browsertest.js:20: 'navigation_tests.js', On 2016/06/03 22:59:50, dpapad wrote: > Can we not put this test under one of our existing _browsertest.js files? Not really, and there's plenty of precedent for this: https://cs.chromium.org/search/?q=settingspagebrowsertest%5C.prototype&sq=pac... https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/navigation_tests.js (right): https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/navigation_tests.js:30: assertLT(0, basicPage.scroller.scrollTop); On 2016/06/03 22:59:50, dpapad wrote: > I am guessing that this is necessary because the code being tested also uses > setTimeout? Is there a way we could avoid this (either in the test or both in > the test and prod code)? not without messing with the prod code, which already has a TODO for this
dschuyler@chromium.org changed reviewers: + dschuyler@chromium.org
https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/navigation_tests.js (right): https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/navigation_tests.js:15: menu = document.querySelector('* /deep/ settings-menu'); Can we avoid this use of /deep/?
https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/navigation_tests.js (right): https://codereview.chromium.org/2039603002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/navigation_tests.js:15: menu = document.querySelector('* /deep/ settings-menu'); On 2016/06/06 21:57:48, dschuyler wrote: > Can we avoid this use of /deep/? there's still no consensus on the deprecation of /deep/ in querySelector[All], mainly because of its usefulness in tests. imo this is a fine way to grab the element we want to test when doing full-page browser tests.
It looks like the unit test here may superseed the unit test in basic_page_browsertest.js that recently landed. If they are covering the same ground, we can remove the test in basic_page_browsertest.js (or either test, as you wish). https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/main_page_behavior.js:383: } else if (newRoute && newRoute.section && !oldRouteIsSubpage) { What is this addition addressing?
On 2016/06/06 23:51:43, dschuyler wrote: > It looks like the unit test here may superseed > the unit test in basic_page_browsertest.js that > recently landed. If they are covering the same > ground, we can remove the test in > basic_page_browsertest.js (or either test, as > you wish). > > https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_page/main_page_behavior.js > (right): > > https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/main_page_behavior.js:383: } > else if (newRoute && newRoute.section && !oldRouteIsSubpage) { > What is this addition addressing? s/superseed/supersede/
https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/main_page_behavior.js:383: } else if (newRoute && newRoute.section && !oldRouteIsSubpage) { On 2016/06/06 at 23:51:42, dschuyler wrote: > What is this addition addressing? Warning: wall of text that I'm publishing for my notes, probably not worth reading because implementing the very last line may nullify everything else. here, oldRouteIsSubpage implies newRouteIsSubpage which implies newRoute.section == oldRoute.section so: old route and new route are both subpages within the same section, so it does NOT make sense to change the scroll position (ie, we shouldn't[1] change the scroll position of the page from whatever it was before entering the first subpage). [1] it doesn't actually, because of implementation details elsewhere, so this is just preventative (and a very minor optimization) rather than a bug fix. Equivalently, we shouldn't scroll to the section when showing a subpage (!oldRouteIsSubpage also implies !newRouteIsSubpage due to 378-379). We may want to add this behavior, but we should do it at line 380 where it belongs. However, there's another fix we need: 377 should also scroll to the section IF we got here by clicking a menu entry, or a link to some other section, but NOT if we used the back button or just hit the back arrow in the subpage... crap.
On 2016/06/06 at 23:51:43, dschuyler wrote: > It looks like the unit test here may superseed > the unit test in basic_page_browsertest.js that > recently landed. If they are covering the same > ground, we can remove the test in > basic_page_browsertest.js (or either test, as > you wish). that's fine, my thought was the menu itself should have a browser test rather than this being in the basic (or advanced) page test. we can revisit when we continue adjusting menu behavior > > https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): > > https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/main_page_behavior.js:383: } else if (newRoute && newRoute.section && !oldRouteIsSubpage) { > What is this addition addressing?
LGTM with a new bug entry and a comment (see comments). If ya decide to rework the logic in the current route handler, I'd like to re-review that. (Either way is cool with me). https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/main_page_behavior.js:383: } else if (newRoute && newRoute.section && !oldRouteIsSubpage) { On 2016/06/07 02:55:05, michaelpg wrote: > On 2016/06/06 at 23:51:42, dschuyler wrote: > > What is this addition addressing? > > Warning: wall of text that I'm publishing for my notes, probably not worth > reading because implementing the very last line may nullify everything else. > > here, oldRouteIsSubpage implies newRouteIsSubpage which implies newRoute.section > == oldRoute.section > > so: old route and new route are both subpages within the same section, so it > does NOT make sense to change the scroll position (ie, we shouldn't[1] change > the scroll position of the page from whatever it was before entering the first > subpage). > > [1] it doesn't actually, because of implementation details elsewhere, so this is > just preventative (and a very minor optimization) rather than a bug fix. > > Equivalently, we shouldn't scroll to the section when showing a subpage > (!oldRouteIsSubpage also implies !newRouteIsSubpage due to 378-379). We may want > to add this behavior, but we should do it at line 380 where it belongs. > > However, there's another fix we need: 377 should also scroll to the section IF > we got here by clicking a menu entry, or a link to some other section, but NOT > if we used the back button or just hit the back arrow in the subpage... crap. That's a lot of indirect logic for folks following along at home, please consider one of the following: - rework the logic flow here (lines 374 to 385) to make it easier to follow - or add a bug with your notes above and comment around line 374 that this logic should be reworked, referencing the new bug.
On 2016/06/08 00:35:06, dschuyler wrote: > LGTM with a new bug entry and a comment > (see comments). > > If ya decide to rework the logic in the > current route handler, I'd like to re-review > that. > > (Either way is cool with me). > > https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_page/main_page_behavior.js > (right): > > https://codereview.chromium.org/2039603002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/main_page_behavior.js:383: } > else if (newRoute && newRoute.section && !oldRouteIsSubpage) { > On 2016/06/07 02:55:05, michaelpg wrote: > > On 2016/06/06 at 23:51:42, dschuyler wrote: > > > What is this addition addressing? > > > > Warning: wall of text that I'm publishing for my notes, probably not worth > > reading because implementing the very last line may nullify everything else. > > > > here, oldRouteIsSubpage implies newRouteIsSubpage which implies > newRoute.section > > == oldRoute.section > > > > so: old route and new route are both subpages within the same section, so it > > does NOT make sense to change the scroll position (ie, we shouldn't[1] change > > the scroll position of the page from whatever it was before entering the first > > subpage). > > > > [1] it doesn't actually, because of implementation details elsewhere, so this > is > > just preventative (and a very minor optimization) rather than a bug fix. > > > > Equivalently, we shouldn't scroll to the section when showing a subpage > > (!oldRouteIsSubpage also implies !newRouteIsSubpage due to 378-379). We may > want > > to add this behavior, but we should do it at line 380 where it belongs. > > > > However, there's another fix we need: 377 should also scroll to the section IF > > we got here by clicking a menu entry, or a link to some other section, but NOT > > if we used the back button or just hit the back arrow in the subpage... crap. > > That's a lot of indirect logic for folks following along at home, please > consider one of the following: > - rework the logic flow here (lines 374 to 385) to make it easier to follow > - or add a bug with your notes above and comment around line 374 that this logic > should be reworked, referencing the new bug. I told you not to read it ;-) We'll revisit this issue one way or another. I don't see a need to land this CL as is because yours already went in.
Description was changed from ========== MD Settings: Fix scroll to section Scrolling to sections broke in crrev.com/2018753002 with errors about host being undefined. The host is just "this", anyway. BUG=569761 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix scroll to section Scrolling to sections broke in crrev.com/2018753002 with errors about host being undefined. The host is just "this", anyway. BUG=569761 R=dpapad@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
dpapad@chromium.org changed reviewers: - dpapad@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
