Summary: | document.body.scrollTop & document.documentElement.scrollTop differ cross-browser | ||
---|---|---|---|
Product: | WebKit | Reporter: | Vince Malone <vince.t.malone> |
Component: | New Bugs | Assignee: | 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
Vince Malone
2013-01-04 14:42:55 PST
Created attachment 209083 [details]
WIP Patch
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 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 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 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 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. 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 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 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. 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 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 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 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
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 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 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 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
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 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? 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. 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 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
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
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 on attachment 209533 [details] Patch Attachment 209533 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1546505 Comment on attachment 209533 [details] Patch Attachment 209533 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1544508 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 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 on attachment 209533 [details] Patch Attachment 209533 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1529775 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
Comment on attachment 209535 [details] Patch Attachment 209535 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1562019 Comment on attachment 209535 [details] Patch Attachment 209535 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1543579 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 on attachment 209535 [details] Patch Attachment 209535 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1556466 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 on attachment 209535 [details] Patch Attachment 209535 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1542532 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
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 on attachment 209537 [details] Patch Attachment 209537 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1560243 Comment on attachment 209537 [details] Patch Attachment 209537 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1542563 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 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. 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
Comment on attachment 209615 [details] Patch Clearing flags on attachment: 209615 Committed r154614: <http://trac.webkit.org/changeset/154614> All reviewed patches have been landed. Closing bug. *** Bug 9248 has been marked as a duplicate of this bug. *** 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? 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. This patch caused a major usability regression on Facebook: https://webkit.org/b/122882 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. I don't think we can keep this patch in the trunk. It broke Facebook and causing all sorts of regressions on other pages. (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. (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 isn't bug 5991 a duplicate? 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 WebKit supports scrollingElement now (bug 143609). 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? *** This bug has been marked as a duplicate of bug 5991 *** |