Bug 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
Summary: REGRESSION (r154614): Setting the document scroll position isn't symmetric; c...
Status: RESOLVED DUPLICATE of bug 5991
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: gur.trio
URL: http://maisqi.com/outros/bugs/chrome/...
Keywords: InRadar
: 121937 (view as bug list)
Depends on: 106133
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-24 15:55 PDT by Ricky Mondello
Modified: 2017-04-21 01:58 PDT (History)
21 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ricky Mondello 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
Comment 1 Ricky Mondello 2013-09-24 16:06:08 PDT
Tracking a related issue at <rdar://problem/14931862>.
Comment 2 Alexey Proskuryakov 2013-09-24 22:13:19 PDT
This issue is now tracked as <rdar://problem/15073848>.
Comment 3 gur.trio 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.
Comment 4 gur.trio 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.
Comment 5 gur.trio 2013-09-25 06:02:38 PDT
Created attachment 212564 [details]
Patch
Comment 6 EFL EWS Bot 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
Comment 7 EFL EWS Bot 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
Comment 8 kov's GTK+ EWS bot 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
Comment 9 Early Warning System Bot 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
Comment 10 Early Warning System Bot 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
Comment 11 Build Bot 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
Comment 12 Build Bot 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
Comment 13 gur.trio 2013-09-25 06:44:51 PDT
Created attachment 212568 [details]
Patch
Comment 14 Build Bot 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
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Build Bot 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
Comment 18 gur.trio 2013-09-25 08:04:19 PDT
Created attachment 212580 [details]
Patch
Comment 19 Build Bot 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
Comment 20 Build Bot 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
Comment 21 Build Bot 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
Comment 22 Build Bot 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
Comment 23 Darin Adler 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.
Comment 24 gur.trio 2013-09-26 01:08:34 PDT
Created attachment 212675 [details]
Patch
Comment 25 gur.trio 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.
Comment 26 gur.trio 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
Comment 27 Build Bot 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
Comment 28 Build Bot 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
Comment 29 Build Bot 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
Comment 30 Build Bot 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
Comment 31 Antonio Gomes 2013-09-26 06:00:31 PDT
*** Bug 121937 has been marked as a duplicate of this bug. ***
Comment 32 Darin Adler 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?
Comment 33 gur.trio 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.
Comment 34 Alexey Proskuryakov 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.
Comment 35 gur.trio 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.
Comment 36 gur.trio 2013-09-27 04:29:31 PDT
Created attachment 212795 [details]
Patch
Comment 37 gur.trio 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.
Comment 38 Jessie Berlin 2013-09-27 08:33:52 PDT
<rdar://problem/15073848>
Comment 39 WebKit Commit Bot 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>
Comment 40 WebKit Commit Bot 2013-09-28 09:09:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 Ryosuke Niwa 2013-10-16 00:02:44 PDT
The original patch also caused https://bugs.webkit.org/show_bug.cgi?id=122882.
Comment 42 gur.trio 2014-03-06 07:40:34 PST
Changes reverted so re-opening the bug.
Comment 43 Antonio Gomes 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).
Comment 44 Frédéric Wang (:fredw) 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?
Comment 45 Frédéric Wang (:fredw) 2017-04-21 01:58:38 PDT

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