Chromium Code Reviews
|
|
|
Created:
4 years, 11 months ago by tdresser Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-events_chromium.org, dglazkov+blink, eae+blinkwatch, cbiesinger Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionForce layout for main thread scrolling.
BUG=495802
TEST=LayoutTests/fast/scroll-behavior/overflow-scroll-triggers-layout.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197126
Patch Set 1 #Patch Set 2 : Fix layout-triggers test. #
Total comments: 4
Patch Set 3 : Fix scroll customization. #Patch Set 4 : Address rbyers' nits. #Patch Set 5 : Rebase. #
Messages
Total messages: 23 (7 generated)
tdresser@chromium.org changed reviewers: + rbyers@chromium.org
Looks like everyone is generally in favor of this change. https://groups.google.com/a/chromium.org/forum/#!topic/input-dev/kj9PebO-u8Y Rick, can you take a look?
Hmm, why should this ignore pending stylesheets?
On 2015/06/04 17:16:57, cbiesinger1 wrote: > Hmm, why should this ignore pending stylesheets? This aligns us with the behavior of Element::setScrollLeft, along with the other methods for scrolling programmatically. I'm not super clear on what the alternative to ignoring pending stylesheets is. Based on https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., it sounds like we'd need to block until all stylesheets are loaded, which isn't practical. Am I misinterpreting the documentation?
LGTM with nits https://codereview.chromium.org/1148003010/diff/20001/LayoutTests/fast/scroll... File LayoutTests/fast/scroll-behavior/overflow-scroll-triggers-layout.html (right): https://codereview.chromium.org/1148003010/diff/20001/LayoutTests/fast/scroll... LayoutTests/fast/scroll-behavior/overflow-scroll-triggers-layout.html:3: <head> nit: coding style says to omit unnecessary tags like <html>, <head> and <Body> https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... https://codereview.chromium.org/1148003010/diff/20001/LayoutTests/fast/scroll... LayoutTests/fast/scroll-behavior/overflow-scroll-triggers-layout.html:23: <div id="inner"> nit: indentation would help make this html clearer
LGTM with nits
On 2015/06/04 17:34:33, tdresser wrote: > On 2015/06/04 17:16:57, cbiesinger1 wrote: > > Hmm, why should this ignore pending stylesheets? > > This aligns us with the behavior of Element::setScrollLeft, along with the other > methods for scrolling programmatically. I'm not super clear on what the > alternative to ignoring pending stylesheets is. > > Based on > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., > it sounds like we'd need to block until all stylesheets are loaded, which isn't > practical. > > Am I misinterpreting the documentation? I agree this should be the same as Element::setScrollLeft (otherwise we're not meeting out goal of "explaining scrolling"). I agree ignorning pending stylesheets is questionable, but I think it's orthogonal to this CL - today we do for all scrolling, let's be consistent. Maybe add a comment saying this is to be consistent with the behavior of programmatic scroll APIs? We could file a bug for the layout team to investigate that...
On 2015/06/04 17:59:21, Rick Byers wrote: > On 2015/06/04 17:34:33, tdresser wrote: > > On 2015/06/04 17:16:57, cbiesinger1 wrote: > > > Hmm, why should this ignore pending stylesheets? > > > > This aligns us with the behavior of Element::setScrollLeft, along with the > other > > methods for scrolling programmatically. I'm not super clear on what the > > alternative to ignoring pending stylesheets is. > > > > Based on > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., > > it sounds like we'd need to block until all stylesheets are loaded, which > isn't > > practical. > > > > Am I misinterpreting the documentation? > > I agree this should be the same as Element::setScrollLeft (otherwise we're not > meeting out goal of "explaining scrolling"). I agree ignorning pending > stylesheets is questionable, but I think it's orthogonal to this CL - today we > do for all scrolling, let's be consistent. Maybe add a comment saying this is > to be consistent with the behavior of programmatic scroll APIs? We could file a > bug for the layout team to investigate that... Fair enough -- my mental model is that only programmatic calls need to ignore pending stylesheets, but maybe the consistency is worth it. Here's what I'm concerned about. I once fixed a bug where sometimes you'd get a flash of white when transitioning between pages. It turned out the reason was that if you move the mouse while the new page is loading, we'd hit test in order to dispatch a mousemove event. The hit test would force layout ignoring pending stylesheets. Forcing the layout produced a white page because nothing is loaded yet. -> sucky situation So, if someone moves the mousewheel in the early stages of a pageload, we might still have this issue?
cbiesinger@, do you have the bug with mousemove that you're referring to handy? https://codereview.chromium.org/1148003010/diff/20001/LayoutTests/fast/scroll... File LayoutTests/fast/scroll-behavior/overflow-scroll-triggers-layout.html (right): https://codereview.chromium.org/1148003010/diff/20001/LayoutTests/fast/scroll... LayoutTests/fast/scroll-behavior/overflow-scroll-triggers-layout.html:3: <head> On 2015/06/04 17:57:00, Rick Byers wrote: > nit: coding style says to omit unnecessary tags like <html>, <head> and <Body> > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... Done. https://codereview.chromium.org/1148003010/diff/20001/LayoutTests/fast/scroll... LayoutTests/fast/scroll-behavior/overflow-scroll-triggers-layout.html:23: <div id="inner"> On 2015/06/04 17:57:00, Rick Byers wrote: > nit: indentation would help make this html clearer Done.
On 2015/06/04 18:36:21, cbiesinger wrote: > On 2015/06/04 17:59:21, Rick Byers wrote: > > On 2015/06/04 17:34:33, tdresser wrote: > > > On 2015/06/04 17:16:57, cbiesinger1 wrote: > > > > Hmm, why should this ignore pending stylesheets? > > > > > > This aligns us with the behavior of Element::setScrollLeft, along with the > > other > > > methods for scrolling programmatically. I'm not super clear on what the > > > alternative to ignoring pending stylesheets is. > > > > > > Based on > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., > > > it sounds like we'd need to block until all stylesheets are loaded, which > > isn't > > > practical. > > > > > > Am I misinterpreting the documentation? > > > > I agree this should be the same as Element::setScrollLeft (otherwise we're not > > meeting out goal of "explaining scrolling"). I agree ignorning pending > > stylesheets is questionable, but I think it's orthogonal to this CL - today we > > do for all scrolling, let's be consistent. Maybe add a comment saying this is > > to be consistent with the behavior of programmatic scroll APIs? We could file > a > > bug for the layout team to investigate that... > > Fair enough -- my mental model is that only programmatic calls need to ignore > pending stylesheets, but maybe the consistency is worth it. > > Here's what I'm concerned about. I once fixed a bug where sometimes you'd get a > flash of white when transitioning between pages. It turned out the reason was > that if you move the mouse while the new page is loading, we'd hit test in order > to dispatch a mousemove event. The hit test would force layout ignoring pending > stylesheets. Forcing the layout produced a white page because nothing is loaded > yet. -> sucky situation > So, if someone moves the mousewheel in the early stages of a pageload, we might > still have this issue? Interesting case! I agree we want to avoid this. Why don't we have the exact same issue with script? Eg. some script reads documentElement.scrollTop during load, forcing layout ignoring pending stylesheets?
On 2015/06/04 20:09:14, Rick Byers wrote: > On 2015/06/04 18:36:21, cbiesinger wrote: > > On 2015/06/04 17:59:21, Rick Byers wrote: > > > On 2015/06/04 17:34:33, tdresser wrote: > > > > On 2015/06/04 17:16:57, cbiesinger1 wrote: > > > > > Hmm, why should this ignore pending stylesheets? > > > > > > > > This aligns us with the behavior of Element::setScrollLeft, along with the > > > other > > > > methods for scrolling programmatically. I'm not super clear on what the > > > > alternative to ignoring pending stylesheets is. > > > > > > > > Based on > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., > > > > it sounds like we'd need to block until all stylesheets are loaded, which > > > isn't > > > > practical. > > > > > > > > Am I misinterpreting the documentation? > > > > > > I agree this should be the same as Element::setScrollLeft (otherwise we're > not > > > meeting out goal of "explaining scrolling"). I agree ignorning pending > > > stylesheets is questionable, but I think it's orthogonal to this CL - today > we > > > do for all scrolling, let's be consistent. Maybe add a comment saying this > is > > > to be consistent with the behavior of programmatic scroll APIs? We could > file > > a > > > bug for the layout team to investigate that... > > > > Fair enough -- my mental model is that only programmatic calls need to ignore > > pending stylesheets, but maybe the consistency is worth it. > > > > Here's what I'm concerned about. I once fixed a bug where sometimes you'd get > a > > flash of white when transitioning between pages. It turned out the reason was > > that if you move the mouse while the new page is loading, we'd hit test in > order > > to dispatch a mousemove event. The hit test would force layout ignoring > pending > > stylesheets. Forcing the layout produced a white page because nothing is > loaded > > yet. -> sucky situation > > So, if someone moves the mousewheel in the early stages of a pageload, we > might > > still have this issue? > > Interesting case! I agree we want to avoid this. Why don't we have the exact > same issue with script? Eg. some script reads documentElement.scrollTop during > load, forcing layout ignoring pending stylesheets? +esprehn@, to comment on whether we should be ignoring pending style sheets. cbiesinger@ is concerned that if we ignore pending style sheets, we could cause the screen to render a bunch of white earlier than it would otherwise.
tdresser@chromium.org changed reviewers: + esprehn@chromium.org
+esprehn to comment on https://codereview.chromium.org/1148003010/#msg8 regarding the use of updateLayoutIgnorePendingStylesheets.
On 2015/06/10 13:49:56, tdresser wrote: > +esprehn to comment on https://codereview.chromium.org/1148003010/#msg8 > regarding the use of updateLayoutIgnorePendingStylesheets. I talked with esprehn@ about this offline. I'm unable to repro the issue mentioned by cbiesinger@, and esprehn thinks this is the correct approach. Landing like this for now - if this issue arises in the wild, we can readdress the approach.
The CQ bit was checked by tdresser@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1148003010/#ps60001 (title: "Address rbyers' nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1148003010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/58928)
The CQ bit was checked by tdresser@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1148003010/#ps80001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1148003010/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197126 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
