[webkit-reviews] review denied: [Bug 106133] document.body.scrollTop & document.documentElement.scrollTop differ cross-browser : [Attachment 209187] Patch

Tue Aug 20 13:59:23 PDT 2013

Darin Adler <darin at apple.com> has denied gur.trio at gmail.com's request for
review:
Bug 106133: document.body.scrollTop & document.documentElement.scrollTop differ
cross-browser
https://bugs.webkit.org/show_bug.cgi?id=106133

Attachment 209187: Patch
https://bugs.webkit.org/attachment.cgi?id=209187&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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-c
ache-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.