| Summary: | REGRESSION (r154614): Setting the document scroll position isn't symmetric; can successfully set document.body.scrollTop, but can only read from document.documentElement.scrollTop | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Ricky Mondello <rmondello> |
| Component: | DOM | Assignee: | gur.trio |
| Status: | RESOLVED DUPLICATE | ||
| Severity: | Normal | CC: | ap, bdakin, buildbot, cmarcelo, commit-queue, darin, eflews.bot, esprehn+autocc, fred.wang, gtk-ews, gur.trio, gyuyoung.kim, hyatt, jberlin, kangil.han, rego+ews, rniwa, tonikitoo, webkit-bug-importer, webkit-ews, xan.lopez |
| Priority: | P2 | Keywords: | InRadar |
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| URL: | http://maisqi.com/outros/bugs/chrome/CHN6 | ||
| Bug Depends on: | 106133 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Ricky Mondello
2013-09-24 15:55:01 PDT
Tracking a related issue at <rdar://problem/14931862>. This issue is now tracked as <rdar://problem/15073848>. (In reply to comment #2) > This issue is now tracked as <rdar://problem/15073848>. Quirks mode should set scrolltop/scrollleft through document.documentElement.scrollTop/document.documentElement.scrollLeft Non-Quirks mode should set scrolltop/scrollleft through document.body.scrollTop/document.body.scrollLeft Other combinations should not scroll. Please comfirm. (In reply to comment #3) > (In reply to comment #2) > > This issue is now tracked as <rdar://problem/15073848>. > > Quirks mode should set scrolltop/scrollleft through document.documentElement.scrollTop/document.documentElement.scrollLeft > > Non-Quirks mode should set scrolltop/scrollleft through document.body.scrollTop/document.body.scrollLeft > > Other combinations should not scroll. > > Please comfirm. Please ignore the previous comment Non-Quirks mode should set/get scrolltop/scrollleft through document.documentElement.scrollTop/document.documentElement.scrollLeft Quirks mode should set/get scrolltop/scrollleft through document.body.scrollTop/document.body.scrollLeft Other combinations should not scroll. Please confirm. Created attachment 212564 [details]
Patch
Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/2235138 Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/2245041 Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/2212029 Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/2261034 Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/2277003 Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2162190 Comment on attachment 212564 [details] Patch Attachment 212564 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2281003 Created attachment 212568 [details]
Patch
Comment on attachment 212568 [details] Patch Attachment 212568 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2288011 New failing tests: fast/multicol/scrolling-overflow.html Created attachment 212576 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 212568 [details] Patch Attachment 212568 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2166232 New failing tests: fast/multicol/scrolling-overflow.html Created attachment 212578 [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.5
Created attachment 212580 [details]
Patch
Comment on attachment 212580 [details] Patch Attachment 212580 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2289007 New failing tests: fast/multicol/scrolling-overflow.html Created attachment 212583 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 212580 [details] Patch Attachment 212580 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2287054 New failing tests: fast/multicol/scrolling-overflow.html Created attachment 212606 [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.5
Comment on attachment 212580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212580&action=review r=me if you address the failing test > Source/WebCore/dom/Element.cpp:807 > + RenderView& renderView = *document().renderView(); This is only used inside the if statement. So this should be moved inside the if statement. > Source/WebCore/dom/Element.cpp:811 > + FrameView& view = renderView.frameView(); This local variable doesn’t add anything. I suggest getting rid of it. > Source/WebCore/dom/Element.cpp:829 > + RenderView& renderView = *document().renderView(); This is only used inside the if statement. So this should be moved inside the if statement. > LayoutTests/fast/multicol/scrolling-overflow.html:1 > -<!DOCTYPE html> > <html> Looks like this test is failing in the EWS bot. Created attachment 212675 [details]
Patch
(In reply to comment #23) > (From update of attachment 212580 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212580&action=review > > r=me if you address the failing test > > > Source/WebCore/dom/Element.cpp:807 > > + RenderView& renderView = *document().renderView(); > > This is only used inside the if statement. So this should be moved inside the if statement. > > > Source/WebCore/dom/Element.cpp:811 > > + FrameView& view = renderView.frameView(); > > This local variable doesn’t add anything. I suggest getting rid of it. > > > Source/WebCore/dom/Element.cpp:829 > > + RenderView& renderView = *document().renderView(); > > This is only used inside the if statement. So this should be moved inside the if statement. > > > LayoutTests/fast/multicol/scrolling-overflow.html:1 > > -<!DOCTYPE html> > > <html> > > Looks like this test is failing in the EWS bot. Hi Darin. Thanks for the review. I added FrameView& view = renderView.frameView(); because twice we need frameview once to setScrollPosition and once for scrollY/scrollX so thought of using as a local variable. (In reply to comment #25) > (In reply to comment #23) > > (From update of attachment 212580 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=212580&action=review > > > > r=me if you address the failing test > > > > > Source/WebCore/dom/Element.cpp:807 > > > + RenderView& renderView = *document().renderView(); > > > > This is only used inside the if statement. So this should be moved inside the if statement. > > > > > Source/WebCore/dom/Element.cpp:811 > > > + FrameView& view = renderView.frameView(); > > > > This local variable doesn’t add anything. I suggest getting rid of it. > > > > > Source/WebCore/dom/Element.cpp:829 > > > + RenderView& renderView = *document().renderView(); > > > > This is only used inside the if statement. So this should be moved inside the if statement. > > > > > LayoutTests/fast/multicol/scrolling-overflow.html:1 > > > -<!DOCTYPE html> > > > <html> > > > > Looks like this test is failing in the EWS bot. > > Hi Darin. Thanks for the review. > I added FrameView& view = renderView.frameView(); because twice we need frameview once to setScrollPosition and once for scrollY/scrollX so thought of using as a local variable. The failing test case shows a difference in the dump render tree rects of html, body which should not happen because the height of div element is specified in the test case as 300 and also my changes would not affect the height. Expected: RenderBlock {HTML} at (0,0) size 800x316 Actual : RenderBlock {HTML} at (0,0) size 800x585 Comment on attachment 212675 [details] Patch Attachment 212675 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2397007 New failing tests: fast/multicol/scrolling-overflow.html Created attachment 212678 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 212675 [details] Patch Attachment 212675 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2423006 New failing tests: fast/multicol/scrolling-overflow.html Created attachment 212681 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
*** Bug 121937 has been marked as a duplicate of this bug. *** Comment on attachment 212675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212675&action=review Looks like the test is still failing. > LayoutTests/fast/multicol/scrolling-overflow.html:7 > - -webkit-column-width: 200px; > + -webkit-column-width: 200px; Why this whitespace change? (In reply to comment #32) > (From update of attachment 212675 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212675&action=review > > Looks like the test is still failing. > > > LayoutTests/fast/multicol/scrolling-overflow.html:7 > > - -webkit-column-width: 200px; > > + -webkit-column-width: 200px; > > Why this whitespace change? The whitespace is by mistake.Will make the changes. As mentioned not sure why the test case is failing.The expected.txt html rect's height and body rect's height donot match the actual.txt. But my changes shouldnot affect those. Does this test fail for you with this patch locally? It's almost certain that the patch actually changes its results. (In reply to comment #34) > Does this test fail for you with this patch locally? It's almost certain that the patch actually changes its results. Will run the test case locally with and withoutthe patch and confirm. Created attachment 212795 [details]
Patch
(In reply to comment #36) > Created an attachment (id=212795) [details] > Patch Removing the <!DOCTYPE html> was creating some issues and hence the test case was failing. So have modified the test case accordingly. Comment on attachment 212795 [details] Patch Clearing flags on attachment: 212795 Committed r156605: <http://trac.webkit.org/changeset/156605> All reviewed patches have been landed. Closing bug. The original patch also caused https://bugs.webkit.org/show_bug.cgi?id=122882. Changes reverted so re-opening the bug. I believe we this duplicate this against bug 106133, and do both setter and getter together (one patch). (In reply to Antonio Gomes from comment #43) > I believe we this duplicate this against bug 106133, and do both setter and > getter together (one patch). What about marked this duplicate of 5991 and continue the work there? *** This bug has been marked as a duplicate of bug 5991 *** |