Bug 106133 – document.body.scrollTop & document.documentElement.scrollTop differ cross-browser

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Vince Malone]

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.

Created attachment 209083 [details]
WIP Patch

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Build Bot]

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

[Build Bot]

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

Created attachment 209187 [details]
Patch

(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.

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

Created attachment 209257 [details]
Patch

(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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

Created attachment 209268 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

Created attachment 209331 [details]
Patch

[Build Bot]

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

[Build Bot]

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

Created attachment 209345 [details]
Patch

(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?

[Darin Adler]

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.

Created attachment 209446 [details]
Patch

(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.

[Build Bot]

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

[Build Bot]

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

Created attachment 209458 [details]
Patch

[Darin Adler]

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.

Created attachment 209533 [details]
Patch

[EFL EWS Bot]

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

[EFL EWS Bot]

Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1546505

[Early Warning System Bot]

Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1544508

[Early Warning System Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 209533 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 209533 [details]
Patch

Attachment 209533 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1546510

Created attachment 209535 [details]
Patch

[Build Bot]

Comment on attachment 209535 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1543579

[EFL EWS Bot]

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

[Early Warning System Bot]

Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1556466

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 209535 [details]
Patch

Attachment 209535 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1542532

[Build Bot]

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

Created attachment 209537 [details]
Patch

[Build Bot]

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

[EFL EWS Bot]

Comment on attachment 209537 [details]
Patch

Attachment 209537 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1560243

[Early Warning System Bot]

Comment on attachment 209537 [details]
Patch

Attachment 209537 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1542563

[Early Warning System Bot]

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

[EFL EWS Bot]

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

Created attachment 209538 [details]
Patch

(In reply to comment #57)
> Created an attachment (id=209538) [details]
> Patch

Sorry about the last 3 build breaks.

[Darin Adler]

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.

Created attachment 209615 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 209615 [details]
Patch

Clearing flags on attachment: 209615

Committed r154614: <http://trac.webkit.org/changeset/154614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

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

[Antonio Gomes]

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.

(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?

[Darin Adler]

No, it’s not a good idea to add a patch to a resolved bug. New bug report is much better.

(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.

[Ryosuke Niwa]

This patch caused a major usability regression on Facebook: https://webkit.org/b/122882

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

I don't think we can keep this patch in the trunk. It broke Facebook and causing all sorts of regressions on other pages.

[Antonio Gomes]

(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)

Changes reverted so re-opening the bug.

Changes reverted so re-opening the bug.

[Mathias Bynens]

(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

[j.j.]

isn't bug 5991 a duplicate?

[Rick Byers]

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

[Radar WebKit Bug Importer]

<rdar://problem/22296476>

[Simon Fraser (smfr)]

WebKit supports scrollingElement now (bug 143609).

[Frédéric Wang (:fredw)]

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?

[Frédéric Wang (:fredw)]

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