Bug 106133

Summary: document.body.scrollTop & document.documentElement.scrollTop differ cross-browser
Product: WebKit Reporter: Vince Malone <vince.t.malone>
Component: New BugsAssignee: gur.trio
Status: RESOLVED DUPLICATE    
Severity: Normal CC: adele, ap, buildbot, claude.pache, cmarcelo, commit-queue, darin, dbates, diego.perini, dvoytenko, eflews.bot, esprehn+autocc, fred.wang, gtk-ews, gur.trio, gyuyoung.kim, kangil.han, mathias, moz, philn, rbyers, rego+ews, rniwa, simon.fraser, syoichi, tonikitoo, webkit-bug-importer, webkit-ews, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
URL: http://jsfiddle.net/CbYgk/
Bug Depends on: 121937    
Bug Blocks: 121876, 122882    
Attachments:
Description Flags
WIP Patch
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-13 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-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Patch
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Vince Malone 2013-01-04 14:42:55 PST
Chrome 23 & Safari 6
  document.body.scrollTop - returns the scrollbar location on screen
  document.documentElement.scrollTop - always returns 0

Firefox 17, Opera 12 & IE 10
  document.body.scrollTop - always returns 0
  document.documentElement.scrollTop - returns the scrollbar location on screen

Meaning, webkit vs non-webkit browser swap the values returned.

Here's an interactive example, try it in different browers: http://jsfiddle.net/CbYgk/

sidenote: window.pageYOffset - returns the scrollbar location on screen on all listed browsers.
Comment 1 gur.trio 2013-08-19 07:04:25 PDT
Created attachment 209083 [details]
WIP Patch
Comment 2 Build Bot 2013-08-19 07:30:00 PDT
Comment on attachment 209083 [details]
WIP Patch

Attachment 209083 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1516200

New failing tests:
fast/css/zoom-body-scroll.html
fast/events/mouse-cursor.html
http/tests/navigation/anchor-frames-gbk.html
platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html
Comment 3 Build Bot 2013-08-19 07:30:02 PDT
Created attachment 209085 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 4 Darin Adler 2013-08-19 09:03:17 PDT
Comment on attachment 209083 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209083&action=review

> Source/WebCore/ChangeLog:7
> +        No new tests (OOPS!).

WebKit changes that fix bugs must come with tests that cover the new behavior. Code change looks OK, but need test cases demonstrating both quirks mode and strict mode behavior.

> Source/WebCore/dom/Element.cpp:780
>      document()->updateLayoutIgnorePendingStylesheets();
> +    bool inQuirksMode = document()->inQuirksMode();     
> +
> +    if (document()->documentElement() == this) {
> +        if (!inQuirksMode) {
> +            if (FrameView* view = document()->view()) {
> +                if (RenderView* renderView = document()->renderView())
> +                    return adjustForAbsoluteZoom(view->scrollX(), renderView);
> +            }
> +        } else
> +            return 0;
> +    }
>      if (RenderBox* rend = renderBox())
>          return adjustForAbsoluteZoom(rend->scrollLeft(), rend);
>      return 0;

A few small improvements are possible here; I suggest doing one or more of these:

First, if we are in quirks mode and this is the document element, it would be nice to not call updateLayoutIgnorePendingStylesheets at all, since there's no need to do that just to return 0.
Second, We normally prefer "early return", so it would be more like this:

    if (inQuirksMode)
        return 0;
    // continue with strict mode case

Third, I think it's slightly more elegant to get the FrameView* from the RenderView* rather than getting both from the Document. Sort of “move over to the view system” first, and then get the relevant objects within the view system rather than going from model to view twice in a row.

All of these improvements could apply to scrollTop as well.

> Source/WebCore/html/HTMLBodyElement.cpp:270
>      Document* document = this->document();
>      document->updateLayoutIgnorePendingStylesheets();
>      FrameView* view = document->view();
> -    return view ? adjustForZoom(view->scrollX(), document) : 0;
> +    if (document->inQuirksMode())
> +        return view ? adjustForZoom(view->scrollX(), document) : 0;
> +    return 0;

We should restructure the code so it does not do updateLayoutIgnorePendingStylesheets if it is just going to return 0. Same in scrollTop.
Comment 5 Build Bot 2013-08-19 17:54:07 PDT
Comment on attachment 209083 [details]
WIP Patch

Attachment 209083 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1515403

New failing tests:
fast/css/zoom-body-scroll.html
http/tests/navigation/anchor-frames-same-origin.html
fast/events/mouse-cursor.html
http/tests/navigation/anchor-frames.html
http/tests/navigation/anchor-frames-gbk.html
Comment 6 Build Bot 2013-08-19 17:54:09 PDT
Created attachment 209145 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 7 gur.trio 2013-08-20 06:05:32 PDT
Created attachment 209187 [details]
Patch
Comment 8 gur.trio 2013-08-20 06:09:15 PDT
(In reply to comment #4)
> (From update of attachment 209083 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209083&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        No new tests (OOPS!).
> 
> WebKit changes that fix bugs must come with tests that cover the new behavior. Code change looks OK, but need test cases demonstrating both quirks mode and strict mode behavior.
> 
> > Source/WebCore/dom/Element.cpp:780
> >      document()->updateLayoutIgnorePendingStylesheets();
> > +    bool inQuirksMode = document()->inQuirksMode();     
> > +
> > +    if (document()->documentElement() == this) {
> > +        if (!inQuirksMode) {
> > +            if (FrameView* view = document()->view()) {
> > +                if (RenderView* renderView = document()->renderView())
> > +                    return adjustForAbsoluteZoom(view->scrollX(), renderView);
> > +            }
> > +        } else
> > +            return 0;
> > +    }
> >      if (RenderBox* rend = renderBox())
> >          return adjustForAbsoluteZoom(rend->scrollLeft(), rend);
> >      return 0;
> 
> A few small improvements are possible here; I suggest doing one or more of these:
> 
> First, if we are in quirks mode and this is the document element, it would be nice to not call updateLayoutIgnorePendingStylesheets at all, since there's no need to do that just to return 0.
> Second, We normally prefer "early return", so it would be more like this:
> 
>     if (inQuirksMode)
>         return 0;
>     // continue with strict mode case
> 
> Third, I think it's slightly more elegant to get the FrameView* from the RenderView* rather than getting both from the Document. Sort of “move over to the view system” first, and then get the relevant objects within the view system rather than going from model to view twice in a row.
> 
> All of these improvements could apply to scrollTop as well.
> 
> > Source/WebCore/html/HTMLBodyElement.cpp:270
> >      Document* document = this->document();
> >      document->updateLayoutIgnorePendingStylesheets();
> >      FrameView* view = document->view();
> > -    return view ? adjustForZoom(view->scrollX(), document) : 0;
> > +    if (document->inQuirksMode())
> > +        return view ? adjustForZoom(view->scrollX(), document) : 0;
> > +    return 0;
> 
> We should restructure the code so it does not do updateLayoutIgnorePendingStylesheets if it is just going to return 0. Same in scrollTop.

Thanks for the review. I have done the changes as per your suggestion and also test cases have been added.
Comment 9 Build Bot 2013-08-20 09:18:40 PDT
Comment on attachment 209187 [details]
Patch

Attachment 209187 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1519436

New failing tests:
http/tests/navigation/anchor-frames-same-origin.html
http/tests/navigation/anchor-frames.html
http/tests/navigation/anchor-frames-gbk.html
Comment 10 Build Bot 2013-08-20 09:18:42 PDT
Created attachment 209201 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 11 Darin Adler 2013-08-20 13:58:53 PDT
Comment on attachment 209187 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209187&action=review

Code change is good, but tests need quite a bit of work.

> LayoutTests/fast/css/zoom-body-scroll-expected.txt:7
>  Scrolling right to 50
> -scrollLeft: 50
> +scrollLeft: 0

This change indicates that this test is no longer testing what it used to. We need to fix the test so it does test something. Simply confirming that scrollLeft is always 0 is not what we are trying to test here. We probably need to update the test, not just the expected result.

> LayoutTests/fast/dom/Element/scrollLeft-Quirks-expected.txt:3
> +PASS 500 is 500
> +PASS 500 is 500
> +PASS 0 is 0

This test is unnecessarily cryptic, not showing what it’s testing at all.

> LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:6
> +			    height: 9999px;

Formatting here is a mess; I think we are using tabs. Just spaces would be good.

> LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:16
> +				shouldEvaluateTo("500",window.pageXOffset);
> +			    shouldEvaluateTo("500",document.body.scrollLeft);
> +			    shouldEvaluateTo("0",document.documentElement.scrollLeft);

The correct way to write this is:

    shouldBe(“pageXOffset”, “500”);
    shouldBe(“document.body.scrollLeft”, “500”);
    shouldBe(“document.documentElement.scrollLeft”, “0”);

The test output will be considerably better if we do it that way.

> LayoutTests/fast/events/mouse-cursor.html:1
> -<!DOCTYPE html>
>  <html>

Please give the rationale for making this test use quirks mode.

> LayoutTests/http/tests/navigation/anchor-frames-expected.txt:13
> +PASS document.body.scrollTop == 0 is true
> +PASS document.body.scrollTop + document.documentElement.clientHeight == 2000 is true

This looks like it reflects a change in the test itself, but I don’t see the change to the test in this patch. Also, if we are expecting a specific value, then we should move to normal shouldBe style rather than using the boolean check, which is needed when we are doing something like “>” rather than “==”.

> LayoutTests/platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration-expected.txt:1
> -document.body.scrollTop = 1000
> +document.body.scrollTop = 0

We need this to log something that reflects the scrolling location, not just log zero.
Comment 12 gur.trio 2013-08-21 01:37:02 PDT
Created attachment 209257 [details]
Patch
Comment 13 gur.trio 2013-08-21 01:38:39 PDT
(In reply to comment #11)
> (From update of attachment 209187 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209187&action=review
> 
> Code change is good, but tests need quite a bit of work.
> 
> > LayoutTests/fast/css/zoom-body-scroll-expected.txt:7
> >  Scrolling right to 50
> > -scrollLeft: 50
> > +scrollLeft: 0
> 
> This change indicates that this test is no longer testing what it used to. We need to fix the test so it does test something. Simply confirming that scrollLeft is always 0 is not what we are trying to test here. We probably need to update the test, not just the expected result.
> 
> > LayoutTests/fast/dom/Element/scrollLeft-Quirks-expected.txt:3
> > +PASS 500 is 500
> > +PASS 500 is 500
> > +PASS 0 is 0
> 
> This test is unnecessarily cryptic, not showing what it’s testing at all.
> 
> > LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:6
> > +			    height: 9999px;
> 
> Formatting here is a mess; I think we are using tabs. Just spaces would be good.
> 
> > LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:16
> > +				shouldEvaluateTo("500",window.pageXOffset);
> > +			    shouldEvaluateTo("500",document.body.scrollLeft);
> > +			    shouldEvaluateTo("0",document.documentElement.scrollLeft);
> 
> The correct way to write this is:
> 
>     shouldBe(“pageXOffset”, “500”);
>     shouldBe(“document.body.scrollLeft”, “500”);
>     shouldBe(“document.documentElement.scrollLeft”, “0”);
> 
> The test output will be considerably better if we do it that way.
> 
> > LayoutTests/fast/events/mouse-cursor.html:1
> > -<!DOCTYPE html>
> >  <html>
> 
> Please give the rationale for making this test use quirks mode.
> 
> > LayoutTests/http/tests/navigation/anchor-frames-expected.txt:13
> > +PASS document.body.scrollTop == 0 is true
> > +PASS document.body.scrollTop + document.documentElement.clientHeight == 2000 is true
> 
> This looks like it reflects a change in the test itself, but I don’t see the change to the test in this patch. Also, if we are expecting a specific value, then we should move to normal shouldBe style rather than using the boolean check, which is needed when we are doing something like “>” rather than “==”.
> 
> > LayoutTests/platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration-expected.txt:1
> > -document.body.scrollTop = 1000
> > +document.body.scrollTop = 0
> 
> We need this to log something that reflects the scrolling location, not just log zero.

Thanks Darin for the detailed analysis and comments. Modified the existing and new test cases.
Comment 14 Build Bot 2013-08-21 04:30:28 PDT
Comment on attachment 209257 [details]
Patch

Attachment 209257 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1518748

New failing tests:
http/tests/navigation/anchor-frames-gbk.html
platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html
Comment 15 Build Bot 2013-08-21 04:30:30 PDT
Created attachment 209260 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 16 Build Bot 2013-08-21 04:51:53 PDT
Comment on attachment 209257 [details]
Patch

Attachment 209257 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1526174

New failing tests:
http/tests/navigation/anchor-frames.html
http/tests/navigation/anchor-frames-gbk.html
Comment 17 Build Bot 2013-08-21 04:51:56 PDT
Created attachment 209262 [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.4
Comment 18 gur.trio 2013-08-21 08:39:55 PDT
Created attachment 209268 [details]
Patch
Comment 19 Build Bot 2013-08-21 09:49:15 PDT
Comment on attachment 209268 [details]
Patch

Attachment 209268 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1512907

New failing tests:
http/tests/navigation/anchor-frames.html
http/tests/navigation/anchor-frames-gbk.html
Comment 20 Build Bot 2013-08-21 09:49:17 PDT
Created attachment 209279 [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.4
Comment 21 Build Bot 2013-08-21 14:10:50 PDT
Comment on attachment 209268 [details]
Patch

Attachment 209268 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1528186

New failing tests:
http/tests/navigation/anchor-frames-gbk.html
Comment 22 Build Bot 2013-08-21 14:10:55 PDT
Created attachment 209302 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 23 gur.trio 2013-08-22 01:07:02 PDT
Created attachment 209331 [details]
Patch
Comment 24 Build Bot 2013-08-22 04:20:58 PDT
Comment on attachment 209331 [details]
Patch

Attachment 209331 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1380516

New failing tests:
http/tests/navigation/anchor-frames.html
Comment 25 Build Bot 2013-08-22 04:21:02 PDT
Created attachment 209343 [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.4
Comment 26 gur.trio 2013-08-22 04:32:23 PDT
Created attachment 209345 [details]
Patch
Comment 27 gur.trio 2013-08-22 04:34:26 PDT
(In reply to comment #11)
> (From update of attachment 209187 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209187&action=review
> 
> Code change is good, but tests need quite a bit of work.
> 
> > LayoutTests/fast/css/zoom-body-scroll-expected.txt:7
> >  Scrolling right to 50
> > -scrollLeft: 50
> > +scrollLeft: 0
> 
> This change indicates that this test is no longer testing what it used to. We need to fix the test so it does test something. Simply confirming that scrollLeft is always 0 is not what we are trying to test here. We probably need to update the test, not just the expected result.
> 
> > LayoutTests/fast/dom/Element/scrollLeft-Quirks-expected.txt:3
> > +PASS 500 is 500
> > +PASS 500 is 500
> > +PASS 0 is 0
> 
> This test is unnecessarily cryptic, not showing what it’s testing at all.
> 
> > LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:6
> > +			    height: 9999px;
> 
> Formatting here is a mess; I think we are using tabs. Just spaces would be good.
> 
> > LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:16
> > +				shouldEvaluateTo("500",window.pageXOffset);
> > +			    shouldEvaluateTo("500",document.body.scrollLeft);
> > +			    shouldEvaluateTo("0",document.documentElement.scrollLeft);
> 
> The correct way to write this is:
> 
>     shouldBe(“pageXOffset”, “500”);
>     shouldBe(“document.body.scrollLeft”, “500”);
>     shouldBe(“document.documentElement.scrollLeft”, “0”);
> 
> The test output will be considerably better if we do it that way.
> 
> > LayoutTests/fast/events/mouse-cursor.html:1
> > -<!DOCTYPE html>
> >  <html>
> 
> Please give the rationale for making this test use quirks mode.
> 
> > LayoutTests/http/tests/navigation/anchor-frames-expected.txt:13
> > +PASS document.body.scrollTop == 0 is true
> > +PASS document.body.scrollTop + document.documentElement.clientHeight == 2000 is true
> 
> This looks like it reflects a change in the test itself, but I don’t see the change to the test in this patch. Also, if we are expecting a specific value, then we should move to normal shouldBe style rather than using the boolean check, which is needed when we are doing something like “>” rather than “==”.
> 
> > LayoutTests/platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration-expected.txt:1
> > -document.body.scrollTop = 1000
> > +document.body.scrollTop = 0
> 
> We need this to log something that reflects the scrolling location, not just log zero.

Hi Darin. I have uploaded the final patch id 209345. Can you please review?
Comment 28 Darin Adler 2013-08-22 14:26:48 PDT
Comment on attachment 209345 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209345&action=review

> Source/WebCore/dom/Element.cpp:768
> +    bool inQuirksMode = document()->inQuirksMode();
> +    if (document()->documentElement() == this && inQuirksMode)   
> +        return 0;

Why put inQuirksMode into a local variable since it’s only used once?

> Source/WebCore/dom/Element.cpp:787
> +    bool inQuirksMode = document()->inQuirksMode();
> +    if (document()->documentElement() == this && inQuirksMode)   
> +        return 0;

Why put inQuirksMode into a local variable since it’s only used once?

> Source/WebCore/html/HTMLBodyElement.cpp:271
>      // Update the document's layout.
> -    Document* document = this->document();
> -    document->updateLayoutIgnorePendingStylesheets();
> +    Document* document = this->document();  
>      FrameView* view = document->view();
> -    return view ? adjustForZoom(view->scrollX(), document) : 0;
> +    if (document->inQuirksMode()) {
> +        document->updateLayoutIgnorePendingStylesheets();
> +        return view ? adjustForZoom(view->scrollX(), document) : 0;
> +    }
> +    return 0;

Much cleaner to use early return:

    if (!document->inQuirksMode())
        return 0;

Then you don’t have to touch the rest of the function at all, except probably for removing the bogus comment “Update the document's layout”, which just says the same thing that the function name updateLayoutIgnorePendingStylesheets already does.

> Source/WebCore/html/HTMLBodyElement.cpp:296
>      // Update the document's layout.
> -    Document* document = this->document();
> -    document->updateLayoutIgnorePendingStylesheets();
> +    Document* document = this->document();  
>      FrameView* view = document->view();
> -    return view ? adjustForZoom(view->scrollY(), document) : 0;
> +    if (document->inQuirksMode()) {
> +        document->updateLayoutIgnorePendingStylesheets();
> +        return view ? adjustForZoom(view->scrollY(), document) : 0;
> +    }
> +    return 0;

Same suggestion: Use early return.

> LayoutTests/fast/css/zoom-body-scroll.html:3
> -    <div style="width: 1000px; height: 1000px; position: absolute; top: 0; left: 0;"></div>
> +<body onload="pageScroll()">
> +    <div style="width: 9999px; height: 9999px; position: absolute; top: 0; left: 0;"></div>

Why the change from 1000 to 9999?

> LayoutTests/fast/css/zoom-body-scroll.html:18
> +            setTimeout(bodyScroll(),3000);

Why the 3 second delay? We don’t want test that take 3 seconds to run!

> LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:14
> +				setTimeout(function() {

This file still has tab characters in it.

> LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:18
> +                    shouldBe("500","window.pageXOffset");
> +                    shouldBe("500","document.body.scrollLeft");
> +                    shouldBe("0","document.documentElement.scrollLeft");

These are backwards. The test needs to be in the left expression, and the expected value on the right.

> LayoutTests/fast/dom/Element/scrollLeft.html:19
> +                    shouldBe("500","window.pageXOffset");
> +                    shouldBe("0","document.body.scrollLeft");
> +                    shouldBe("500","document.documentElement.scrollLeft");

These are backwards. The test needs to be in the left expression, and the expected value on the right.

> LayoutTests/fast/dom/Element/scrollTop-Quirks.html:18
> +                    shouldBe("500","window.pageYOffset");              
> +					shouldBe("500","document.body.scrollTop");
> +                    shouldBe("0","document.documentElement.scrollTop");

These are backwards. The test needs to be in the left expression, and the expected value on the right. And there are some tabs in here.

> LayoutTests/fast/dom/Element/scrollTop.html:19
> +                    shouldBe("500","window.pageYOffset");
> +                    shouldBe("0","document.body.scrollTop");
> +                    shouldBe("500","document.documentElement.scrollTop");

These are backwards. The test needs to be in the left expression, and the expected value on the right.

> LayoutTests/http/tests/navigation/resources/frame-with-anchor-same-origin.html:18
> -<body>
> +<body onload="runTest()">

How did this test run before? I don’t understand why this ever worked.

> LayoutTests/http/tests/navigation/resources/frame-with-anchor.html:7
> -
> +	  

No reason to add this whitespace.
Comment 29 gur.trio 2013-08-23 04:17:22 PDT
Created attachment 209446 [details]
Patch
Comment 30 gur.trio 2013-08-23 04:19:58 PDT
(In reply to comment #27)
> (In reply to comment #11)
> > (From update of attachment 209187 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=209187&action=review
> > 
> > Code change is good, but tests need quite a bit of work.
> > 
> > > LayoutTests/fast/css/zoom-body-scroll-expected.txt:7
> > >  Scrolling right to 50
> > > -scrollLeft: 50
> > > +scrollLeft: 0
> > 
> > This change indicates that this test is no longer testing what it used to. We need to fix the test so it does test something. Simply confirming that scrollLeft is always 0 is not what we are trying to test here. We probably need to update the test, not just the expected result.
> > 
> > > LayoutTests/fast/dom/Element/scrollLeft-Quirks-expected.txt:3
> > > +PASS 500 is 500
> > > +PASS 500 is 500
> > > +PASS 0 is 0
> > 
> > This test is unnecessarily cryptic, not showing what it’s testing at all.
> > 
> > > LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:6
> > > +			    height: 9999px;
> > 
> > Formatting here is a mess; I think we are using tabs. Just spaces would be good.
> > 
> > > LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:16
> > > +				shouldEvaluateTo("500",window.pageXOffset);
> > > +			    shouldEvaluateTo("500",document.body.scrollLeft);
> > > +			    shouldEvaluateTo("0",document.documentElement.scrollLeft);
> > 
> > The correct way to write this is:
> > 
> >     shouldBe(“pageXOffset”, “500”);
> >     shouldBe(“document.body.scrollLeft”, “500”);
> >     shouldBe(“document.documentElement.scrollLeft”, “0”);
> > 
> > The test output will be considerably better if we do it that way.
> > 
> > > LayoutTests/fast/events/mouse-cursor.html:1
> > > -<!DOCTYPE html>
> > >  <html>
> > 
> > Please give the rationale for making this test use quirks mode.
> > 
> > > LayoutTests/http/tests/navigation/anchor-frames-expected.txt:13
> > > +PASS document.body.scrollTop == 0 is true
> > > +PASS document.body.scrollTop + document.documentElement.clientHeight == 2000 is true
> > 
> > This looks like it reflects a change in the test itself, but I don’t see the change to the test in this patch. Also, if we are expecting a specific value, then we should move to normal shouldBe style rather than using the boolean check, which is needed when we are doing something like “>” rather than “==”.
> > 
> > > LayoutTests/platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration-expected.txt:1
> > > -document.body.scrollTop = 1000
> > > +document.body.scrollTop = 0
> > 
> > We need this to log something that reflects the scrolling location, not just log zero.
> 
> Hi Darin. I have uploaded the final patch id 209345. Can you please review?

(In reply to comment #28)
> (From update of attachment 209345 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209345&action=review
> 
> > Source/WebCore/dom/Element.cpp:768
> > +    bool inQuirksMode = document()->inQuirksMode();
> > +    if (document()->documentElement() == this && inQuirksMode)   
> > +        return 0;
> 
> Why put inQuirksMode into a local variable since it’s only used once?
> 
> > Source/WebCore/dom/Element.cpp:787
> > +    bool inQuirksMode = document()->inQuirksMode();
> > +    if (document()->documentElement() == this && inQuirksMode)   
> > +        return 0;
> 
> Why put inQuirksMode into a local variable since it’s only used once?
> 
> > Source/WebCore/html/HTMLBodyElement.cpp:271
> >      // Update the document's layout.
> > -    Document* document = this->document();
> > -    document->updateLayoutIgnorePendingStylesheets();
> > +    Document* document = this->document();  
> >      FrameView* view = document->view();
> > -    return view ? adjustForZoom(view->scrollX(), document) : 0;
> > +    if (document->inQuirksMode()) {
> > +        document->updateLayoutIgnorePendingStylesheets();
> > +        return view ? adjustForZoom(view->scrollX(), document) : 0;
> > +    }
> > +    return 0;
> 
> Much cleaner to use early return:
> 
>     if (!document->inQuirksMode())
>         return 0;
> 
> Then you don’t have to touch the rest of the function at all, except probably for removing the bogus comment “Update the document's layout”, which just says the same thing that the function name updateLayoutIgnorePendingStylesheets already does.
> 
> > Source/WebCore/html/HTMLBodyElement.cpp:296
> >      // Update the document's layout.
> > -    Document* document = this->document();
> > -    document->updateLayoutIgnorePendingStylesheets();
> > +    Document* document = this->document();  
> >      FrameView* view = document->view();
> > -    return view ? adjustForZoom(view->scrollY(), document) : 0;
> > +    if (document->inQuirksMode()) {
> > +        document->updateLayoutIgnorePendingStylesheets();
> > +        return view ? adjustForZoom(view->scrollY(), document) : 0;
> > +    }
> > +    return 0;
> 
> Same suggestion: Use early return.
> 
> > LayoutTests/fast/css/zoom-body-scroll.html:3
> > -    <div style="width: 1000px; height: 1000px; position: absolute; top: 0; left: 0;"></div>
> > +<body onload="pageScroll()">
> > +    <div style="width: 9999px; height: 9999px; position: absolute; top: 0; left: 0;"></div>
> 
> Why the change from 1000 to 9999?
> 
> > LayoutTests/fast/css/zoom-body-scroll.html:18
> > +            setTimeout(bodyScroll(),3000);
> 
> Why the 3 second delay? We don’t want test that take 3 seconds to run!
> 
> > LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:14
> > +				setTimeout(function() {
> 
> This file still has tab characters in it.
> 
> > LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:18
> > +                    shouldBe("500","window.pageXOffset");
> > +                    shouldBe("500","document.body.scrollLeft");
> > +                    shouldBe("0","document.documentElement.scrollLeft");
> 
> These are backwards. The test needs to be in the left expression, and the expected value on the right.
> 
> > LayoutTests/fast/dom/Element/scrollLeft.html:19
> > +                    shouldBe("500","window.pageXOffset");
> > +                    shouldBe("0","document.body.scrollLeft");
> > +                    shouldBe("500","document.documentElement.scrollLeft");
> 
> These are backwards. The test needs to be in the left expression, and the expected value on the right.
> 
> > LayoutTests/fast/dom/Element/scrollTop-Quirks.html:18
> > +                    shouldBe("500","window.pageYOffset");              
> > +					shouldBe("500","document.body.scrollTop");
> > +                    shouldBe("0","document.documentElement.scrollTop");
> 
> These are backwards. The test needs to be in the left expression, and the expected value on the right. And there are some tabs in here.
> 
> > LayoutTests/fast/dom/Element/scrollTop.html:19
> > +                    shouldBe("500","window.pageYOffset");
> > +                    shouldBe("0","document.body.scrollTop");
> > +                    shouldBe("500","document.documentElement.scrollTop");
> 
> These are backwards. The test needs to be in the left expression, and the expected value on the right.
> 
> > LayoutTests/http/tests/navigation/resources/frame-with-anchor-same-origin.html:18
> > -<body>
> > +<body onload="runTest()">
> 
> How did this test run before? I don’t understand why this ever worked.
> 
> > LayoutTests/http/tests/navigation/resources/frame-with-anchor.html:7
> > -
> > +	  
> 
> No reason to add this whitespace.

Hi Darin. Thanks for the review.

I changed 1000 to 9999 because was testing the test case on browser and I thought it might need bigger width and height so that can scroll.
Comment 31 Build Bot 2013-08-23 05:25:52 PDT
Comment on attachment 209446 [details]
Patch

Attachment 209446 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1543285

New failing tests:
fast/dom/Element/scrollLeft.html
fast/dom/Element/scrollTop.html
Comment 32 Build Bot 2013-08-23 05:25:55 PDT
Created attachment 209454 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 33 gur.trio 2013-08-23 06:50:27 PDT
Created attachment 209458 [details]
Patch
Comment 34 Darin Adler 2013-08-23 10:07:36 PDT
Comment on attachment 209458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209458&action=review

> Source/WebCore/dom/Element.cpp:782
> -{
> +{    

Unwanted trailing whitespace added here.

> Source/WebCore/html/HTMLBodyElement.cpp:263
> +{    

Unwanted trailing whitespace added here.

> Source/WebCore/html/HTMLBodyElement.cpp:286
> +{    

Unwanted trailing whitespace added here.

> Source/WebCore/html/HTMLBodyElement.cpp:309
> -{
> +{ 

Unwanted trailing whitespace added here.
Comment 35 gur.trio 2013-08-23 23:47:50 PDT
Created attachment 209533 [details]
Patch
Comment 36 EFL EWS Bot 2013-08-23 23:52:50 PDT
Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1543565
Comment 37 EFL EWS Bot 2013-08-23 23:53:05 PDT
Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1546505
Comment 38 Early Warning System Bot 2013-08-23 23:53:24 PDT
Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1544508
Comment 39 Early Warning System Bot 2013-08-23 23:54:06 PDT
Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1546506
Comment 40 Build Bot 2013-08-24 00:15:27 PDT
Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1562014
Comment 41 Build Bot 2013-08-24 00:27:26 PDT
Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1529775
Comment 42 kov's GTK+ EWS bot 2013-08-24 00:32:09 PDT
Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1546510
Comment 43 gur.trio 2013-08-24 00:35:01 PDT
Created attachment 209535 [details]
Patch
Comment 44 Build Bot 2013-08-24 00:38:23 PDT
Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1562019
Comment 45 kov's GTK+ EWS bot 2013-08-24 00:41:06 PDT
Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1543579
Comment 46 EFL EWS Bot 2013-08-24 00:41:31 PDT
Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1543580
Comment 47 Early Warning System Bot 2013-08-24 00:43:22 PDT
Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1556466
Comment 48 Early Warning System Bot 2013-08-24 00:43:53 PDT
Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1529781
Comment 49 EFL EWS Bot 2013-08-24 00:53:52 PDT
Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1542532
Comment 50 Build Bot 2013-08-24 01:00:55 PDT
Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1526910
Comment 51 gur.trio 2013-08-24 03:48:58 PDT
Created attachment 209537 [details]
Patch
Comment 52 Build Bot 2013-08-24 03:52:07 PDT
Comment on attachment 209537 [details]
Patch

Attachment 209537 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1557435
Comment 53 EFL EWS Bot 2013-08-24 03:52:56 PDT
Comment on attachment 209537 [details]
Patch

Attachment 209537 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1560243
Comment 54 Early Warning System Bot 2013-08-24 03:54:27 PDT
Comment on attachment 209537 [details]
Patch

Attachment 209537 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1542563
Comment 55 Early Warning System Bot 2013-08-24 03:55:59 PDT
Comment on attachment 209537 [details]
Patch

Attachment 209537 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1529817
Comment 56 EFL EWS Bot 2013-08-24 03:57:01 PDT
Comment on attachment 209537 [details]
Patch

Attachment 209537 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1557436
Comment 57 gur.trio 2013-08-24 04:02:50 PDT
Created attachment 209538 [details]
Patch
Comment 58 gur.trio 2013-08-24 04:05:50 PDT
(In reply to comment #57)
> Created an attachment (id=209538) [details]
> Patch

Sorry about the last 3 build breaks.
Comment 59 Darin Adler 2013-08-24 14:43:55 PDT
Comment on attachment 209538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209538&action=review

> LayoutTests/http/tests/navigation/resources/frame-with-anchor-gbk.html:7
> -      description('Tests that loading a frame with a URL that contains a fragment pointed at a named anchor actually scrolls to that anchor.');
> +	  description('Tests that loading a frame with a URL that contains a fragment pointed at a named anchor actually scrolls to that anchor.');

Here, the patch changes four spaces into a tab character. That’s not a change we want.
Comment 60 gur.trio 2013-08-25 22:12:31 PDT
Created attachment 209615 [details]
Patch
Comment 61 WebKit Commit Bot 2013-08-26 10:47:46 PDT
Comment on attachment 209615 [details]
Patch

Clearing flags on attachment: 209615

Committed r154614: <http://trac.webkit.org/changeset/154614>
Comment 62 WebKit Commit Bot 2013-08-26 10:47:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 63 Antonio Gomes 2013-09-25 13:15:23 PDT
*** Bug 9248 has been marked as a duplicate of this bug. ***
Comment 64 Antonio Gomes 2013-10-01 13:37:23 PDT
Comment on attachment 209615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209615&action=review

Driven by comments, since it has landed..

> LayoutTests/fast/css/zoom-body-scroll.html:-1
> -<!DOCTYPE HTML>

Just a side note, making the tests quirks-mode to pass the test seems awkward. I would have updated the test in strict-mode here and all over.

> LayoutTests/platform/win/fast/css/zoom-body-scroll-expected.txt:12
> -scrollTop: 0
> +scrollTop: 100

this change would not be needed if the DOCTYPE is kept, and the test updated.
Comment 65 gur.trio 2013-10-03 20:31:11 PDT
(In reply to comment #64)
> (From update of attachment 209615 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209615&action=review
> 
> Driven by comments, since it has landed..
> 
> > LayoutTests/fast/css/zoom-body-scroll.html:-1
> > -<!DOCTYPE HTML>
> 
> Just a side note, making the tests quirks-mode to pass the test seems awkward. I would have updated the test in strict-mode here and all over.
> 
> > LayoutTests/platform/win/fast/css/zoom-body-scroll-expected.txt:12
> > -scrollTop: 0
> > +scrollTop: 100
> 
> this change would not be needed if the DOCTYPE is kept, and the test updated.

Hi Antonio. Can I do it now under this bug only?
Comment 66 Darin Adler 2013-10-04 09:43:31 PDT
No, it’s not a good idea to add a patch to a resolved bug. New bug report is much better.
Comment 67 gur.trio 2013-10-08 00:19:30 PDT
(In reply to comment #66)
> No, it’s not a good idea to add a patch to a resolved bug. New bug report is much better.

Ok will create a new bug and make the changes.
Comment 68 Ryosuke Niwa 2013-10-15 22:01:40 PDT
This patch caused a major usability regression on Facebook: https://webkit.org/b/122882
Comment 69 Ryosuke Niwa 2013-10-15 23:58:03 PDT
Comment on attachment 209615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209615&action=review

> Source/WebCore/ChangeLog:9
> +        As per the specification webkit should return document.body.scrollTop

Which specification? Please give us the URL.
Comment 70 Ryosuke Niwa 2013-10-29 17:57:09 PDT
I don't think we can keep this patch in the trunk. It broke Facebook and causing all sorts of regressions on other pages.
Comment 71 Antonio Gomes 2013-11-12 12:18:47 PST
(In reply to comment #70)
> I don't think we can keep this patch in the trunk. It broke Facebook and causing all sorts of regressions on other pages.

@rniwa, did you happen to compile a list of broken sites? (know of facebook and webkit.org)
Comment 72 gur.trio 2014-03-06 07:41:33 PST
Changes reverted so re-opening the bug.
Comment 73 gur.trio 2014-03-06 07:42:01 PST
Changes reverted so re-opening the bug.
Comment 74 Mathias Bynens 2014-09-19 05:02:21 PDT
(In reply to comment #69)
> (From update of attachment 209615 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209615&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        As per the specification webkit should return document.body.scrollTop
> 
> Which specification? Please give us the URL.

http://dev.w3.org/csswg/cssom-view/

https://bugs.webkit.org/show_bug.cgi?id=5991
Comment 75 j.j. 2015-03-07 09:47:14 PST
isn't bug 5991 a duplicate?
Comment 76 Rick Byers 2015-08-11 18:22:35 PDT
Note that we think we're pretty close (after working to update a number of popular websites) to being able to finally change blink's behavior to match the spec here: http://crbug.com/157855
Comment 77 Radar WebKit Bug Importer 2015-08-14 17:59:27 PDT
<rdar://problem/22296476>
Comment 78 Simon Fraser (smfr) 2016-03-07 16:22:10 PST
WebKit supports scrollingElement now (bug 143609).
Comment 79 Frédéric Wang (:fredw) 2017-04-20 07:33:32 PDT
Created attachment 307594 [details]
Patch

This is a quick rebase of Gurpreet's patch.

It indeed provides the correct values when getting the values but setting is wrong per https://bugs.webkit.org/show_bug.cgi?id=9248#c15 ; I checked that indeed http://maisqi.com/outros/bugs/chrome/CHN6 still does not work.

(In reply to j.j. from comment #75)
> isn't bug 5991 a duplicate?

Per bug 5991 comment 2, the intent seems more general since we also want to deal with setting the values too.

Maybe we should resolve this bug as duplicate of bug 5991 and continue the work/discussion there?
Comment 80 Frédéric Wang (:fredw) 2017-04-21 01:59:08 PDT

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