[Webkit-unassigned] [Bug 106133] document.body.scrollTop & document.documentElement.scrollTop differ cross-browser

Tue Aug 20 13:59:25 PDT 2013

https://bugs.webkit.org/show_bug.cgi?id=106133

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #209187|review?                     |review-
               Flag|                            |

--- Comment #11 from Darin Adler <darin at apple.com>  2013-08-20 13:58:53 PST ---
(From update of attachment 209187)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.