RESOLVED DUPLICATE of bug 5991 121876
REGRESSION (r154614): Setting the document scroll position isn't symmetric; can successfully set document.body.scrollTop, but can only read from document.documentElement.scrollTop 
https://bugs.webkit.org/show_bug.cgi?id=121876
Summary REGRESSION (r154614): Setting the document scroll position isn't symmetric; c...
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
DetailsFormatted DiffDiff
Patch (25.08 KB, patch)
2013-09-25 06:44 PDT, gur.trio 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
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.