Issue 51553002: Fix body.scrollTop/Left for scrollable <body> tags

Issue 51553002: Fix body.scrollTop/Left for scrollable <body> tags (Closed)

Created:
7 years, 1 month ago by tonikitoo_

Modified:
6 years, 10 months ago

Reviewers:
Julien - ping for review, eseidel

CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, adamk+blink_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Fix body.scrollTop/Left for scrollable <body> tags Blink was erroneously not setting (and getting) the scrollTop and scrollLeft properties of a scrollable body element. As per the spec [1], if an element (including body) has a scrollable area, being it overflown or not, set and get scroll{Top,Left} should apply to the element itself, in both quirks and strict modes. What Blink used to do instead was to apply these properties to the document viewport. Patch changes it to now match Gecko in that regard. - Note: "scrollable body" above actually refers to a body element that has a scrollable area which differs from the viewport's scrollable area, if any. - Note2: scrollTop/Left were already working properly for other elements (see dom/Element.cpp), but only body differs. Additionally, the patch removes a couple of out of date comments in Element::setScrollTop and Element::setScrollTop, after https://www.w3.org/Bugs/Public/show_bug.cgi?id=23448 got fixed. [1] http://dev.w3.org/csswg/cssom-view/#dom-element-scrolltop Test: fast/dom/Element/scrollTop-scrollLeft-body.html BUG=312435 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165969

Patch Set 1 #

Total comments: 5

Patch Set 2 : #

Total comments: 3

Patch Set 3 : #

Created: 6 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+296 lines, -5 lines)			Patch
A	LayoutTests/fast/dom/Element/resources/display-none-body-quirks.html	View		1 chunk	+15 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/resources/display-none-body-strict.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/resources/overflow-hidden-scrollable-body-quirks.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/resources/overflow-hidden-scrollable-body-strict.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/resources/overflow-scroll-non-scrollable-body-quirks.html	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/resources/overflow-scroll-non-scrollable-body-strict.html	View		1 chunk	+15 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/resources/overflow-scroll-scrollable-body-quirks.html	View		1 chunk	+15 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/resources/overflow-scroll-scrollable-body-strict.html	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html	View	1 2	1 chunk	+88 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body-expected.txt	View	1	1 chunk	+49 lines, -0 lines	0 comments	Download
M	Source/core/html/HTMLBodyElement.cpp	View	1 2	5 chunks	+38 lines, -5 lines	0 comments	Download

Messages

Total messages: 13 (0 generated)

Expand Messages | Collapse Messages

Julien - ping for review

As said on IRC, I would like to see a runtime flag. The more changes ...

7 years, 1 month ago (2013-11-01 16:49:14 UTC) #2

tonikitoo_

On 2013/11/01 16:49:14, Julien Chaffraix - PST wrote: > As said on IRC, I would ...

7 years, 1 month ago (2013-11-04 18:13:45 UTC) #3

tonikitoo_

On 2013/11/01 16:49:14, Julien Chaffraix - AEDT wrote: > https://codereview.chromium.org/51553002/diff/1/LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html > File LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html (right): > ...

7 years, 1 month ago (2013-11-14 14:47:05 UTC) #4

tonikitoo_

On 2013/11/14 14:47:05, tonikitoo_ wrote: > On 2013/11/01 16:49:14, Julien Chaffraix - AEDT wrote: > ...

6 years, 11 months ago (2014-01-23 20:01:52 UTC) #8

On 2013/11/14 14:47:05, tonikitoo_ wrote:
> On 2013/11/01 16:49:14, Julien Chaffraix - AEDT wrote: 
>
https://codereview.chromium.org/51553002/diff/1/Source/core/html/HTMLBodyElem...
> > File Source/core/html/HTMLBodyElement.cpp (right):
>
https://codereview.chromium.org/51553002/diff/1/Source/core/html/HTMLBodyElem...
> > Source/core/html/HTMLBodyElement.cpp:221: if (rend->hasOverflowClip())
> > FYI if <body> is the document element, this will be always true. Maybe we
> should
> > test that.
> 
> Would you mind to share an example where html and body are the same? Maybe I
> misunderstood you.

Back to this! For the record, a while ago, Julien and I discussed it on IRC:
(...)
Nov 17 20:58:36 <tonikitoo>	I am mostly puzzled by this one:
Nov 17 20:58:37 <tonikitoo>	> FYI if <body> is the document element, this will
be always true. Maybe we
Nov 17  <tonikitoo>	should
Nov 17  <tonikitoo>	> test that.
Nov 17  <jchaffraix>	tonikitoo: so we have some logic in
RenderBox::updateFromStyle
Nov 17  <jchaffraix>	where <body> would automatically get an overflow clip (see
the comment in there)
Nov 17  *	tonikitoo looks
Nov 17  <jchaffraix>	I don't think I did a good job at summarizing this case (I
don't think it's possible for <body> to be the document element with the HTML5
parser)
Nov 17  <tonikitoo>	jchaffraix, it is certainly possible that the body's
overflow is different than the documentElement's overflow
Nov 17  <tonikitoo>	are we in agreement about this part, right?
Nov 17  <tonikitoo>	(/me goes part by part)
Nov 17  <jchaffraix>	tonikitoo: true but it's also possible for a body with
overflow: visible to still have an overflow clip
Nov 17  <jchaffraix>	which is the case I was pointing at
Nov 17  <tonikitoo>	so, from my undestanding your concern seems to be that
hasOverflowClip might not be the best condition to check against
Nov 17  <jchaffraix>	we should check against other browsers and inlcude that in
our testing
Nov 17  <tonikitoo>	anyways, now I know what you are talking about (the
renderbox::updatefromstyle bits). I can undestand and come back to you
(...)

So from my investigation, <body> will only have an overflow clip (that differs
from documentElement's) if

<html style=overflow:hidden|auto|scroll>
<body style=overflow:hidden|auto|scroll>
...
</body>
</html>

So checking for hasOverflowClip is ok here. We match Firefox and Opera
(pre-blink) behaviors with the patch.

As for your other comments:
1- I agree to change the tests structure, but would prefer to do that on a
follow-up, given that other unrelated tests have similar structure.
2- I can check the explicit static_cast need too, but given that other methods
unrelated to this patch would be affected, I would rather also do it as a follow
up.

tonikitoo_

Jan 23 17:30:05 <jchaffraix> tonikitoo: looking .. Jan 23 17:33:29 <jchaffraix> tonikitoo: that sounds reasonnable ...

6 years, 10 months ago (2014-01-28 15:44:33 UTC) #9

Julien - ping for review

lgtm https://codereview.chromium.org/51553002/diff/130001/LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html File LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html (right): https://codereview.chromium.org/51553002/diff/130001/LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html#newcode9 LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html:9: // Need to wait on both inner frames ...

6 years, 10 months ago (2014-01-28 18:03:29 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/51553002/150001

6 years, 10 months ago (2014-01-28 19:35:58 UTC) #11

commit-bot: I haz the power

6 years, 10 months ago (2014-01-28 22:21:49 UTC) #13

Message was sent while issue was closed.

CQ bit was unchecked on CL. Ignoring.

Expand Messages | Collapse Messages