Bug 68198

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Varun Jain

Reported 2011-09-15 16:27:15 PDT

On possible way is to move the logic to frameView and test using window.internals

Attachments
Patch (12.25 KB, patch) 2011-09-21 13:21 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (16.66 KB, patch) 2011-09-21 13:47 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (16.53 KB, patch) 2011-09-22 07:37 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (18.50 KB, patch) 2011-09-22 12:08 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (17.28 KB, patch) 2011-09-23 10:09 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (17.28 KB, patch) 2011-09-23 10:36 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (16.24 KB, patch) 2011-09-23 15:24 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (16.12 KB, patch) 2011-09-23 16:36 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Varun Jain

Comment 1 2011-09-21 13:21:14 PDT

Created attachment 108219 [details] Patch

Varun Jain

Comment 2 2011-09-21 13:25:02 PDT

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.

Fady Samuel

Comment 3 2011-09-21 13:26:19 PDT

(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.

Varun Jain

Comment 4 2011-09-21 13:47:57 PDT

Created attachment 108227 [details] Patch

Varun Jain

Comment 5 2011-09-21 13:48:26 PDT

(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.

Gustavo Noronha (kov)

Comment 6 2011-09-21 16:13:35 PDT

Comment on attachment 108227 [details] Patch Attachment 108227 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9773901

Varun Jain

Comment 7 2011-09-22 07:37:26 PDT

Created attachment 108330 [details] Patch

Gustavo Noronha (kov)

Comment 8 2011-09-22 07:50:40 PDT

Comment on attachment 108330 [details] Patch Attachment 108330 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9792319

Varun Jain

Comment 9 2011-09-22 12:08:47 PDT

Created attachment 108378 [details] Patch

Gustavo Noronha (kov)

Comment 10 2011-09-22 13:36:43 PDT

Comment on attachment 108378 [details] Patch Attachment 108378 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9796377

Varun Jain

Comment 11 2011-09-23 08:54:23 PDT

ping?

Varun Jain

Comment 12 2011-09-23 10:09:06 PDT

Created attachment 108486 [details] Patch

Gustavo Noronha (kov)

Comment 13 2011-09-23 10:31:08 PDT

Comment on attachment 108486 [details] Patch Attachment 108486 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9822109

Varun Jain

Comment 14 2011-09-23 10:36:33 PDT

Created attachment 108489 [details] Patch

Dimitri Glazkov (Google)

Comment 15 2011-09-23 11:28:44 PDT

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?

Fady Samuel

Comment 16 2011-09-23 12:59:10 PDT

(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.

Varun Jain

Comment 17 2011-09-23 15:24:47 PDT

Created attachment 108545 [details] Patch

Varun Jain

Comment 18 2011-09-23 15:25:17 PDT

(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.

Dimitri Glazkov (Google)

Comment 19 2011-09-23 15:30:25 PDT

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?

Varun Jain

Comment 20 2011-09-23 16:36:51 PDT

Created attachment 108557 [details] Patch

Varun Jain

Comment 21 2011-09-23 16:37:21 PDT

(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.

Dimitri Glazkov (Google)

Comment 22 2011-09-23 16:42:41 PDT

Comment on attachment 108557 [details] Patch Let's give it a whirl.

WebKit Review Bot

Comment 23 2011-09-23 17:23:01 PDT

Comment on attachment 108557 [details] Patch Clearing flags on attachment: 108557 Committed r95890: <http://trac.webkit.org/changeset/95890>

WebKit Review Bot

Comment 24 2011-09-23 17:23:07 PDT

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 25 2011-09-26 10:42:18 PDT

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

Varun Jain

Comment 26 2011-09-26 11:03:40 PDT

(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

Note You need to log in before you can comment on or make changes to this bug.