LayoutBox::scrollRectToVisible doesn't respect overflow:hidden property.

LayoutBox::scrollRectToVisible doesn't respect overflow:hidden property.

Previously, we would scrollRectIntoView despite of the overflow:hidden property.
This makes sense for programmatic scrolls but not user scrolls. For example,
a scroll initiated from Element::scrollIntoView should in fact scroll even if
the element has overflow:hidden. The problem is that user scrolls (autoscroll)
and programmatic scrolls have the same codepath.

This patch takes the type of scroll as a param (defaulted to programmatic scroll),
and only scrolls if it's a programmatic scroll, or a user scroll
on a LayoutBox which does not have overflow:hidden set. The default value of the
param ensures that only the "autoscroll" and "FindText" codepath is affected.
May need to set this flag to false for other cases in the future.

This patch also updates the tests that fail because of adding
input::-webkit-scrollbar {
    display:none;
}
to the default style sheet used to render HTML.

BUG=531525
Committed: https://crrev.com/24936947c5a7019a20e269e523dd338e39588c95
Cr-Commit-Position: refs/heads/master@{#352355}

[ymalik, 2015-09-24 14:52:15]

ymalik@chromium.org changed reviewers:
+ bokan@chromium.org

[ymalik, 2015-09-24 14:52:16]

Before scrolling a rect into view, we check if it has overflow set to hidden,
and if it does, don't scroll into view. Input fields are different because they
have a LayoutBlock (where the text goes) which have default overflow:hidden.
This CL overrides the scrollRectToVisible method for the input case to keep
autoscroll on input fields.

Will add tests once I get feedback on the approach.

[ymalik, 2015-09-25 22:23:26]

Added test.

[bokan, 2015-09-25 22:26:26]

Hey, this looks like it'll work but I think we're just papering over some
earlier design mistakes. Lets chat Monday about how to proceed.

https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:528: if (programmaticScroll
|| style()->overflowX() != OHIDDEN || style()->overflowY() != OHIDDEN) {
Hmm...I don't like that the knowledge of when scrolling is allowed or not is
enshrined here and in ScrollableArea (and likely lots of other places). We
should start trying to consolidate it. (this predates you....and me...)

How about this: lets make the ScrollableArea::scrollIntoView method also take
whether the scroll is a user/programmatic scroll. ScrollableArea already has
this notion on most of its other methods and there's an enum ScrollType. From
what I can tell, all the overrides of scrollIntoView call into setScrollPosition
with scrollType=Programmatic so you'd just have to use the parameter there.
Unfortunately, setScrollPosition doesn't do a check whether the area has
overflow:hidden (ScrollableArea::userScroll does - it should eventually be
merged with setScrollPosition anyway, but that's a story for another day) so
you'll have to add that. That should hopefully remove much of the complexity
from this method.

What do you think?

https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:536: } else if (!parentBox
&& canBeProgramaticallyScrolled()) {
canBeProgrammaticallyScrolled seems like it should only matter if your
programmaticScroll param is true.

https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTextControl.cpp (right):

https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTextControl.cpp:84: void
LayoutTextControl::scrollRectToVisible(const LayoutRect& rect, const
ScrollAlignment& alignX, const ScrollAlignment& alignY, bool programmaticScroll)
I'm not up to speed on what LayoutObjects LayoutTextControl creates but I
suspect what we'll really want to do is change it so that it doesn't use
overflow:hidden.

[ymalik, 2015-09-29 17:35:17]

On 2015/09/25 22:26:26, bokan wrote:
> Hey, this looks like it'll work but I think we're just papering over some
> earlier design mistakes. Lets chat Monday about how to proceed.
> 
>
https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:528: if
(programmaticScroll
> || style()->overflowX() != OHIDDEN || style()->overflowY() != OHIDDEN) {
> Hmm...I don't like that the knowledge of when scrolling is allowed or not is
> enshrined here and in ScrollableArea (and likely lots of other places). We
> should start trying to consolidate it. (this predates you....and me...)
> 
> How about this: lets make the ScrollableArea::scrollIntoView method also take
> whether the scroll is a user/programmatic scroll. ScrollableArea already has
> this notion on most of its other methods and there's an enum ScrollType. From
> what I can tell, all the overrides of scrollIntoView call into
setScrollPosition
> with scrollType=Programmatic so you'd just have to use the parameter there.
> Unfortunately, setScrollPosition doesn't do a check whether the area has
> overflow:hidden (ScrollableArea::userScroll does - it should eventually be
> merged with setScrollPosition anyway, but that's a story for another day) so
> you'll have to add that. That should hopefully remove much of the complexity
> from this method.
> 
> What do you think?

I agree with you. I like the idea or propagating the "scrollType" to
ScrollableArea::scrollIntoView.

> 
>
https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:536: } else if (!parentBox
> && canBeProgramaticallyScrolled()) {
> canBeProgrammaticallyScrolled seems like it should only matter if your
> programmaticScroll param is true.
> 
>
https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTextControl.cpp (right):
> 
>
https://codereview.chromium.org/1365853003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTextControl.cpp:84: void
> LayoutTextControl::scrollRectToVisible(const LayoutRect& rect, const
> ScrollAlignment& alignX, const ScrollAlignment& alignY, bool
programmaticScroll)
> I'm not up to speed on what LayoutObjects LayoutTextControl creates but I
> suspect what we'll really want to do is change it so that it doesn't use
> overflow:hidden.

[ymalik (do not use), 2015-09-29 17:52:30]

ymalik@google.com changed reviewers:
+ ymalik@google.com

[ymalik (do not use), 2015-09-29 17:52:31]

@bokan PTAL. I propagated the ScrollType to scroll into view and added a check
for userInputScrollable.

[ymalik (do not use), 2015-09-29 17:56:38]

ymalik@google.com changed reviewers:
+ skobes@chromium.org

[ymalik (do not use), 2015-09-29 18:03:44]

@skobes

Because we set the scrollbar display to none in html.css, some tests under
fast/css/invalidations failed. I believe this is because the call to
"internals.updateStyleAndReturnAffectedElementCount" on input elements now has
to update the scrollbars. I realize that its kind of bad that the writer of a
particular test has to know where this + 2 offset is coming from, but do you
think its fair to do it this way?

[bokan, 2015-09-29 19:29:36]

On 2015/09/29 18:03:44, ymalik wrote:
> @skobes
> 
> Because we set the scrollbar display to none in html.css, some tests under
> fast/css/invalidations failed. I believe this is because the call to
> "internals.updateStyleAndReturnAffectedElementCount" on input elements now has
> to update the scrollbars. I realize that its kind of bad that the writer of a
> particular test has to know where this + 2 offset is coming from, but do you
> think its fair to do it this way?

Maybe it makes more sense to modify DeprecatedPaintLayerScrollableArea to not
create scrollbars at all if the element is an SingleLineTextControl?
Historically, scrollable used to mean "has scrollbars". I think we need some way
to say "Scrollable but doesn't need scrollbars". WDYT skobes@?

[bokan, 2015-09-29 19:42:50]

Digging through the history here, it seems autoscroll is filled with special
cases and hacks. It'd be great if we could eventually get rid of these (like
isDisallowedAutoscroll) and it would help move along root-layer-scrolls. IMO,
this is a good step in that direction. 

I think that the use in TextFinder should also be a UserScroll, can you confirm?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt
(right):

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt:1:
This test insures that if an autoscroll starts from within a scrollable div, it
does not propagate to its non-scrollabe document body. Furthermore, it tests
that if the body has only either overflowX, of overflowY set to hidden, the
scrollable axis actually scrolls. Note that this test is pertaining to
crbug.com/531525.
nit: "of overflowY" -> "or overflowY"? Should this sentence be "if the body has
only one of overflowX or overflowY set to hidden,..."

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html
(right):

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html:46:
<iframe id="partialScrolliFrame"
src="resources/page-with-scrollable-div-overflow-y-hidden-body.html"></iframe>
Could you also copy this test with the only difference being the iframe document
doesn't have overflow-y: hidden but the iframe element has the scrolling
attribute set? e.g. <iframe scrolling="no"></iframe>

I think that's what the isDisallowedAutoscroll method is checking for and the
expectation is that it shouldn't allow user scrolling either (I think). It's
quite ugly and it'll be easier to clean up if we have the tests for it.

Also, this bug seems to repro without an iframe too (if the mainframe is
overflow:hidden but we start autoscrolling a div). Please add that test case as
well.

Thanks.

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:525: } else if (!parentBox
&& canBeProgramaticallyScrolled()) {
I suspect we can remove the canBeProgrammaticallyScrolled condition. If we have
no parentBox then I think the node will be the documentNode so it will always
return true anyway.

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:528: if
(!isDisallowedAutoscroll(ownerElement, frameView)) {
So many special cases...

[skobes, 2015-09-29 20:30:31]

On 2015/09/29 19:29:36, bokan wrote:
> On 2015/09/29 18:03:44, ymalik wrote:
> > @skobes
> > 
> > Because we set the scrollbar display to none in html.css, some tests under
> > fast/css/invalidations failed. I believe this is because the call to
> > "internals.updateStyleAndReturnAffectedElementCount" on input elements now
has
> > to update the scrollbars. I realize that its kind of bad that the writer of
a
> > particular test has to know where this + 2 offset is coming from, but do you
> > think its fair to do it this way?
> 
> Maybe it makes more sense to modify DeprecatedPaintLayerScrollableArea to not
> create scrollbars at all if the element is an SingleLineTextControl?
> Historically, scrollable used to mean "has scrollbars". I think we need some
way
> to say "Scrollable but doesn't need scrollbars". WDYT skobes@?

Since we have a way to say that in CSS, namely overflow:auto and
::-webkit-scrollbar{display:none}, I prefer not to special-case
LayoutTextControlSingleLine in DeprecatedPaintLayerScrollableArea.  It's fine to
update expectations accordingly for tests that use
internals.updateStyleAndReturnAffectedElementCount.

We do need to refactor and consolidate the code for checking scrollability.  I
am planning to do some of this as part of root layer scrolling.

[skobes, 2015-09-29 20:33:23]

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html
(right):

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html:46:
<iframe id="partialScrolliFrame"
src="resources/page-with-scrollable-div-overflow-y-hidden-body.html"></iframe>
On 2015/09/29 19:42:49, bokan wrote:
> I think that's what the isDisallowedAutoscroll method is checking for and the
> expectation is that it shouldn't allow user scrolling either (I think).

I think this is correct, <iframe scrolling="no"> should disable user scrolls but
permit programmatic scrolls.

[ymalik, 2015-09-30 00:39:11]

On 2015/09/29 19:42:50, bokan wrote:
> Digging through the history here, it seems autoscroll is filled with special
> cases and hacks. It'd be great if we could eventually get rid of these (like
> isDisallowedAutoscroll) and it would help move along root-layer-scrolls. IMO,
> this is a good step in that direction. 
> 
> I think that the use in TextFinder should also be a UserScroll, can you
confirm?
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

TextFinder is used by Android. I changed it to UserScroll and tried it out and
seems to be working fine. Though, I am not very familiar with the
user/programmatic scroll semantics on Android. I think we should keep it to the
default value unless a bug related to this comes up. WDYT?
 
>
https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt
> (right):
> 
>
https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt:1:
> This test insures that if an autoscroll starts from within a scrollable div,
it
> does not propagate to its non-scrollabe document body. Furthermore, it tests
> that if the body has only either overflowX, of overflowY set to hidden, the
> scrollable axis actually scrolls. Note that this test is pertaining to
> crbug.com/531525.
> nit: "of overflowY" -> "or overflowY"? Should this sentence be "if the body
has
> only one of overflowX or overflowY set to hidden,..."
> 
>
https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html
> (right):
> 
>
https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html:46:
> <iframe id="partialScrolliFrame"
> src="resources/page-with-scrollable-div-overflow-y-hidden-body.html"></iframe>
> Could you also copy this test with the only difference being the iframe
document
> doesn't have overflow-y: hidden but the iframe element has the scrolling
> attribute set? e.g. <iframe scrolling="no"></iframe>
> 
> I think that's what the isDisallowedAutoscroll method is checking for and the
> expectation is that it shouldn't allow user scrolling either (I think). It's
> quite ugly and it'll be easier to clean up if we have the tests for it.
> 
> Also, this bug seems to repro without an iframe too (if the mainframe is
> overflow:hidden but we start autoscrolling a div). Please add that test case
as
> well.
> 
> Thanks.
> 
>
https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:525: } else if (!parentBox
> && canBeProgramaticallyScrolled()) {
> I suspect we can remove the canBeProgrammaticallyScrolled condition. If we
have
> no parentBox then I think the node will be the documentNode so it will always
> return true anyway.
> 
>
https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:528: if
> (!isDisallowedAutoscroll(ownerElement, frameView)) {
> So many special cases...

[ymalik, 2015-09-30 00:40:11]

On 2015/09/29 20:30:31, skobes wrote:
> On 2015/09/29 19:29:36, bokan wrote:
> > On 2015/09/29 18:03:44, ymalik wrote:
> > > @skobes
> > > 
> > > Because we set the scrollbar display to none in html.css, some tests under
> > > fast/css/invalidations failed. I believe this is because the call to
> > > "internals.updateStyleAndReturnAffectedElementCount" on input elements now
> has
> > > to update the scrollbars. I realize that its kind of bad that the writer
of
> a
> > > particular test has to know where this + 2 offset is coming from, but do
you
> > > think its fair to do it this way?
> > 
> > Maybe it makes more sense to modify DeprecatedPaintLayerScrollableArea to
not
> > create scrollbars at all if the element is an SingleLineTextControl?
> > Historically, scrollable used to mean "has scrollbars". I think we need some
> way
> > to say "Scrollable but doesn't need scrollbars". WDYT skobes@?
> 
> Since we have a way to say that in CSS, namely overflow:auto and
> ::-webkit-scrollbar{display:none}, I prefer not to special-case
> LayoutTextControlSingleLine in DeprecatedPaintLayerScrollableArea.  It's fine
to
> update expectations accordingly for tests that use
> internals.updateStyleAndReturnAffectedElementCount.
> 
> We do need to refactor and consolidate the code for checking scrollability.  I
> am planning to do some of this as part of root layer scrolling.

Okay, I will keep this as is then.

[ymalik, 2015-09-30 00:41:09]

@bokan @skobes

I worked on your comments.

PTAL :)

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt
(right):

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt:1:
This test insures that if an autoscroll starts from within a scrollable div, it
does not propagate to its non-scrollabe document body. Furthermore, it tests
that if the body has only either overflowX, of overflowY set to hidden, the
scrollable axis actually scrolls. Note that this test is pertaining to
crbug.com/531525.
On 2015/09/29 19:42:49, bokan wrote:
> nit: "of overflowY" -> "or overflowY"? Should this sentence be "if the body
has
> only one of overflowX or overflowY set to hidden,..."

Done. Thanks.

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html
(right):

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html:46:
<iframe id="partialScrolliFrame"
src="resources/page-with-scrollable-div-overflow-y-hidden-body.html"></iframe>
On 2015/09/29 19:42:49, bokan wrote:
> Could you also copy this test with the only difference being the iframe
document
> doesn't have overflow-y: hidden but the iframe element has the scrolling
> attribute set? e.g. <iframe scrolling="no"></iframe>
> I think that's what the isDisallowedAutoscroll method is checking for and the
> expectation is that it shouldn't allow user scrolling either (I think). It's
> quite ugly and it'll be easier to clean up if we have the tests for it.

Added autoscroll-upwards-propagation-no-scroll-iframe.html for this.

 
> Also, this bug seems to repro without an iframe too (if the mainframe is
> overflow:hidden but we start autoscrolling a div). Please add that test case
as
> well.

Updated autoscroll-upwards-propagation-overflow-hidden-body.html to cover the
case without an iframe.
Added autoscroll-upwards-propagation-overflow-hidden-iframe-body.html to cover
original case with an iframe.


> Thanks.

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html:46:
<iframe id="partialScrolliFrame"
src="resources/page-with-scrollable-div-overflow-y-hidden-body.html"></iframe>
On 2015/09/29 20:33:23, skobes wrote:
> On 2015/09/29 19:42:49, bokan wrote:
> > I think that's what the isDisallowedAutoscroll method is checking for and
the
> > expectation is that it shouldn't allow user scrolling either (I think).
> 
> I think this is correct, <iframe scrolling="no"> should disable user scrolls
but
> permit programmatic scrolls.

Acknowledged.

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:525: } else if (!parentBox
&& canBeProgramaticallyScrolled()) {
On 2015/09/29 19:42:49, bokan wrote:
> I suspect we can remove the canBeProgrammaticallyScrolled condition. If we
have
> no parentBox then I think the node will be the documentNode so it will always
> return true anyway. 

I would like to keep this CL just to propagate the ScrollType change along with
fixing the overflow bug. Removing the call to canBeProgramaticallyScrolled in
this CL may make it hard to point to the change in case there are bugs. Will
remove it in follow-up CL.

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:528: if
(!isDisallowedAutoscroll(ownerElement, frameView)) {
On 2015/09/29 19:42:49, bokan wrote:
> So many special cases...

Indeed. I will see if I can clean it up in a followup CL.

[bokan, 2015-09-30 12:52:40]

lgtm w/nits.

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:525: } else if (!parentBox
&& canBeProgramaticallyScrolled()) {
On 2015/09/30 00:41:09, ymalik1 wrote:
> On 2015/09/29 19:42:49, bokan wrote:
> > I suspect we can remove the canBeProgrammaticallyScrolled condition. If we
> have
> > no parentBox then I think the node will be the documentNode so it will
always
> > return true anyway. 
> 
> I would like to keep this CL just to propagate the ScrollType change along
with
> fixing the overflow bug. Removing the call to canBeProgramaticallyScrolled in
> this CL may make it hard to point to the change in case there are bugs. Will
> remove it in follow-up CL.

sgtm

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-no-scroll-iframe.html
(right):

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-no-scroll-iframe.html:6:
description("This test insures that if an autoscroll starts from within a " +
nit: s/insures/ensures

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt
(right):

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt:1:
This test insures that if an autoscroll starts from within a scrollable div, it
does not propagate to its non-scrollabe document body. Furthermore, it tests
that if the body has only one of overflowX, or overflowY set to hidden, the
scrollable axis actually scrolls. Note that this test is pertaining to
crbug.com/531525.
Nit: you don't need the comma after overflowX </grammar-police>

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html
(right):

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html:55:
eventSender.mouseMoveTo(x + 795, y + 795);
Cool, you got autoscroll to work on the main frame?

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-iframe-body.html
(right):

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-iframe-body.html:8:
description("This test insures that if an autoscroll starts from within a " +
ditto about ensures/insures

[bokan, 2015-09-30 12:57:42]

One more thing - forgot to reply to this one:

On 2015/09/30 00:39:11, ymalik1 wrote:
> On 2015/09/29 19:42:50, bokan wrote:
> > Digging through the history here, it seems autoscroll is filled with special
> > cases and hacks. It'd be great if we could eventually get rid of these (like
> > isDisallowedAutoscroll) and it would help move along root-layer-scrolls.
IMO,
> > this is a good step in that direction. 
> > 
> > I think that the use in TextFinder should also be a UserScroll, can you
> confirm?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> TextFinder is used by Android. I changed it to UserScroll and tried it out and
> seems to be working fine. Though, I am not very familiar with the
> user/programmatic scroll semantics on Android. I think we should keep it to
the
> default value unless a bug related to this comes up. WDYT?

I think the semantics should be the same for Android and Desktop, we shouldn't
scroll overflow:hidden elements unless the page does it itself (since you can
then get stuck, unable to scroll back). It saddens me that the same feature has
two disparate implementations but such is life. Lets change it to UserScroll
too.

[ymalik, 2015-09-30 14:16:58]

Sorry about the insures/ensures typo. Carelessness at its best :)

Worked on nits.

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1365853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:525: } else if (!parentBox
&& canBeProgramaticallyScrolled()) {
On 2015/09/30 12:52:39, bokan wrote:
> On 2015/09/30 00:41:09, ymalik1 wrote:
> > On 2015/09/29 19:42:49, bokan wrote:
> > > I suspect we can remove the canBeProgrammaticallyScrolled condition. If we
> > have
> > > no parentBox then I think the node will be the documentNode so it will
> always
> > > return true anyway. 
> > 
> > I would like to keep this CL just to propagate the ScrollType change along
> with
> > fixing the overflow bug. Removing the call to canBeProgramaticallyScrolled
in
> > this CL may make it hard to point to the change in case there are bugs. Will
> > remove it in follow-up CL.
> 
> sgtm

Acknowledged.

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-no-scroll-iframe.html
(right):

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-no-scroll-iframe.html:6:
description("This test insures that if an autoscroll starts from within a " +
On 2015/09/30 12:52:40, bokan wrote:
> nit: s/insures/ensures

Done.

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt
(right):

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body-expected.txt:1:
This test insures that if an autoscroll starts from within a scrollable div, it
does not propagate to its non-scrollabe document body. Furthermore, it tests
that if the body has only one of overflowX, or overflowY set to hidden, the
scrollable axis actually scrolls. Note that this test is pertaining to
crbug.com/531525.
On 2015/09/30 12:52:40, bokan wrote:
> Nit: you don't need the comma after overflowX </grammar-police>

Done.

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html
(right):

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html:55:
eventSender.mouseMoveTo(x + 795, y + 795);
On 2015/09/30 12:52:40, bokan wrote:
> Cool, you got autoscroll to work on the main frame?

Yes. the trick is to move the mouse to the scrollbar region.

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-iframe-body.html
(right):

https://codereview.chromium.org/1365853003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-iframe-body.html:8:
description("This test insures that if an autoscroll starts from within a " +
On 2015/09/30 12:52:40, bokan wrote:
> ditto about ensures/insures

Done.

[ymalik, 2015-09-30 14:17:16]

On 2015/09/30 12:57:42, bokan wrote:
> One more thing - forgot to reply to this one:
> 
> On 2015/09/30 00:39:11, ymalik1 wrote:
> > On 2015/09/29 19:42:50, bokan wrote:
> > > Digging through the history here, it seems autoscroll is filled with
special
> > > cases and hacks. It'd be great if we could eventually get rid of these
(like
> > > isDisallowedAutoscroll) and it would help move along root-layer-scrolls.
> IMO,
> > > this is a good step in that direction. 
> > > 
> > > I think that the use in TextFinder should also be a UserScroll, can you
> > confirm?
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > TextFinder is used by Android. I changed it to UserScroll and tried it out
and
> > seems to be working fine. Though, I am not very familiar with the
> > user/programmatic scroll semantics on Android. I think we should keep it to
> the
> > default value unless a bug related to this comes up. WDYT?
> 
> I think the semantics should be the same for Android and Desktop, we shouldn't
> scroll overflow:hidden elements unless the page does it itself (since you can
> then get stuck, unable to scroll back). It saddens me that the same feature
has
> two disparate implementations but such is life. Lets change it to UserScroll
> too.

Fair enough. Done.

[ymalik, 2015-09-30 14:27:50]

ymalik@chromium.org changed reviewers:
+ rbyers@chromium.org

[ymalik, 2015-09-30 14:29:07]

+Rick for Source/platform/ OWNER

[Rick Byers, 2015-09-30 15:11:34]

On 2015/09/30 14:29:07, ymalik1 wrote:
> +Rick for Source/platform/ OWNER

Source/platform LGTM

[ymalik, 2015-09-30 22:21:02]

@skobes

Gentle ping :)

[skobes, 2015-09-30 22:31:14]

https://chromiumcodereview.appspot.com/1365853003/diff/140001/third_party/Web...
File third_party/WebKit/Source/core/css/html.css (right):

https://chromiumcodereview.appspot.com/1365853003/diff/140001/third_party/Web...
third_party/WebKit/Source/core/css/html.css:529: input::-webkit-scrollbar {
Can we constrain this to [type="text"]?

https://chromiumcodereview.appspot.com/1365853003/diff/140001/third_party/Web...
File third_party/WebKit/Source/core/frame/RootFrameViewport.h (right):

https://chromiumcodereview.appspot.com/1365853003/diff/140001/third_party/Web...
third_party/WebKit/Source/core/frame/RootFrameViewport.h:41: ScrollType  =
ProgrammaticScroll) override;
nit: extra space

https://chromiumcodereview.appspot.com/1365853003/diff/140001/third_party/Web...
File third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
(right):

https://chromiumcodereview.appspot.com/1365853003/diff/140001/third_party/Web...
third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp:364:
textBlockStyle->setOverflowX(OSCROLL);
Do we need these two lines, or is the default (OAUTO) sufficient?

https://chromiumcodereview.appspot.com/1365853003/diff/140001/third_party/Web...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/1365853003/diff/140001/third_party/Web...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:216: double x = 
userInputScrollable(HorizontalScrollbar) ? position.x() :
scrollAnimator()->currentPosition().x();
extra space after =

(here and the next line)

https://chromiumcodereview.appspot.com/1365853003/diff/140001/third_party/Web...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:218: DoublePoint
newPosition(x, y);
you can inline this below as "DoublePoint(x, y)"

[ymalik, 2015-10-01 22:25:31]

@skobes PTAL :)

https://codereview.chromium.org/1365853003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/html.css (right):

https://codereview.chromium.org/1365853003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/html.css:529: input::-webkit-scrollbar {
On 2015/09/30 22:31:14, skobes wrote:
> Can we constrain this to [type="text"]?

Spoke to you on hangouts. We would have to constraint it to types that use
LayoutTextControlSingleLine.

https://codereview.chromium.org/1365853003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RootFrameViewport.h (right):

https://codereview.chromium.org/1365853003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RootFrameViewport.h:41: ScrollType  =
ProgrammaticScroll) override;
On 2015/09/30 22:31:14, skobes wrote:
> nit: extra space

Done.

https://codereview.chromium.org/1365853003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
(right):

https://codereview.chromium.org/1365853003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp:364:
textBlockStyle->setOverflowX(OSCROLL);
On 2015/09/30 22:31:14, skobes wrote:
> Do we need these two lines, or is the default (OAUTO) sufficient?

Yes we need these two lines.OAUTO causes the text to go past the width of the
input field. Didn't work for me.

https://codereview.chromium.org/1365853003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1365853003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:216: double x = 
userInputScrollable(HorizontalScrollbar) ? position.x() :
scrollAnimator()->currentPosition().x();
On 2015/09/30 22:31:14, skobes wrote:
> extra space after =
> 
> (here and the next line)

Done.

https://codereview.chromium.org/1365853003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:218: DoublePoint
newPosition(x, y);
On 2015/09/30 22:31:14, skobes wrote:
> you can inline this below as "DoublePoint(x, y)"

Just did FloatPoint(x, y), as toFloatPoint would.

https://codereview.chromium.org/1365853003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
(right):

https://codereview.chromium.org/1365853003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp:354: void
LayoutTextControlSingleLine::addScrollbarPeudoStyle()
Added this method for input fields that default to type="text" because an
invalid type was entered (e.g. type="foo").

I think it makes sense to have the scrollbar hiding logic for valid types in CSS
because doing it here causes additional repaint steps (noticed from failing
LayoutTests when only this function is used).

@skobes WDYT?

[skobes, 2015-10-01 22:40:19]

https://codereview.chromium.org/1365853003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
(right):

https://codereview.chromium.org/1365853003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp:354: void
LayoutTextControlSingleLine::addScrollbarPeudoStyle()
On 2015/10/01 22:25:31, ymalik1 wrote:
> Added this method for input fields that default to type="text" because an
> invalid type was entered (e.g. type="foo").
> 
> I think it makes sense to have the scrollbar hiding logic for valid types in
CSS
> because doing it here causes additional repaint steps (noticed from failing
> LayoutTests when only this function is used).
> 
> @skobes WDYT? 

It would be better to have this in one place instead of two different places. 
Can you clarify where the additional repaints come from?  I thought this would
all run during style recalc, before any painting occurs.

[ymalik, 2015-10-03 20:49:21]

@skobes PTAL :)

https://codereview.chromium.org/1365853003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
(right):

https://codereview.chromium.org/1365853003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp:354: void
LayoutTextControlSingleLine::addScrollbarPeudoStyle()
On 2015/10/01 22:40:19, skobes wrote:
> On 2015/10/01 22:25:31, ymalik1 wrote:
> > Added this method for input fields that default to type="text" because an
> > invalid type was entered (e.g. type="foo").
> > 
> > I think it makes sense to have the scrollbar hiding logic for valid types in
> CSS
> > because doing it here causes additional repaint steps (noticed from failing
> > LayoutTests when only this function is used).
> > 
> > @skobes WDYT? 
> 
> It would be better to have this in one place instead of two different places. 
> Can you clarify where the additional repaints come from?  I thought this would
> all run during style recalc, before any painting occurs.

Removed the logic from html.css

Discussed this in great details with you over email, but the reason for
repainting 
as you pointed out was because we were adding the style to the input element 
and not the inner editor. This caused a discrepancy between the style from 
StyleResolver and this method upon stylechange, marking the input as needs
repainting.

[skobes, 2015-10-05 01:37:24]

lgtm % nits

https://codereview.chromium.org/1365853003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html
(right):

https://codereview.chromium.org/1365853003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html:25:
"scrollable div, it does not propagate to its non-scrollabe document " +
typo: non-scrollable

https://codereview.chromium.org/1365853003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/html.css (right):

https://codereview.chromium.org/1365853003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/html.css:974: :focus {
Since this file only has whitespace changes now, you can revert it.

[ymalik, 2015-10-05 14:25:03]

Worked on nits.

https://codereview.chromium.org/1365853003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html
(right):

https://codereview.chromium.org/1365853003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/autoscroll-upwards-propagation-overflow-hidden-body.html:25:
"scrollable div, it does not propagate to its non-scrollabe document " +
On 2015/10/05 01:37:24, skobes wrote:
> typo: non-scrollable

Done.

https://codereview.chromium.org/1365853003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/html.css (right):

https://codereview.chromium.org/1365853003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/html.css:974: :focus {
On 2015/10/05 01:37:24, skobes wrote:
> Since this file only has whitespace changes now, you can revert it.

Done.

[ymalik, 2015-10-05 14:36:32]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2015-10-05 14:36:32]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
rbyers@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1365853003/#ps220001
(title: "nits")

[commit-bot: I haz the power, 2015-10-05 14:37:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365853003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365853003/220001

[commit-bot: I haz the power, 2015-10-05 14:44:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-05 14:44:53]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ymalik, 2015-10-05 16:35:41]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2015-10-05 16:36:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365853003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365853003/220001

[commit-bot: I haz the power, 2015-10-05 18:20:42]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-10-05 18:21:45]

Patchset 12 (id:??) landed as
https://crrev.com/24936947c5a7019a20e269e523dd338e39588c95
Cr-Commit-Position: refs/heads/master@{#352355}