Bug 121876

Summary:

REGRESSION (r154614): Setting the document scroll position isn't symmetric; can successfully set document.body.scrollTop, but can only read from document.documentElement.scrollTop

Product:

WebKit

Reporter:

Ricky Mondello <rmondello>

Component:

DOM

Assignee:

gur.trio

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

ap, bdakin, buildbot, cmarcelo, commit-queue, darin, eflews.bot, esprehn+autocc, fred.wang, gtk-ews, gur.trio, gyuyoung.kim, hyatt, jberlin, kangil.han, rego+ews, rniwa, tonikitoo, webkit-bug-importer, webkit-ews, xan.lopez

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

http://maisqi.com/outros/bugs/chrome/CHN6

Bug Depends on:

106133

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion	none
Patch	none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion	none
Patch	none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion	none
Patch	none

Ricky Mondello

Reported 2013-09-24 15:55:01 PDT

Using a WebKit nighty or a ToT build, load a page whose height is sufficient for playing around with scrollTop in Web Inspector, like http://www.w3.org/html/wg/drafts/html/master/ After r154614, `document.body.scrollTop = 400;` will scroll the document, but turning around to read from `document.body.scrollTop` always returns 0. Reading `document.documentElement.scrollTop` gives the document's actual scroll offset at any given time, but setting it with `document.documentElement.scrollTop = 800;`does nothing. This asymmetry doesn't seem right. Whether it's document.body.scrollTop or document.documentElement.scrollTop which actually sets and gets the document's scrollTop, it should be symmetric. For reference: IE10 can read/write document.documentElement.scrollTop Firefox 23.0.1 can read/write document.documentElement.scrollTop Chrome (27.0.1453.93) can read/write document.body.scrollTop Safari (6.0.5) can read/write document.body.scrollTop WebKit ToT (after r154614): document.body.scrollTop = 400; // works document.body.scrollTop; // always returns 0 document.documentElement.scrollTop; // works for reading the value document.documentElement.scrollTop = 500; // doesn't do anything

Attachments
Patch (24.99 KB, patch) 2013-09-25 06:02 PDT, gur.trio	no flags	Details Formatted Diff Diff
Patch (25.08 KB, patch) 2013-09-25 06:44 PDT, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (728.44 KB, application/zip) 2013-09-25 07:31 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (750.40 KB, application/zip) 2013-09-25 07:56 PDT, Build Bot	no flags	Details
Patch (25.55 KB, patch) 2013-09-25 08:04 PDT, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (526.02 KB, application/zip) 2013-09-25 08:30 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (483.28 KB, application/zip) 2013-09-25 11:12 PDT, Build Bot	no flags	Details
Patch (25.72 KB, patch) 2013-09-26 01:08 PDT, gur.trio	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (539.51 KB, application/zip) 2013-09-26 02:05 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (551.52 KB, application/zip) 2013-09-26 02:21 PDT, Build Bot	no flags	Details
Patch (25.81 KB, patch) 2013-09-27 04:29 PDT, gur.trio	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Ricky Mondello

Comment 1 2013-09-24 16:06:08 PDT

Tracking a related issue at <rdar://problem/14931862>.

Alexey Proskuryakov

Comment 2 2013-09-24 22:13:19 PDT

This issue is now tracked as <rdar://problem/15073848>.

gur.trio

Comment 3 2013-09-24 22:53:09 PDT

(In reply to comment #2) > This issue is now tracked as <rdar://problem/15073848>. Quirks mode should set scrolltop/scrollleft through document.documentElement.scrollTop/document.documentElement.scrollLeft Non-Quirks mode should set scrolltop/scrollleft through document.body.scrollTop/document.body.scrollLeft Other combinations should not scroll. Please comfirm.

gur.trio

Comment 4 2013-09-25 05:11:47 PDT

(In reply to comment #3) > (In reply to comment #2) > > This issue is now tracked as <rdar://problem/15073848>. > > Quirks mode should set scrolltop/scrollleft through document.documentElement.scrollTop/document.documentElement.scrollLeft > > Non-Quirks mode should set scrolltop/scrollleft through document.body.scrollTop/document.body.scrollLeft > > Other combinations should not scroll. > > Please comfirm. Please ignore the previous comment Non-Quirks mode should set/get scrolltop/scrollleft through document.documentElement.scrollTop/document.documentElement.scrollLeft Quirks mode should set/get scrolltop/scrollleft through document.body.scrollTop/document.body.scrollLeft Other combinations should not scroll. Please confirm.

gur.trio

Comment 5 2013-09-25 06:02:38 PDT

Created attachment 212564 [details] Patch

EFL EWS Bot

Comment 6 2013-09-25 06:07:20 PDT

Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/2235138

EFL EWS Bot

Comment 7 2013-09-25 06:07:45 PDT

Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/2245041

kov's GTK+ EWS bot

Comment 8 2013-09-25 06:07:57 PDT

Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/2212029

Early Warning System Bot

Comment 9 2013-09-25 06:13:45 PDT

Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/2261034

Early Warning System Bot

Comment 10 2013-09-25 06:15:34 PDT

Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/2277003

Build Bot

Comment 11 2013-09-25 06:27:23 PDT

Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2162190

Build Bot

Comment 12 2013-09-25 06:42:08 PDT

Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2281003

gur.trio

Comment 13 2013-09-25 06:44:51 PDT

Created attachment 212568 [details] Patch

Build Bot

Comment 14 2013-09-25 07:31:48 PDT

Comment on attachment 212568 [details] Patch Attachment 212568 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2288011 New failing tests: fast/multicol/scrolling-overflow.html

Build Bot

Comment 15 2013-09-25 07:31:50 PDT

Created attachment 212576 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 16 2013-09-25 07:56:21 PDT

Comment on attachment 212568 [details] Patch Attachment 212568 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2166232 New failing tests: fast/multicol/scrolling-overflow.html

Build Bot

Comment 17 2013-09-25 07:56:24 PDT

Created attachment 212578 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5

gur.trio

Comment 18 2013-09-25 08:04:19 PDT

Created attachment 212580 [details] Patch

Build Bot

Comment 19 2013-09-25 08:30:12 PDT

Comment on attachment 212580 [details] Patch Attachment 212580 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2289007 New failing tests: fast/multicol/scrolling-overflow.html

Build Bot

Comment 20 2013-09-25 08:30:15 PDT

Created attachment 212583 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 21 2013-09-25 11:12:44 PDT

Comment on attachment 212580 [details] Patch Attachment 212580 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2287054 New failing tests: fast/multicol/scrolling-overflow.html

Build Bot

Comment 22 2013-09-25 11:12:49 PDT

Created attachment 212606 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Darin Adler

Comment 23 2013-09-25 15:43:26 PDT

Comment on attachment 212580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212580&action=review r=me if you address the failing test > Source/WebCore/dom/Element.cpp:807 > + RenderView& renderView = *document().renderView(); This is only used inside the if statement. So this should be moved inside the if statement. > Source/WebCore/dom/Element.cpp:811 > + FrameView& view = renderView.frameView(); This local variable doesn’t add anything. I suggest getting rid of it. > Source/WebCore/dom/Element.cpp:829 > + RenderView& renderView = *document().renderView(); This is only used inside the if statement. So this should be moved inside the if statement. > LayoutTests/fast/multicol/scrolling-overflow.html:1 > -<!DOCTYPE html> > <html> Looks like this test is failing in the EWS bot.

gur.trio

Comment 24 2013-09-26 01:08:34 PDT

Created attachment 212675 [details] Patch

gur.trio

Comment 25 2013-09-26 01:11:28 PDT

(In reply to comment #23) > (From update of attachment 212580 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212580&action=review > > r=me if you address the failing test > > > Source/WebCore/dom/Element.cpp:807 > > + RenderView& renderView = *document().renderView(); > > This is only used inside the if statement. So this should be moved inside the if statement. > > > Source/WebCore/dom/Element.cpp:811 > > + FrameView& view = renderView.frameView(); > > This local variable doesn’t add anything. I suggest getting rid of it. > > > Source/WebCore/dom/Element.cpp:829 > > + RenderView& renderView = *document().renderView(); > > This is only used inside the if statement. So this should be moved inside the if statement. > > > LayoutTests/fast/multicol/scrolling-overflow.html:1 > > -<!DOCTYPE html> > > <html> > > Looks like this test is failing in the EWS bot. Hi Darin. Thanks for the review. I added FrameView& view = renderView.frameView(); because twice we need frameview once to setScrollPosition and once for scrollY/scrollX so thought of using as a local variable.

gur.trio

Comment 26 2013-09-26 01:17:42 PDT

(In reply to comment #25) > (In reply to comment #23) > > (From update of attachment 212580 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=212580&action=review > > > > r=me if you address the failing test > > > > > Source/WebCore/dom/Element.cpp:807 > > > + RenderView& renderView = *document().renderView(); > > > > This is only used inside the if statement. So this should be moved inside the if statement. > > > > > Source/WebCore/dom/Element.cpp:811 > > > + FrameView& view = renderView.frameView(); > > > > This local variable doesn’t add anything. I suggest getting rid of it. > > > > > Source/WebCore/dom/Element.cpp:829 > > > + RenderView& renderView = *document().renderView(); > > > > This is only used inside the if statement. So this should be moved inside the if statement. > > > > > LayoutTests/fast/multicol/scrolling-overflow.html:1 > > > -<!DOCTYPE html> > > > <html> > > > > Looks like this test is failing in the EWS bot. > > Hi Darin. Thanks for the review. > I added FrameView& view = renderView.frameView(); because twice we need frameview once to setScrollPosition and once for scrollY/scrollX so thought of using as a local variable. The failing test case shows a difference in the dump render tree rects of html, body which should not happen because the height of div element is specified in the test case as 300 and also my changes would not affect the height. Expected: RenderBlock {HTML} at (0,0) size 800x316 Actual : RenderBlock {HTML} at (0,0) size 800x585

Build Bot

Comment 27 2013-09-26 02:05:14 PDT

Comment on attachment 212675 [details] Patch Attachment 212675 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2397007 New failing tests: fast/multicol/scrolling-overflow.html

Build Bot

Comment 28 2013-09-26 02:05:19 PDT

Created attachment 212678 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Build Bot

Comment 29 2013-09-26 02:21:35 PDT

Comment on attachment 212675 [details] Patch Attachment 212675 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2423006 New failing tests: fast/multicol/scrolling-overflow.html

Build Bot

Comment 30 2013-09-26 02:21:40 PDT

Created attachment 212681 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Antonio Gomes

Comment 31 2013-09-26 06:00:31 PDT

*** Bug 121937 has been marked as a duplicate of this bug. ***

Darin Adler

Comment 32 2013-09-26 09:25:00 PDT

Comment on attachment 212675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212675&action=review Looks like the test is still failing. > LayoutTests/fast/multicol/scrolling-overflow.html:7 > - -webkit-column-width: 200px; > + -webkit-column-width: 200px; Why this whitespace change?

gur.trio

Comment 33 2013-09-26 10:03:36 PDT

(In reply to comment #32) > (From update of attachment 212675 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212675&action=review > > Looks like the test is still failing. > > > LayoutTests/fast/multicol/scrolling-overflow.html:7 > > - -webkit-column-width: 200px; > > + -webkit-column-width: 200px; > > Why this whitespace change? The whitespace is by mistake.Will make the changes. As mentioned not sure why the test case is failing.The expected.txt html rect's height and body rect's height donot match the actual.txt. But my changes shouldnot affect those.

Alexey Proskuryakov

Comment 34 2013-09-26 10:37:43 PDT

Does this test fail for you with this patch locally? It's almost certain that the patch actually changes its results.

gur.trio

Comment 35 2013-09-26 10:48:31 PDT

(In reply to comment #34) > Does this test fail for you with this patch locally? It's almost certain that the patch actually changes its results. Will run the test case locally with and withoutthe patch and confirm.

gur.trio

Comment 36 2013-09-27 04:29:31 PDT

Created attachment 212795 [details] Patch

gur.trio

Comment 37 2013-09-27 07:46:16 PDT

(In reply to comment #36) > Created an attachment (id=212795) [details] > Patch Removing the <!DOCTYPE html> was creating some issues and hence the test case was failing. So have modified the test case accordingly.

Jessie Berlin

Comment 38 2013-09-27 08:33:52 PDT

<rdar://problem/15073848>

WebKit Commit Bot

Comment 39 2013-09-28 09:09:21 PDT

Comment on attachment 212795 [details] Patch Clearing flags on attachment: 212795 Committed r156605: <http://trac.webkit.org/changeset/156605>

WebKit Commit Bot

Comment 40 2013-09-28 09:09:27 PDT

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 41 2013-10-16 00:02:44 PDT

The original patch also caused https://bugs.webkit.org/show_bug.cgi?id=122882.

gur.trio

Comment 42 2014-03-06 07:40:34 PST

Changes reverted so re-opening the bug.

Antonio Gomes

Comment 43 2014-03-06 07:43:55 PST

I believe we this duplicate this against bug 106133, and do both setter and getter together (one patch).

Frédéric Wang (:fredw)

Comment 44 2017-04-20 07:35:00 PDT

(In reply to Antonio Gomes from comment #43) > I believe we this duplicate this against bug 106133, and do both setter and > getter together (one patch). What about marked this duplicate of 5991 and continue the work there?

Frédéric Wang (:fredw)

Comment 45 2017-04-21 01:58:38 PDT

*** This bug has been marked as a duplicate of bug 5991 ***

Note You need to log in before you can comment on or make changes to this bug.