Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259163002:
MD Settings: Fix scrolling bugs and hacks caused by undefined load behavior (Closed)
|
Created:
4 years, 4 months ago by michaelpg Modified:
4 years, 2 months ago Reviewers:
dschuyler 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@TransitionsNoTestsFakeRebase Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Fix scrolling bugs and hacks caused by undefined load behavior
Imports in <body> cause undefined behavior: crbug.com/638074
We have a bunch of hacks to try to avoid doing things with an invalid DOM.
Instead of these hacks, sidestep the problem by moving the i18n import to
<head> (yeah, this probably breaks i18n-foo in <body>).
The code is less haphazard now, but still a minefield because:
* Basic and Advanced attempt to control the scroller simultaneously
* settings-main updates the overscroll with no regard for section transitions
* settings-main eagerly removes overscroll on "scroll" events, but can't
differentiate between user scrolls and Basic/Advanced-initiated scrolls
BUG=637508, 537359, 589681
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6dabb5a5dcefa2d2a775bd5abdf64f87e8aa2621
Cr-Commit-Position: refs/heads/master@{#422625}
Patch Set 1 #Patch Set 2 : bug fixes #
Total comments: 3
Patch Set 3 : Update #Patch Set 4 : rebase on pending overscroll CL #Patch Set 5 : Switching pages #Patch Set 6 : rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 21 (11 generated)
Description was changed from ========== MD Settings: Fix scrolling bugs and hacks caused by undefined load behavior Imports in <body> cause undefined behavior: crbug.com/638074 We have a bunch of hacks to try to avoid doing things with an invalid DOM. Instead of these hacks, sidestep the problem by moving the i18n import to <head> (yeah, this probably breaks i18n-foo in <body>). The code is less haphazard now, but still a minefield because: * Basic and Advanced attempt to control the scroller simultaneously * settings-main updates the overscroll with no regard for section transitions * settings-main eagerly removes overscroll on "scroll" events, but can't differentiate between user scrolls and Basic/Advanced-initiated scrolls BUG= ========== to ========== MD Settings: Fix scrolling bugs and hacks caused by undefined load behavior Imports in <body> cause undefined behavior: crbug.com/638074 We have a bunch of hacks to try to avoid doing things with an invalid DOM. Instead of these hacks, sidestep the problem by moving the i18n import to <head> (yeah, this probably breaks i18n-foo in <body>). The code is less haphazard now, but still a minefield because: * Basic and Advanced attempt to control the scroller simultaneously * settings-main updates the overscroll with no regard for section transitions * settings-main eagerly removes overscroll on "scroll" events, but can't differentiate between user scrolls and Basic/Advanced-initiated scrolls BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Fix scrolling bugs and hacks caused by undefined load behavior Imports in <body> cause undefined behavior: crbug.com/638074 We have a bunch of hacks to try to avoid doing things with an invalid DOM. Instead of these hacks, sidestep the problem by moving the i18n import to <head> (yeah, this probably breaks i18n-foo in <body>). The code is less haphazard now, but still a minefield because: * Basic and Advanced attempt to control the scroller simultaneously * settings-main updates the overscroll with no regard for section transitions * settings-main eagerly removes overscroll on "scroll" events, but can't differentiate between user scrolls and Basic/Advanced-initiated scrolls BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix scrolling bugs and hacks caused by undefined load behavior Imports in <body> cause undefined behavior: crbug.com/638074 We have a bunch of hacks to try to avoid doing things with an invalid DOM. Instead of these hacks, sidestep the problem by moving the i18n import to <head> (yeah, this probably breaks i18n-foo in <body>). The code is less haphazard now, but still a minefield because: * Basic and Advanced attempt to control the scroller simultaneously * settings-main updates the overscroll with no regard for section transitions * settings-main eagerly removes overscroll on "scroll" events, but can't differentiate between user scrolls and Basic/Advanced-initiated scrolls BUG=637508,537359,589681 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
michaelpg@chromium.org changed reviewers: + dschuyler@chromium.org
Can you please take a preliminary look? This ensures that we don't actually start querying the DOM until we can get accurate results from it, which frees us from some hacks (doWhenReady) but exposes a bunch of other issues I'm trying to fix. It's really friggin' hard to handle the interactions of all these behaviors/pages, so there may still be bugs, but this is the best it's been in a long time. https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:107: setTimeout(function() { I wish I didn't have to do this, but when we route to a section we would set the overscroll (adding this handler), then the MainPageBehavior tries to scroll (triggering this handler) and the overscroll gets reset to 0.
https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:107: setTimeout(function() { On 2016/08/19 04:40:45, michaelpg wrote: > I wish I didn't have to do this, but when we route to a section we would set the > overscroll (adding this handler), then the MainPageBehavior tries to scroll > (triggering this handler) and the overscroll gets reset to 0. would afterNextRender help any better?
PTAL -- was able to simplify after dpapad's CL. This fixes scrolling to section on loading a URL. Also adds tests. I think it can land before or after the overscroll CL (https://codereview.chromium.org/2312423003). https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:107: setTimeout(function() { On 2016/08/23 02:09:50, dschuyler wrote: > On 2016/08/19 04:40:45, michaelpg wrote: > > I wish I didn't have to do this, but when we route to a section we would set > the > > overscroll (adding this handler), then the MainPageBehavior tries to scroll > > (triggering this handler) and the overscroll gets reset to 0. > > would afterNextRender help any better? Ended up not needing this.
The CQ bit was checked by michaelpg@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/09/29 01:12:48, michaelpg wrote: > PTAL -- was able to simplify after dpapad's CL. This fixes > scrolling to section on loading a URL. Also adds tests. > > I think it can land before or after the overscroll CL > (https://codereview.chromium.org/2312423003). > > https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_main/settings_main.js (right): > > https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/settings_main/settings_main.js:107: > setTimeout(function() { > On 2016/08/23 02:09:50, dschuyler wrote: > > On 2016/08/19 04:40:45, michaelpg wrote: > > > I wish I didn't have to do this, but when we route to a section we would set > > the > > > overscroll (adding this handler), then the MainPageBehavior tries to scroll > > > (triggering this handler) and the overscroll gets reset to 0. > > > > would afterNextRender help any better? > > Ended up not needing this. Without https://codereview.chromium.org/2312423003 this CL is not good, imo. Going to sections using the side nav often doesn't scroll to the correct section (though sometimes it does). I patched in Patch#3 from https://codereview.chromium.org/2312423003 and it's a mix. My takeaway is that CL 2312423003 is good to land before this CL. And this CL has an issue that I'm hoping can be fixed before landing. With both CLs Improved: Going directly to chrome://md-setting/reset is better. It reliably scrolls and adds the overscroll. There is a new flash of the page though. i.e. I see a whole screen of sections, then it clears, then the reset section is drawn again in the correct place. Regressed: If I select the following from the side nav: about, then search engine the search engine section is not scrolled to the top. There's a similar problem if I go to about then system: system doesn't scroll to the top. This does not happen with the 2312423003 CL. Unchanged: Going from search engines subpage to About: the about section still jumps to the right (same in the other CL and on ToT).
On 2016/09/29 22:23:55, dschuyler wrote: > On 2016/09/29 01:12:48, michaelpg wrote: > > PTAL -- was able to simplify after dpapad's CL. This fixes > > scrolling to section on loading a URL. Also adds tests. > > > > I think it can land before or after the overscroll CL > > (https://codereview.chromium.org/2312423003). > > > > > https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... > > File chrome/browser/resources/settings/settings_main/settings_main.js (right): > > > > > https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... > > chrome/browser/resources/settings/settings_main/settings_main.js:107: > > setTimeout(function() { > > On 2016/08/23 02:09:50, dschuyler wrote: > > > On 2016/08/19 04:40:45, michaelpg wrote: > > > > I wish I didn't have to do this, but when we route to a section we would > set > > > the > > > > overscroll (adding this handler), then the MainPageBehavior tries to > scroll > > > > (triggering this handler) and the overscroll gets reset to 0. > > > > > > would afterNextRender help any better? > > > > Ended up not needing this. > > Without https://codereview.chromium.org/2312423003 this CL > is not good, imo. Going to sections using the side nav often > doesn't scroll to the correct section (though sometimes it > does). > > I patched in Patch#3 from https://codereview.chromium.org/2312423003 > and it's a mix. > > My takeaway is that CL 2312423003 is good to land before this > CL. And this CL has an issue that I'm hoping can be fixed before > landing. > > > With both CLs > > Improved: > > Going directly to chrome://md-setting/reset is better. It reliably > scrolls and adds the overscroll. There is a new flash of the page > though. i.e. I see a whole screen of sections, then it clears, > then the reset section is drawn again in the correct place. > > Regressed: > > If I select the following from the side nav: about, then search engine > the search engine section is not scrolled to the top. There's a similar > problem if I go to about then system: system doesn't scroll to the top. > This does not happen with the 2312423003 CL. > > Unchanged: > > Going from search engines subpage to About: the about section still > jumps to the right (same in the other CL and on ToT). By "not good" I meant "not good to go", like not yet ready. Not that it wasn't "good". I hope that didn't read too badly.
On 2016/09/29 22:23:55, dschuyler wrote: > On 2016/09/29 01:12:48, michaelpg wrote: > > PTAL -- was able to simplify after dpapad's CL. This fixes > > scrolling to section on loading a URL. Also adds tests. > > > > I think it can land before or after the overscroll CL > > (https://codereview.chromium.org/2312423003). > > > > > https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... > > File chrome/browser/resources/settings/settings_main/settings_main.js (right): > > > > > https://codereview.chromium.org/2259163002/diff/20001/chrome/browser/resource... > > chrome/browser/resources/settings/settings_main/settings_main.js:107: > > setTimeout(function() { > > On 2016/08/23 02:09:50, dschuyler wrote: > > > On 2016/08/19 04:40:45, michaelpg wrote: > > > > I wish I didn't have to do this, but when we route to a section we would > set > > > the > > > > overscroll (adding this handler), then the MainPageBehavior tries to > scroll > > > > (triggering this handler) and the overscroll gets reset to 0. > > > > > > would afterNextRender help any better? > > > > Ended up not needing this. > > Without https://codereview.chromium.org/2312423003 this CL > is not good, imo. Going to sections using the side nav often > doesn't scroll to the correct section (though sometimes it > does). > > I patched in Patch#3 from https://codereview.chromium.org/2312423003 > and it's a mix. > > My takeaway is that CL 2312423003 is good to land before this > CL. And this CL has an issue that I'm hoping can be fixed before > landing. > > > With both CLs > > Improved: > > Going directly to chrome://md-setting/reset is better. It reliably > scrolls and adds the overscroll. There is a new flash of the page > though. i.e. I see a whole screen of sections, then it clears, > then the reset section is drawn again in the correct place. The page loads before the scroll happens -- we can't scroll until the section we want to scroll to exists. It's possible we could scroll earlier than we currently do, but I think that's outside the scope of this CL. There shouldn't be any "clearing" though. We should go directly from "whole screen of sections" to "reset section in the right place", so 2 frames total. (ignoring the People section's flicker on every page load.) > > Regressed: > > If I select the following from the side nav: about, then search engine > the search engine section is not scrolled to the top. There's a similar > problem if I go to about then system: system doesn't scroll to the top. > This does not happen with the 2312423003 CL. Thanks for catching this! Fixed in the latest CL. I had removed something and couldn't find anything that it broke, but it indeed broke switching between About and Settings. > > Unchanged: > > Going from search engines subpage to About: the about section still > jumps to the right (same in the other CL and on ToT).
If CL 2312423003 lands first, LGTM.
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2259163002/#ps100001 (title: "rebase")
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.
Description was changed from ========== MD Settings: Fix scrolling bugs and hacks caused by undefined load behavior Imports in <body> cause undefined behavior: crbug.com/638074 We have a bunch of hacks to try to avoid doing things with an invalid DOM. Instead of these hacks, sidestep the problem by moving the i18n import to <head> (yeah, this probably breaks i18n-foo in <body>). The code is less haphazard now, but still a minefield because: * Basic and Advanced attempt to control the scroller simultaneously * settings-main updates the overscroll with no regard for section transitions * settings-main eagerly removes overscroll on "scroll" events, but can't differentiate between user scrolls and Basic/Advanced-initiated scrolls BUG=637508,537359,589681 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix scrolling bugs and hacks caused by undefined load behavior Imports in <body> cause undefined behavior: crbug.com/638074 We have a bunch of hacks to try to avoid doing things with an invalid DOM. Instead of these hacks, sidestep the problem by moving the i18n import to <head> (yeah, this probably breaks i18n-foo in <body>). The code is less haphazard now, but still a minefield because: * Basic and Advanced attempt to control the scroller simultaneously * settings-main updates the overscroll with no regard for section transitions * settings-main eagerly removes overscroll on "scroll" events, but can't differentiate between user scrolls and Basic/Advanced-initiated scrolls BUG=637508,537359,589681 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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: Fix scrolling bugs and hacks caused by undefined load behavior Imports in <body> cause undefined behavior: crbug.com/638074 We have a bunch of hacks to try to avoid doing things with an invalid DOM. Instead of these hacks, sidestep the problem by moving the i18n import to <head> (yeah, this probably breaks i18n-foo in <body>). The code is less haphazard now, but still a minefield because: * Basic and Advanced attempt to control the scroller simultaneously * settings-main updates the overscroll with no regard for section transitions * settings-main eagerly removes overscroll on "scroll" events, but can't differentiate between user scrolls and Basic/Advanced-initiated scrolls BUG=637508,537359,589681 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix scrolling bugs and hacks caused by undefined load behavior Imports in <body> cause undefined behavior: crbug.com/638074 We have a bunch of hacks to try to avoid doing things with an invalid DOM. Instead of these hacks, sidestep the problem by moving the i18n import to <head> (yeah, this probably breaks i18n-foo in <body>). The code is less haphazard now, but still a minefield because: * Basic and Advanced attempt to control the scroller simultaneously * settings-main updates the overscroll with no regard for section transitions * settings-main eagerly removes overscroll on "scroll" events, but can't differentiate between user scrolls and Basic/Advanced-initiated scrolls BUG=637508,537359,589681 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6dabb5a5dcefa2d2a775bd5abdf64f87e8aa2621 Cr-Commit-Position: refs/heads/master@{#422625} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6dabb5a5dcefa2d2a775bd5abdf64f87e8aa2621 Cr-Commit-Position: refs/heads/master@{#422625} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
