| Summary: | Refactor WebViewImpl::scrollFocusedNodeIntoRect to a better place | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Varun Jain <varunjain> | ||||||||||||||||||
| Component: | DOM | Assignee: | Varun Jain <varunjain> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | dglazkov, fishd, fsamuel, gustavo, rniwa, webkit.review.bot, xan.lopez | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Varun Jain
2011-09-15 16:27:15 PDT
Created attachment 108219 [details]
Patch
Hi Darin, Dimitri refactored code according to discussion with Dimitri. PTAL. @Darin: regarding your last comment on my earlier CL. This method is only invoked on elements that are currently in view. Hence it is enough to simply scroll the main frame. We need not care about the case where the element is in a subframe. (In reply to comment #1) > Created an attachment (id=108219) [details] > Patch It would be great to have a layout test where there's sufficient size to center the input box in the rect so we know for a fact centering is working as expected. Created attachment 108227 [details]
Patch
(In reply to comment #3) > (In reply to comment #1) > > Created an attachment (id=108219) [details] [details] > > Patch > > It would be great to have a layout test where there's sufficient size to center the input box in the rect so we know for a fact centering is working as expected. Done. Comment on attachment 108227 [details] Patch Attachment 108227 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9773901 Created attachment 108330 [details]
Patch
Comment on attachment 108330 [details] Patch Attachment 108330 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9792319 Created attachment 108378 [details]
Patch
Comment on attachment 108378 [details] Patch Attachment 108378 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9796377 ping? Created attachment 108486 [details]
Patch
Comment on attachment 108486 [details] Patch Attachment 108486 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9822109 Created attachment 108489 [details]
Patch
Comment on attachment 108489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108489&action=review Looks good, except for the test. > LayoutTests/fast/dom/scroll-element-to-rect-centered.html:17 > + if (window.internals) { > + var box = document.getElementById('textbox'); > + window.internals.scrollElementToRect(box, 50, 30, 200, 250); > + } > + } > + window.addEventListener("load", doTest); > + </script> > + </head> > + <body> > + This is test<br> > + <div style="position: relative; width: 2400px; height: 2400px; background-color: white;"> > + <div style="position:fixed; left: 50px; top: 30px; width: 200px; height: 250px; background-color: green;"></div> > + <div style="position:absolute; left: 600px; top: 800px;"> <input id="textbox" type="text"></div> I am pretty sure we can make this test dumpAsText(), to avoid pixel baselines. Can we? > Source/WebKit/chromium/src/WebViewImpl.cpp:1824 > + if (focusedNode && focusedNode->isElementNode()) { early return? (In reply to comment #15) > (From update of attachment 108489 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108489&action=review > > Looks good, except for the test. > > > LayoutTests/fast/dom/scroll-element-to-rect-centered.html:17 > > + if (window.internals) { > > + var box = document.getElementById('textbox'); > > + window.internals.scrollElementToRect(box, 50, 30, 200, 250); > > + } > > + } > > + window.addEventListener("load", doTest); > > + </script> > > + </head> > > + <body> > > + This is test<br> > > + <div style="position: relative; width: 2400px; height: 2400px; background-color: white;"> > > + <div style="position:fixed; left: 50px; top: 30px; width: 200px; height: 250px; background-color: green;"></div> > > + <div style="position:absolute; left: 600px; top: 800px;"> <input id="textbox" type="text"></div> > > I am pretty sure we can make this test dumpAsText(), to avoid pixel baselines. Can we? Agreed, we should be able to output the final scroll position as text. Created attachment 108545 [details]
Patch
(In reply to comment #15) > (From update of attachment 108489 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108489&action=review > > Looks good, except for the test. > > > LayoutTests/fast/dom/scroll-element-to-rect-centered.html:17 > > + if (window.internals) { > > + var box = document.getElementById('textbox'); > > + window.internals.scrollElementToRect(box, 50, 30, 200, 250); > > + } > > + } > > + window.addEventListener("load", doTest); > > + </script> > > + </head> > > + <body> > > + This is test<br> > > + <div style="position: relative; width: 2400px; height: 2400px; background-color: white;"> > > + <div style="position:fixed; left: 50px; top: 30px; width: 200px; height: 250px; background-color: green;"></div> > > + <div style="position:absolute; left: 600px; top: 800px;"> <input id="textbox" type="text"></div> > > I am pretty sure we can make this test dumpAsText(), to avoid pixel baselines. Can we? Done. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:1824 > > + if (focusedNode && focusedNode->isElementNode()) { > > early return? Done. Comment on attachment 108545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108545&action=review > LayoutTests/platform/chromium-linux/fast/dom/scroll-element-to-rect-centered-expected.txt:6 > +PASS rect.left is 72 > +PASS rect.top is 144 > +PASS computedLeft is 72 > +PASS computedTop is 144 > +PASS successfullyParsed is true Thanks for converting the test! Are these numbers going to be different from platform to platform? If not, why are the expectations in platform-linux? If yes -- can we do better? Can the test be more of a PASS/FAIL report, with the assessment of the exact coordinates being platform-independent? Created attachment 108557 [details]
Patch
(In reply to comment #19) > (From update of attachment 108545 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108545&action=review > > > LayoutTests/platform/chromium-linux/fast/dom/scroll-element-to-rect-centered-expected.txt:6 > > +PASS rect.left is 72 > > +PASS rect.top is 144 > > +PASS computedLeft is 72 > > +PASS computedTop is 144 > > +PASS successfullyParsed is true > > Thanks for converting the test! Are these numbers going to be different from platform to platform? If not, why are the expectations in platform-linux? If yes -- can we do better? Can the test be more of a PASS/FAIL report, with the assessment of the exact coordinates being platform-independent? Added to all platforms. I think it should pass all tests. Comment on attachment 108557 [details]
Patch
Let's give it a whirl.
Comment on attachment 108557 [details] Patch Clearing flags on attachment: 108557 Committed r95890: <http://trac.webkit.org/changeset/95890> All reviewed patches have been landed. Closing bug. The test added by this patch is failing on Snow Leopard: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r95957%20(33384)/results.html (In reply to comment #25) > The test added by this patch is failing on Snow Leopard: > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r95957%20(33384)/results.html hi.. thanks for bringing this to my attention. I think the test is failing because the frame doesnot have enough room to scroll and hence cannot position the element correctly. This should be easily fixable by increasing the size of the div in the test. I'll try to get hold of a machine to try this fix on |