Issue 25039006: document.documentElement.scrollTop/Left is zero

Issue 25039006: document.documentElement.scrollTop/Left is zero (Closed)

Created:
7 years, 2 months ago by tonikitoo_

Modified:
7 years, 2 months ago

Reviewers:
Julien - ping for review

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

document.documentElement.scrollTop/Left is zero Blink, as of today, is the only engine that does not respect the specification with regards to the values returned from scrollTop and scrollLeft properties of documentElement and body nodes. Current behavior is: in both strict and quirks modes, documentElement.scrollTop always returns 0, while body.scrollTop always returns scrollY. The analogy behavior applies to scrollLeft and scrollX (the return). This patch fixes Blink's behavior in accordance to specification [1]: if a document is in strict mode, document.documentElement.scrollTop should return 'scrollY', and document.body.scrollTop should return 0. On the other hand, if it is in quirks mode, document.documentElement should return 0, and document.body.scrollTop should return 'scrollY'. Same analogy applies to scrollLeft property. An optimistic approach was chosen to fix this in the sense of doing this change without collecting usage data first. The rationale behind it is that given that Blink is the only engine behavior'ing differently, relevant WebSites would already have two codepaths. [1] http://dev.w3.org/csswg/cssom-view/#dom-element-scrolltop Test: fast/dom/Element/scrollTop-scrollLeft-strict-quirks-modes.html Existing updated tests: fast/dom/anchor-without-content.html fast/events/mouse-cursor-image-set.html fast/events/mouse-cursor.html fast/events/touch/gesture/touch-gesture-fully-scrolled-iframe-propagates.html fast/events/touch/gesture/touch-gesture-noscroll-body-propagated.html fast/events/touch/gesture/touch-gesture-noscroll-body-xhidden.html fast/events/touch/gesture/touch-gesture-noscroll-body-yhidden.html fast/events/touch/gesture/touch-gesture-scroll-iframe-editable.html fast/events/touch/gesture/touch-gesture-scroll-iframe-not-propagated.html fast/events/touch/gesture/touch-gesture-scroll-page-propagated.html fast/events/touch/gesture/touch-gesture-scroll-page.html fast/scrolling/hover-during-scroll.html http/tests/navigation/resources/frame-with-anchor-same-origin.html http/tests/navigation/resources/frame-with-anchor.html BUG=157855 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158719 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158864

Patch Set 1 : document.documentElement.scrollTop/Left is zero #

Total comments: 14

Patch Set 2 : document.documentElement.scrollTop/Left is zero #

Patch Set 3 : document.documentElement.scrollTop/Left is zero #

Created: 7 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+209 lines, -77 lines)			Patch
M	LayoutTests/fast/css/zoom-body-scroll.html	View		1 chunk	+7 lines, -6 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html	View	1	1 chunk	+17 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html	View	1	1 chunk	+19 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-strict-quirks-modes.html	View	1	1 chunk	+38 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-strict-quirks-modes-expected.txt	View		1 chunk	+19 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/dom/anchor-without-content.html	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/events/mouse-cursor.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/fast/events/mouse-cursor-image-set.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-fully-scrolled-iframe-propagates.html	View		3 chunks	+5 lines, -3 lines	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-fully-scrolled-iframe-propagates-expected.txt	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-noscroll-body-propagated.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-noscroll-body-propagated-expected.txt	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-noscroll-body-xhidden.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-noscroll-body-xhidden-expected.txt	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-noscroll-body-yhidden.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-noscroll-body-yhidden-expected.txt	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-iframe-editable.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-iframe-editable-expected.txt	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-iframe-not-propagated.html	View		3 chunks	+3 lines, -1 line	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-iframe-not-propagated-expected.txt	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-page.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-page-expected.txt	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-page-propagated.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-page-propagated-expected.txt	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/scrolling/hover-during-scroll.html	View		3 chunks	+3 lines, -3 lines	0 comments	Download
M	LayoutTests/fast/scrolling/hover-during-scroll-expected.txt	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	LayoutTests/http/tests/navigation/anchor-frames-expected.txt	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	LayoutTests/http/tests/navigation/anchor-frames-same-origin-expected.txt	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/http/tests/navigation/resources/frame-with-anchor.html	View		2 chunks	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/http/tests/navigation/resources/frame-with-anchor-same-origin.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/platform/mac/fast/events/touch/gesture/touch-gesture-noscroll-body-propagated-expected.txt	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/platform/mac/fast/events/touch/gesture/touch-gesture-noscroll-body-yhidden-expected.txt	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/rubberbanding/momentum-reset.html	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/dom/Element.cpp	View	1 2	2 chunks	+22 lines, -0 lines	0 comments	Download
M	Source/core/html/HTMLBodyElement.cpp	View	1	2 chunks	+12 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (0 generated)

Expand Messages | Collapse Messages

Julien - ping for review

https://codereview.chromium.org/25039006/diff/3001/LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html File LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html (right): https://codereview.chromium.org/25039006/diff/3001/LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html#newcode5 LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html:5: width:9999px; Nit: space. https://codereview.chromium.org/25039006/diff/3001/LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html File LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html (right): https://codereview.chromium.org/25039006/diff/3001/LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html#newcode6 LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html:6: ...

7 years, 2 months ago (2013-09-30 23:39:57 UTC) #1

tonikitoo_

https://codereview.chromium.org/25039006/diff/3001/LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html File LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html (right): https://codereview.chromium.org/25039006/diff/3001/LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html#newcode6 LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html:6: width:9999px; On 2013/09/30 23:39:57, Julien Chaffraix wrote: > Nit: ...

7 years, 2 months ago (2013-10-01 14:42:17 UTC) #2

Julien - ping for review

lgtm with all the requested changes. https://codereview.chromium.org/25039006/diff/3001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/25039006/diff/3001/Source/core/dom/Element.cpp#newcode696 Source/core/dom/Element.cpp:696: return adjustForAbsoluteZoom(view->scrollX(), renderView); ...

7 years, 2 months ago (2013-10-01 15:19:51 UTC) #3

lgtm with all the requested changes.

https://codereview.chromium.org/25039006/diff/3001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/25039006/diff/3001/Source/core/dom/Element.cp...
Source/core/dom/Element.cpp:696: return adjustForAbsoluteZoom(view->scrollX(),
renderView);
On 2013/10/01 14:42:17, a1.gomes wrote:
> On 2013/09/30 23:39:57, Julien Chaffraix wrote:
> > It would be nice to have some zoom testing too.
> 
> Do you think the test this patch edits in
> LayoutTests/fast/css/zoom-body-scroll.html is enough?

I missed this test which exercise this code, my bad. By the way, it is missing
an expected result so you won't be able to land this change as-is.

https://codereview.chromium.org/25039006/diff/3001/Source/core/html/HTMLBodyE...
File Source/core/html/HTMLBodyElement.cpp (right):

https://codereview.chromium.org/25039006/diff/3001/Source/core/html/HTMLBodyE...
Source/core/html/HTMLBodyElement.cpp:282: return 0;
On 2013/10/01 14:42:17, a1.gomes wrote:
> On 2013/09/30 23:39:57, Julien Chaffraix wrote:
> > Is this totally correct?
> > 
> > Step 4 of scrollTop says: "Return the y-coordinate of the scrolling area at
> the
> > alignment point with the top of the padding edge of the element."
> 
> This is a valid observation! I believe the specification is not clear about
> this, or it lacks a "return zero and terminate these steps" advice.
> 
> The way I implemented it makes Blink to match Firefox, IE, Safari nightly and
> Opera (pre-blink) for this case, as it can be seen on
> http://13thparallel.com/archive/viewport/example6.htm . Firefox implementation
> is explicit about it:
> http://lxr.mozilla.org/mozilla-central/source/content/base/src/Element.cpp#510
.
> 
> We could maybe chose to match other engines, and clarify with Hixie the spec.

Clarifying the specification is definitely the way to go. It shouldn't block
this change though (you can put a comment linking to the thread or just a
FIXME).

tonikitoo_

> > > > We could maybe chose to match other engines, and clarify with ...

7 years, 2 months ago (2013-10-01 16:08:16 UTC) #4

tonikitoo_

https://codereview.chromium.org/25039006/diff/3001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/25039006/diff/3001/Source/core/dom/Element.cpp#newcode696 Source/core/dom/Element.cpp:696: return adjustForAbsoluteZoom(view->scrollX(), renderView); On 2013/10/01 15:19:51, Julien Chaffraix wrote: ...

7 years, 2 months ago (2013-10-01 16:49:19 UTC) #5