|
|
Created:
6 years ago by Yufeng Shen (Slow to review) Modified:
6 years ago CC:
blink-layers+watch_chromium.org, blink-reviews, blink-reviews-rendering, eae+blinkwatch, eae, jchaffraix+rendering, kenneth.christiansen, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionPass integer scroll position to CC if frameView has non-layered viewport constrained objects
Non-layered Viewport constrained objects, e.g. fixed position elements,
are not composited and are positioned in Blink using integer coordinates.
In that case, we don't want to set the WebLayer's scroll position at
fractional precision otherwise the WebLayer's position after snapping
to device pixel can be off with regard to fixed position elements.
BUG=414283
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187073
Patch Set 1 #Patch Set 2 : better formmating #Patch Set 3 : ignore invisible fixed position elements #Patch Set 4 : remove debugging #Patch Set 5 : call updateDescendantDependentFlags() before calling hasVisibleContent() otherwise it crashes #
Total comments: 1
Patch Set 6 : use HasNonLayerViewportConstrainedObjects #Patch Set 7 : use preferCompositingToLCDTextEnabled() to gauge sending integer/floating point scroll offset to cc #Patch Set 8 : restore to using ScrollingCoordinator::HasNonLayer #
Messages
Total messages: 36 (5 generated)
miletus@chromium.org changed reviewers: + leviw@chromium.org, rbyers@chromium.org
miletus@chromium.org changed reviewers: + skobes@chromium.org
+ skobes@.
rbyers@chromium.org changed reviewers: + vollick@chromium.org - skobes@chromium.org
+vollick who may be able to provide some better guidance here. As it is now, this CL would break fractional scrolling on high-dpi (eg. Android) in many cases (whenever there's a visible fixed position element), right? That seems bad. We really need this logic to use integer scrolling _only_ in scenarios where we're already disabling threaded scrolling today. High level, the compositing infrastructure already has some logic to determine if a scroll event must go to main because repainting will be necessary during the scroll. Trying to duplicate this logic seems very brittle. Can't we re-use the logic somehow? Eg. if blink would have told CC to send scrolling to main due to repainting, then blink should scroll by integers. I don't know the ScrollingCoordinator code well enough here to say exactly how that would look, but Ian probably does.
On 2014/12/03 18:47:34, Rick Byers wrote: > +vollick who may be able to provide some better guidance here. > > As it is now, this CL would break fractional scrolling on high-dpi (eg. Android) > in many cases (whenever there's a visible fixed position element), right? That > seems bad. We really need this logic to use integer scrolling _only_ in > scenarios where we're already disabling threaded scrolling today. > No, the code path should not affect impl scrolling. Before we are working on main fractional scrolling, ScrollingCoordinator was already passing integer scroll offset to CC so this should not break impl scrolling. The patch only limits the case when Blink sends fractional scroll offset to impl. > High level, the compositing infrastructure already has some logic to determine > if a scroll event must go to main because repainting will be necessary during > the scroll. Trying to duplicate this logic seems very brittle. Can't we re-use > the logic somehow? Eg. if blink would have told CC to send scrolling to main > due to repainting, then blink should scroll by integers. > Exactly. ScrollingCoordinator has this function hasVisibleSlowRepaintViewportConstrainedObjects implementing the logic you had in mind. The hasVisibileViewportConstrainedObjects I added is basically copying over from it, with one exception that it does not check if the fixed position element is composited. I am doing this on purpose for staging my patches, as the FIXME I put in ScrollingCoordinator.cpp. So high level it is 1) It does not affect impl scrolling, with or without fixed position element, if they are composited 2) It enables main fractional scrolling for page without visible fixed position elements. 3) Followup would be enable main fractional scrolling for page with composited fixed position elements. > I don't know the ScrollingCoordinator code well enough here to say exactly how > that would look, but Ian probably does.
On 2014/12/03 19:13:37, Yufeng Shen wrote: > On 2014/12/03 18:47:34, Rick Byers wrote: > > +vollick who may be able to provide some better guidance here. > > > > As it is now, this CL would break fractional scrolling on high-dpi (eg. > Android) > > in many cases (whenever there's a visible fixed position element), right? > That > > seems bad. We really need this logic to use integer scrolling _only_ in > > scenarios where we're already disabling threaded scrolling today. > > > > No, the code path should not affect impl scrolling. Before we are working on > main fractional scrolling, ScrollingCoordinator was already passing integer > scroll offset to CC so this should not break impl scrolling. > The patch only limits the case when Blink sends fractional scroll offset to > impl. Duh, of course ;-) Sorry. > > High level, the compositing infrastructure already has some logic to determine > > if a scroll event must go to main because repainting will be necessary during > > the scroll. Trying to duplicate this logic seems very brittle. Can't we > re-use > > the logic somehow? Eg. if blink would have told CC to send scrolling to main > > due to repainting, then blink should scroll by integers. > > > > > Exactly. ScrollingCoordinator has this function > hasVisibleSlowRepaintViewportConstrainedObjects > implementing the logic you had in mind. The > hasVisibileViewportConstrainedObjects I added > is basically copying over from it, with one exception that it does not check if > the fixed > position element is composited. > > I am doing this on purpose for staging my patches, as the FIXME I put in > ScrollingCoordinator.cpp. > > So high level it is > 1) It does not affect impl scrolling, with or without fixed position element, if > they are composited > 2) It enables main fractional scrolling for page without visible fixed position > elements. > 3) Followup would be enable main fractional scrolling for page with composited > fixed position elements. Ok, this is sounding more reasonable. I'm going to need a bit more time to study the code to see if I can find a cleaner way of sharing the logic between these two cases. At first glance it looked pretty ad hoc to me (as opposed to re-using the existing concept), but perhaps it's the best we can reasonably do. I'll dig into this in detail tomorrow - sorry for the delay. > > I don't know the ScrollingCoordinator code well enough here to say exactly how > > that would look, but Ian probably does.
Here's one idea for unifying the logic with ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects. WDYT? https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... Source/core/page/scrolling/ScrollingCoordinator.cpp:380: if (hasVisibleViewportConstrainedObjects) { Rather than taking this bit in from our caller (which computes it independently in FrameView), can we just test m_lastMainThreadScrollingReasons for the HasNonLayerViewportConstrainedObjects flag? We really should be able to share this logic somehow. I don't know if there are potential timing issues where m_lastMainThreadScrollingReasons is out of date.
On 2014/12/05 19:01:41, Rick Byers wrote: > Here's one idea for unifying the logic with > ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects. WDYT? > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > Source/core/page/scrolling/ScrollingCoordinator.cpp:380: if > (hasVisibleViewportConstrainedObjects) { > Rather than taking this bit in from our caller (which computes it independently > in FrameView), can we just test m_lastMainThreadScrollingReasons for the > HasNonLayerViewportConstrainedObjects flag? We really should be able to share > this logic somehow. I don't know if there are potential timing issues where > m_lastMainThreadScrollingReasons is out of date. Alright. I will take your way of checking HasNonLayerViewportConstrainedObjects. Then this patch only fixes the issue of scrolling fixed position element on low-dpi device. For scrolling fixed position element on high-dpi device, I will get another CL that computes the correct fractional scroll offset for the fixed-position layer.
On 2014/12/09 04:10:53, Yufeng Shen wrote: > On 2014/12/05 19:01:41, Rick Byers wrote: > > Here's one idea for unifying the logic with > > ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects. WDYT? > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > Source/core/page/scrolling/ScrollingCoordinator.cpp:380: if > > (hasVisibleViewportConstrainedObjects) { > > Rather than taking this bit in from our caller (which computes it > independently > > in FrameView), can we just test m_lastMainThreadScrollingReasons for the > > HasNonLayerViewportConstrainedObjects flag? We really should be able to share > > this logic somehow. I don't know if there are potential timing issues where > > m_lastMainThreadScrollingReasons is out of date. > > Alright. I will take your way of checking HasNonLayerViewportConstrainedObjects. This definitely seems a lot simpler and more maintainable than attempting to duplicate the logic, thanks! I don't know if there are timing issues with relying on m_lastMainThreadScrollReasons. I think vollick@ needs to weigh in on this CL, he's probably the best expert on this. One odd side effect I expect from the design constraints here is that adding the first non-composited viewport contained object will cause the scroll position to snap to an integer. This could be a little surprising, but I suspect it's OK and probably very hard to avoid without updating all our rendering code to support fractional co-ordinates. > Then this patch only fixes the issue of scrolling fixed position element on > low-dpi device. > For scrolling fixed position element on high-dpi device, I will get another CL > that computes > the correct fractional scroll offset for the fixed-position layer. I think it makes sense that we'd want to address the two separately. In the high-dpi case
On 2014/12/10 02:12:51, Rick Byers wrote: > On 2014/12/09 04:10:53, Yufeng Shen wrote: > > On 2014/12/05 19:01:41, Rick Byers wrote: > > > Here's one idea for unifying the logic with > > > ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects. WDYT? > > > > > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > > File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): > > > > > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > > Source/core/page/scrolling/ScrollingCoordinator.cpp:380: if > > > (hasVisibleViewportConstrainedObjects) { > > > Rather than taking this bit in from our caller (which computes it > > independently > > > in FrameView), can we just test m_lastMainThreadScrollingReasons for the > > > HasNonLayerViewportConstrainedObjects flag? We really should be able to > share > > > this logic somehow. I don't know if there are potential timing issues where > > > m_lastMainThreadScrollingReasons is out of date. > > > > Alright. I will take your way of checking > HasNonLayerViewportConstrainedObjects. > > This definitely seems a lot simpler and more maintainable than attempting to > duplicate the logic, thanks! I don't know if there are timing issues with > relying on m_lastMainThreadScrollReasons. I think vollick@ needs to weigh in on > this CL, he's probably the best expert on this. By the way, if Ian has no objections, then this CL looks good to me too. > One odd side effect I expect from the design constraints here is that adding the > first non-composited viewport contained object will cause the scroll position to > snap to an integer. This could be a little surprising, but I suspect it's OK > and probably very hard to avoid without updating all our rendering code to > support fractional co-ordinates. > > > Then this patch only fixes the issue of scrolling fixed position element on > > low-dpi device. > > For scrolling fixed position element on high-dpi device, I will get another CL > > that computes > > the correct fractional scroll offset for the fixed-position layer. > > I think it makes sense that we'd want to address the two separately. In the > high-dpi case Sorry, I forgot to finish this thought. High-dpi can mean two different things in this context: can display fractional scroll offsets, and will promote fixed-position elements to layers. We can't necessarily assume the former implies the latter. Eg. Cases like 'background-attachment: fixed' will cause non-composited scrolling. I was expecting that what you've got here will work for any non-composited scrolling (high-dpi and low-dpi). Do you know if that's true?
On 2014/12/10 02:21:35, Rick Byers wrote: > On 2014/12/10 02:12:51, Rick Byers wrote: > > On 2014/12/09 04:10:53, Yufeng Shen wrote: > > > On 2014/12/05 19:01:41, Rick Byers wrote: > > > > Here's one idea for unifying the logic with > > > > ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects. > WDYT? > > > > > > > > > > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > > > File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > > > Source/core/page/scrolling/ScrollingCoordinator.cpp:380: if > > > > (hasVisibleViewportConstrainedObjects) { > > > > Rather than taking this bit in from our caller (which computes it > > > independently > > > > in FrameView), can we just test m_lastMainThreadScrollingReasons for the > > > > HasNonLayerViewportConstrainedObjects flag? We really should be able to > > share > > > > this logic somehow. I don't know if there are potential timing issues > where > > > > m_lastMainThreadScrollingReasons is out of date. > > > > > > Alright. I will take your way of checking > > HasNonLayerViewportConstrainedObjects. > > > > This definitely seems a lot simpler and more maintainable than attempting to > > duplicate the logic, thanks! I don't know if there are timing issues with > > relying on m_lastMainThreadScrollReasons. I think vollick@ needs to weigh in > on > > this CL, he's probably the best expert on this. > > By the way, if Ian has no objections, then this CL looks good to me too. > > > One odd side effect I expect from the design constraints here is that adding > the > > first non-composited viewport contained object will cause the scroll position > to > > snap to an integer. This could be a little surprising, but I suspect it's OK > > and probably very hard to avoid without updating all our rendering code to > > support fractional co-ordinates. > > > > > Then this patch only fixes the issue of scrolling fixed position element on > > > low-dpi device. > > > For scrolling fixed position element on high-dpi device, I will get another > CL > > > that computes > > > the correct fractional scroll offset for the fixed-position layer. > > > > I think it makes sense that we'd want to address the two separately. In the > > high-dpi case > > Sorry, I forgot to finish this thought. High-dpi can mean two different things > in this context: can display fractional scroll offsets, and will promote > fixed-position elements to layers. We can't necessarily assume the former > implies the latter. Eg. Cases like 'background-attachment: fixed' will cause > non-composited scrolling. > > I was expecting that what you've got here will work for any non-composited > scrolling (high-dpi and low-dpi). Do you know if that's true? I guess my words are misleading. The dividing line should be "if the fixed position element has its own layer" instead of "if it is high-dpi or low-dpi". You can in high-dpi but still fixed-position may not get its own layer or you are in low-dpi but fixed-position get its own layer. So this CL is meant to make the case where fixed-position does not get its own layer working. And the a followup CL will make the case where fixed-position has its own layer working.
On 2014/12/10 03:07:14, Yufeng Shen wrote: > On 2014/12/10 02:21:35, Rick Byers wrote: > > On 2014/12/10 02:12:51, Rick Byers wrote: > > > On 2014/12/09 04:10:53, Yufeng Shen wrote: > > > > On 2014/12/05 19:01:41, Rick Byers wrote: > > > > > Here's one idea for unifying the logic with > > > > > ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects. > > WDYT? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > > > > File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > > > > Source/core/page/scrolling/ScrollingCoordinator.cpp:380: if > > > > > (hasVisibleViewportConstrainedObjects) { > > > > > Rather than taking this bit in from our caller (which computes it > > > > independently > > > > > in FrameView), can we just test m_lastMainThreadScrollingReasons for the > > > > > HasNonLayerViewportConstrainedObjects flag? We really should be able to > > > share > > > > > this logic somehow. I don't know if there are potential timing issues > > where > > > > > m_lastMainThreadScrollingReasons is out of date. > > > > > > > > Alright. I will take your way of checking > > > HasNonLayerViewportConstrainedObjects. > > > > > > This definitely seems a lot simpler and more maintainable than attempting to > > > duplicate the logic, thanks! I don't know if there are timing issues with > > > relying on m_lastMainThreadScrollReasons. I think vollick@ needs to weigh > in > > on > > > this CL, he's probably the best expert on this. > > > > By the way, if Ian has no objections, then this CL looks good to me too. > > > > > One odd side effect I expect from the design constraints here is that adding > > the > > > first non-composited viewport contained object will cause the scroll > position > > to > > > snap to an integer. This could be a little surprising, but I suspect it's > OK > > > and probably very hard to avoid without updating all our rendering code to > > > support fractional co-ordinates. > > > > > > > Then this patch only fixes the issue of scrolling fixed position element > on > > > > low-dpi device. > > > > For scrolling fixed position element on high-dpi device, I will get > another > > CL > > > > that computes > > > > the correct fractional scroll offset for the fixed-position layer. > > > > > > I think it makes sense that we'd want to address the two separately. In the > > > high-dpi case > > > > Sorry, I forgot to finish this thought. High-dpi can mean two different > things > > in this context: can display fractional scroll offsets, and will promote > > fixed-position elements to layers. We can't necessarily assume the former > > implies the latter. Eg. Cases like 'background-attachment: fixed' will cause > > non-composited scrolling. > > > > I was expecting that what you've got here will work for any non-composited > > scrolling (high-dpi and low-dpi). Do you know if that's true? > > I guess my words are misleading. The dividing line should be "if the fixed > position > element has its own layer" instead of "if it is high-dpi or low-dpi". > > You can in high-dpi but still fixed-position may not get its own layer or you > are > in low-dpi but fixed-position get its own layer. > > So this CL is meant to make the case where fixed-position does not get its own > layer working. And the a followup CL will make the case where fixed-position > has its own layer working. That makes perfect sense, thanks! That's also equivalent to saying "if we're doing composited scrolling" (vs. repainting on every scroll update).
On 2014/12/10 03:11:28, Rick Byers wrote: > On 2014/12/10 03:07:14, Yufeng Shen wrote: > > On 2014/12/10 02:21:35, Rick Byers wrote: > > > On 2014/12/10 02:12:51, Rick Byers wrote: > > > > On 2014/12/09 04:10:53, Yufeng Shen wrote: > > > > > On 2014/12/05 19:01:41, Rick Byers wrote: > > > > > > Here's one idea for unifying the logic with > > > > > > ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects. > > > WDYT? > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > > > > > File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolli... > > > > > > Source/core/page/scrolling/ScrollingCoordinator.cpp:380: if > > > > > > (hasVisibleViewportConstrainedObjects) { > > > > > > Rather than taking this bit in from our caller (which computes it > > > > > independently > > > > > > in FrameView), can we just test m_lastMainThreadScrollingReasons for > the > > > > > > HasNonLayerViewportConstrainedObjects flag? We really should be able > to > > > > share > > > > > > this logic somehow. I don't know if there are potential timing issues > > > where > > > > > > m_lastMainThreadScrollingReasons is out of date. > > > > > > > > > > Alright. I will take your way of checking > > > > HasNonLayerViewportConstrainedObjects. > > > > > > > > This definitely seems a lot simpler and more maintainable than attempting > to > > > > duplicate the logic, thanks! I don't know if there are timing issues with > > > > relying on m_lastMainThreadScrollReasons. I think vollick@ needs to weigh > > in > > > on > > > > this CL, he's probably the best expert on this. > > > > > > By the way, if Ian has no objections, then this CL looks good to me too. > > > > > > > One odd side effect I expect from the design constraints here is that > adding > > > the > > > > first non-composited viewport contained object will cause the scroll > > position > > > to > > > > snap to an integer. This could be a little surprising, but I suspect it's > > OK > > > > and probably very hard to avoid without updating all our rendering code to > > > > support fractional co-ordinates. > > > > > > > > > Then this patch only fixes the issue of scrolling fixed position element > > on > > > > > low-dpi device. > > > > > For scrolling fixed position element on high-dpi device, I will get > > another > > > CL > > > > > that computes > > > > > the correct fractional scroll offset for the fixed-position layer. > > > > > > > > I think it makes sense that we'd want to address the two separately. In > the > > > > high-dpi case > > > > > > Sorry, I forgot to finish this thought. High-dpi can mean two different > > things > > > in this context: can display fractional scroll offsets, and will promote > > > fixed-position elements to layers. We can't necessarily assume the former > > > implies the latter. Eg. Cases like 'background-attachment: fixed' will > cause > > > non-composited scrolling. > > > > > > I was expecting that what you've got here will work for any non-composited > > > scrolling (high-dpi and low-dpi). Do you know if that's true? > > > > I guess my words are misleading. The dividing line should be "if the fixed > > position > > element has its own layer" instead of "if it is high-dpi or low-dpi". > > > > You can in high-dpi but still fixed-position may not get its own layer or you > > are > > in low-dpi but fixed-position get its own layer. > > > > So this CL is meant to make the case where fixed-position does not get its own > > layer working. And the a followup CL will make the case where fixed-position > > has its own layer working. > > That makes perfect sense, thanks! That's also equivalent to saying "if we're > doing composited scrolling" (vs. repainting on every scroll update). The use of m_lastMainThreadScrollingReasons here looks reasonable. The ScrollingCoordinator changes lgtm. I'm not a reviewer in Source/web unfortunately, so you'll need to find another reviewer for that.
miletus@chromium.org changed reviewers: + aelias@chromium.org
Alex, mind taking a look at Source/web/ ?
Are we planning on exposing fractional scrolling to web developers with this hack in place? It seems deeply confusing that making some arbitrary element on the page "position: fixed" would suddenly alter core scrolling behavior. It seems to me we should hold off exposing fractional scrolling at all until we can make fixed-pos elements behave properly.
I don't really understand how this change helps with "staging" your work on the feature either. When you fix the underlying bug, the lines introduced in this change will be deleted. So I think there's no reason to land this, as all this does is introduce code complexity to substitute a subtle bug for an obviously visible one.
OK, rereading the discussion, I realize you're actually planning to maintain this codepath for the long run for the corner case of non-composited fixed-pos elements, so this isn't just a staging-purposes temporary hack. Hmm. And non-composited fixed-pos are kind of a horrible situation to begin with so this is just piling on another hack onto that particular heap. As I recall, the reason we have non-composited fixed-pos at all is because of LCD text. On the other hand, the main reason we want fractional scrolling is because of high-DPI screen. Those are strictly separate configurations. So instead of explicitly checking for fixed-pos here, what would you think about disabling fractional scroll across the board when LCD text setting is enabled? I think that would be more maintainable.
On 2014/12/10 20:33:45, aelias wrote: > Are we planning on exposing fractional scrolling to web developers with this > hack in place? It seems deeply confusing that making some arbitrary element on > the page "position: fixed" would suddenly alter core scrolling behavior. It > seems to me we should hold off exposing fractional scrolling at all until we can > make fixed-pos elements behave properly. Yes we are planning to expose the fractional scrolling to web developers. Due to the limitation of Blink can't position elements at device pixel level, non-composited fixed position element can't stay fixed relative to the viewport which can be scrolled at device pixel level in compositor. If the fixed-pos elements can have its own compositor layer, then it can be positioned at device pixel level in compositor. So this CL only applies to the first case that if fixed-pos elements is not composited, we fallback to reporting the scroll offset of the scrollable layer using integer, so the fixe-pos element and the scrollable layer stay fixed since both are using integer scroll offset. A followup CL will fix the second case that when fixed-pos elements are composited, which will go through a different path. And of course currently the fractional scrolling behavior is disabled in production until I fix the issues for fixed-pos elements.
On 2014/12/10 21:03:22, aelias wrote: > OK, rereading the discussion, I realize you're actually planning to maintain > this codepath for the long run for the corner case of non-composited fixed-pos > elements, so this isn't just a staging-purposes temporary hack. Hmm. And > non-composited fixed-pos are kind of a horrible situation to begin with so this > is just piling on another hack onto that particular heap. > > As I recall, the reason we have non-composited fixed-pos at all is because of > LCD text. On the other hand, the main reason we want fractional scrolling is > because of high-DPI screen. Those are strictly separate configurations. So > instead of explicitly checking for fixed-pos here, what would you think about > disabling fractional scroll across the board when LCD text setting is enabled? > I think that would be more maintainable. hmm, but I feel "explicitly checking for fixed pos" is kind of better. Since I know if fixed-pos element exists and does not have a composited layer, then fractional scroll does not play well. Instead, if I check LCD text setting, the implication from "LCD text setting" -> "fixed-pos element is composited or not" might change. WDYT ?
Makes sense. Our last messages crossed. How about my alternative proposal in #20 to instead disable fractional scroll based on LCD text setting? I think it's much cleaner if we can decide which behavior we use statically at startup, instead of dynamically based on page contents.
On 2014/12/10 21:14:35, Yufeng Shen wrote: > On 2014/12/10 21:03:22, aelias wrote: > > OK, rereading the discussion, I realize you're actually planning to maintain > > this codepath for the long run for the corner case of non-composited fixed-pos > > elements, so this isn't just a staging-purposes temporary hack. Hmm. And > > non-composited fixed-pos are kind of a horrible situation to begin with so > this > > is just piling on another hack onto that particular heap. > > > > As I recall, the reason we have non-composited fixed-pos at all is because of > > LCD text. On the other hand, the main reason we want fractional scrolling is > > because of high-DPI screen. Those are strictly separate configurations. So > > instead of explicitly checking for fixed-pos here, what would you think about > > disabling fractional scroll across the board when LCD text setting is enabled? > > > I think that would be more maintainable. > > hmm, but I feel "explicitly checking for fixed pos" is kind of better. Since I > know if fixed-pos element exists and does not have a composited layer, then > fractional scroll does not play well. Instead, if I check LCD text setting, the > implication from "LCD text setting" -> "fixed-pos element is composited or not" > might change. WDYT ? I don't think that implication can ever change. It's a fundamental theoretical problem with LCD text that it can force noncomposited elements. So I think the static setting is cleaner.
On 2014/12/10 21:18:02, aelias wrote: > On 2014/12/10 21:14:35, Yufeng Shen wrote: > > On 2014/12/10 21:03:22, aelias wrote: > > > OK, rereading the discussion, I realize you're actually planning to maintain > > > this codepath for the long run for the corner case of non-composited > fixed-pos > > > elements, so this isn't just a staging-purposes temporary hack. Hmm. And > > > non-composited fixed-pos are kind of a horrible situation to begin with so > > this > > > is just piling on another hack onto that particular heap. > > > > > > As I recall, the reason we have non-composited fixed-pos at all is because > of > > > LCD text. On the other hand, the main reason we want fractional scrolling > is > > > because of high-DPI screen. Those are strictly separate configurations. So > > > instead of explicitly checking for fixed-pos here, what would you think > about > > > disabling fractional scroll across the board when LCD text setting is > enabled? > > > > > I think that would be more maintainable. > > > > hmm, but I feel "explicitly checking for fixed pos" is kind of better. Since I > > > know if fixed-pos element exists and does not have a composited layer, then > > fractional scroll does not play well. Instead, if I check LCD text setting, > the > > implication from "LCD text setting" -> "fixed-pos element is composited or > not" > > might change. WDYT ? > > I don't think that implication can ever change. It's a fundamental theoretical > problem with LCD text that it can force noncomposited elements. So I think the > static setting is cleaner. Secondly, on Android which are all hi-dpi, 100% of our fixed-pos elements are composited and I would consider it a serious bug if they sometimes were not. So both sides of the implication hold.
On 2014/12/10 21:18:02, aelias wrote: > On 2014/12/10 21:14:35, Yufeng Shen wrote: > > On 2014/12/10 21:03:22, aelias wrote: > > > OK, rereading the discussion, I realize you're actually planning to maintain > > > this codepath for the long run for the corner case of non-composited > fixed-pos > > > elements, so this isn't just a staging-purposes temporary hack. Hmm. And > > > non-composited fixed-pos are kind of a horrible situation to begin with so > > this > > > is just piling on another hack onto that particular heap. > > > > > > As I recall, the reason we have non-composited fixed-pos at all is because > of > > > LCD text. On the other hand, the main reason we want fractional scrolling > is > > > because of high-DPI screen. Those are strictly separate configurations. So > > > instead of explicitly checking for fixed-pos here, what would you think > about > > > disabling fractional scroll across the board when LCD text setting is > enabled? > > > > > I think that would be more maintainable. > > > > hmm, but I feel "explicitly checking for fixed pos" is kind of better. Since I > > > know if fixed-pos element exists and does not have a composited layer, then > > fractional scroll does not play well. Instead, if I check LCD text setting, > the > > implication from "LCD text setting" -> "fixed-pos element is composited or > not" > > might change. WDYT ? > > I don't think that implication can ever change. It's a fundamental theoretical > problem with LCD text that it can force noncomposited elements. So I think the > static setting is cleaner. How about the fixed-pos elements are invisible ? In that case, we can still do fractional scroll but the LCD test setting still holds if it was setting to be true.
On 2014/12/10 21:29:40, Yufeng Shen wrote: > On 2014/12/10 21:18:02, aelias wrote: > > On 2014/12/10 21:14:35, Yufeng Shen wrote: > > > On 2014/12/10 21:03:22, aelias wrote: > > > > OK, rereading the discussion, I realize you're actually planning to > maintain > > > > this codepath for the long run for the corner case of non-composited > > fixed-pos > > > > elements, so this isn't just a staging-purposes temporary hack. Hmm. And > > > > non-composited fixed-pos are kind of a horrible situation to begin with so > > > this > > > > is just piling on another hack onto that particular heap. > > > > > > > > As I recall, the reason we have non-composited fixed-pos at all is because > > of > > > > LCD text. On the other hand, the main reason we want fractional scrolling > > is > > > > because of high-DPI screen. Those are strictly separate configurations. > So > > > > instead of explicitly checking for fixed-pos here, what would you think > > about > > > > disabling fractional scroll across the board when LCD text setting is > > enabled? > > > > > > > I think that would be more maintainable. > > > > > > hmm, but I feel "explicitly checking for fixed pos" is kind of better. Since > I > > > > > know if fixed-pos element exists and does not have a composited layer, then > > > fractional scroll does not play well. Instead, if I check LCD text setting, > > the > > > implication from "LCD text setting" -> "fixed-pos element is composited or > > not" > > > might change. WDYT ? > > > > I don't think that implication can ever change. It's a fundamental > theoretical > > problem with LCD text that it can force noncomposited elements. So I think > the > > static setting is cleaner. > > How about the fixed-pos elements are invisible ? In that case, we can > still do fractional scroll but the LCD test setting still holds if it was > setting to be true. I mean, turning off fracitonal scrolling in the case of low-dpi device definitely covers more cases than just testing if there is visible non-composited fixed-pos elements. It is just that we have case like, on low-dpi, there is no fixed-pos elements, or the fixed position elements are invisible, do we want to enable main thread fractional scrolling. Maybe that does not add much value since it is low-dpi anyway.
I think it's better to be consistent and predictable and draw a hard-line about the condition when fractional scrolling is enabled, than to do a best-effort that makes the situation complex and confusing. Webdevs will more easily understand if we tell them "you will get fractional scrolling if and only if you're on a high-dpi screen"
On 2014/12/10 21:44:04, aelias wrote: > I think it's better to be consistent and predictable and draw a hard-line about > the condition when fractional scrolling is enabled, than to do a best-effort > that makes the situation complex and confusing. Webdevs will more easily > understand if we tell them "you will get fractional scrolling if and only if > you're on a high-dpi screen" Note that this isn't true starting in Chrome 39 where we first exposed fractional scroll offsets to web sites in browser zoom scenarios: https://code.google.com/p/chromium/issues/detail?id=373731. This is similar to how getBoundingClientRect and some other DOM APIs return fractional values so apps can do accurate measurement etc. But my top priority is definitely high-dpi scenarios. If there's good reasons to disable the feature we've already shipped in M39 and limit fractional scroll offsets to just high-dpi then I'm not opposed to that. That said, I don't think it's sufficient to say high-dpi can always do fractional scrolling. We still have some repaint-on-scroll corner cases I believe, at least on desktop. Eg. background-attachment: fixed. Also I believe clip-path still causes accelerated overflow scrolling to be disabled on Android. So trying to make the requirement that high-dpi implies composited scrolling is definitely a bigger problem than what Yufeng is working on here. Ian, WDYT?
On 2014/12/10 22:01:06, Rick Byers wrote: > On 2014/12/10 21:44:04, aelias wrote: > > I think it's better to be consistent and predictable and draw a hard-line > about > > the condition when fractional scrolling is enabled, than to do a best-effort > > that makes the situation complex and confusing. Webdevs will more easily > > understand if we tell them "you will get fractional scrolling if and only if > > you're on a high-dpi screen" > > Note that this isn't true starting in Chrome 39 where we first exposed > fractional scroll offsets to web sites in browser zoom scenarios: > https://code.google.com/p/chromium/issues/detail?id=373731. This is similar to > how getBoundingClientRect and some other DOM APIs return fractional values so > apps can do accurate measurement etc. > > But my top priority is definitely high-dpi scenarios. If there's good reasons > to disable the feature we've already shipped in M39 and limit fractional scroll > offsets to just high-dpi then I'm not opposed to that. > > That said, I don't think it's sufficient to say high-dpi can always do > fractional scrolling. We still have some repaint-on-scroll corner cases I > believe, at least on desktop. Eg. background-attachment: fixed. Also I believe > clip-path still causes accelerated overflow scrolling to be disabled on Android. > So trying to make the requirement that high-dpi implies composited scrolling is > definitely a bigger problem than what Yufeng is working on here. Ian, WDYT? This is tricky. I appreciate Alex's desire for consistency here, but Rick's right about composited scrolling; it's not precisely correlated with high DPI. In addition to the cases where we cannot currently support composited scrolling in high DPI, last we checked, there had been non-trivial number of users that had explicitly opted into composited scrolling on lowDPI.
Alex, I changed to checking preferCompositingToLCDTextEnabled() for if we want to send integer or floating point scroll offset to CC. We are taking a more conservative approach. On low-dpi device main thread fractional scrolling probably does not add too much benefit so we can just rule out this case, and focus more on high-dpi scenarios.\ PTAL.
OK, you convinced me the situation with respect to fractional scroll is complicated enough already that this patch doesn't make it that much worse, and also that background-position: fixed is a case that comes up even in high-dpi, so lgtm the non-layer-viewport-constrained approach after all. Note though I don't think the problem for moving to a low-dpi check is related to impl-thread scrolling per se. The key thing is whether high-dpi implies the nonexistence of nonlayer viewport constrained objects. Those are the ones that would end up jittering with fractional scrolls, regardless of the thread handling the scroll. So I think the blocker for my proposal is specifically background-attachment: fixed, and the other issues mentioned are not? FWIW I consider nonlayer viewport constrained objects to be basically evil and would like never to encounter them on Android again. I would like to see us in the future fix background-attachment: fixed to always be composited on high-dpi. Once we've done that, we can add a DCHECK that high-dpi implies no nonlayer viewport constrained objects.
The CQ bit was checked by miletus@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/747903005/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187073 |