Bug 93108

Summary: [chromium] Add a test to WebFrameTest for selectRange and visiblePositionForWindowPoint.
Product: WebKit Reporter: Oli Lan <olilan>
Component: New BugsAssignee: Oli Lan <olilan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, husky, olilan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 93154    
Bug Blocks: 93998    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Oli Lan
Reported 2012-08-03 06:38:50 PDT
[chromium] Add a test to WebFrameTest for selectRange and visiblePositionForWindowPoint.
Attachments
Patch (6.02 KB, patch)
2012-08-03 06:45 PDT, Oli Lan 		no flags
DetailsFormatted DiffDiff
Patch (9.34 KB, patch)
2012-08-14 08:00 PDT, Iain Merrick 		no flags
DetailsFormatted DiffDiff
Patch (9.03 KB, patch)
2012-08-14 09:37 PDT, Iain Merrick 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Oli Lan
Comment 1 2012-08-03 06:45:03 PDT
Created attachment 156354 [details] Patch
Oli Lan
Comment 2 2012-08-03 06:46:43 PDT
As mentioned in the changelog, this tests https://bugs.webkit.org/show_bug.cgi?id=79117
Adam Barth
Comment 3 2012-08-03 09:51:45 PDT
Comment on attachment 156354 [details] Patch Thanks for the test!
WebKit Review Bot
Comment 4 2012-08-03 11:36:46 PDT
Comment on attachment 156354 [details] Patch Clearing flags on attachment: 156354 Committed r124632: <http://trac.webkit.org/changeset/124632>
WebKit Review Bot
Comment 5 2012-08-03 11:36:49 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 6 2012-08-03 13:22:02 PDT
Re-opened since this is blocked by 93154
Iain Merrick
Comment 7 2012-08-14 08:00:17 PDT
Created attachment 158330 [details] Patch
Iain Merrick
Comment 8 2012-08-14 08:08:42 PDT
This is an update of Oli's patch, hopefully passing reliably on all platforms. The crucial changes: - We manually set the font size and view size. The defaults are all 0, which messes up the rendering. - We subtract (1,1) from the selection endpoint. This is ugly, but I think it's correct (see FIXME in bottomRightMinusOne). And some additional cleanup: - "<!DOCTYPE html>" added to test files - Calling webView->close() after each test - Added extra tests for editable selections (testing code added in https://bugs.webkit.org/show_bug.cgi?id=92513)
Iain Merrick
Comment 9 2012-08-14 08:12:07 PDT
Note: I tried setting the font and view size in createWebViewAndLoad() in FrameTestHelpers.cpp, but that broke several of the other tests. They're worryingly fragile if they *expect* a zero-width window, so the page is laid out in a narrow vertical line. It might be worth making all the tests in WebFrameTest more robust, but I figure that's outside the scope of this patch.
Peter Beverloo
Comment 10 2012-08-14 09:15:09 PDT
Comment on attachment 158330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158330&action=review Some nits.. thanks for the tests! > Source/WebKit/chromium/tests/WebFrameTest.cpp:976 > + // FIXME: If we don't subtract 1 from the x- and y-coordinates of the Should the API be fixed? If so, it may be a good idea to file a bug about it doesn't fly under the radar (regardless who's going to fix it). > Source/WebKit/chromium/tests/WebFrameTest.cpp:997 > + EXPECT_EQ("Some test text for testing.", std::string(frame->selectionAsText().utf8().data())); Maybe add a "std::string selectionText(WebFrame*)" helper? I count 14 uses. > Source/WebKit/chromium/tests/data/select_range_basic.html:9 > +<script> This <script> should be somewhere in the <body> or <head> element. > Source/WebKit/chromium/tests/data/select_range_editable.html:11 > +<script> dito. > Source/WebKit/chromium/tests/data/select_range_iframe.html:8 > +<script> dito. > Source/WebKit/chromium/tests/data/select_range_scroll.html:5 > +<script> dito.
Iain Merrick
Comment 11 2012-08-14 09:22:20 PDT
Comment on attachment 158330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158330&action=review >> Source/WebKit/chromium/tests/WebFrameTest.cpp:976 >> + // FIXME: If we don't subtract 1 from the x- and y-coordinates of the > > Should the API be fixed? If so, it may be a good idea to file a bug about it doesn't fly under the radar (regardless who's going to fix it). Hmm, I'd like to discuss it with people who understand the code before suggesting a specific fix. In general, I think this will be covered by the ongoing SelectRange work (https://code.google.com/p/chromium/issues/detail?id=138939), so it will stay on my radar. >> Source/WebKit/chromium/tests/WebFrameTest.cpp:997 >> + EXPECT_EQ("Some test text for testing.", std::string(frame->selectionAsText().utf8().data())); > > Maybe add a "std::string selectionText(WebFrame*)" helper? I count 14 uses. Good idea, done. >> Source/WebKit/chromium/tests/data/select_range_basic.html:9 >> +<script> > > This <script> should be somewhere in the <body> or <head> element. Done (along with the others)
Iain Merrick
Comment 12 2012-08-14 09:37:12 PDT
Created attachment 158348 [details] Patch
Adam Barth
Comment 13 2012-08-14 10:09:53 PDT
Comment on attachment 158348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158348&action=review Thanks for following up on the tests. > Source/WebKit/chromium/tests/WebFrameTest.cpp:972 > +static WebPoint topLeft(const WebRect& rect) > +{ > + return WebPoint(rect.x, rect.y); > +} Should we add this function to WebRect? > Source/WebKit/chromium/tests/WebFrameTest.cpp:980 > + // FIXME: If we don't subtract 1 from the x- and y-coordinates of the > + // selection bounds, selectRange() will select the *next* element. That's > + // strictly correct, as hit-testing checks the pixel to the lower-right of > + // the input coordinate, but it's a wart on the API. > + return WebPoint(rect.x + rect.width - 1, rect.y + rect.height - 1); Interesting.
Iain Merrick
Comment 14 2012-08-14 10:29:05 PDT
> > Source/WebKit/chromium/tests/WebFrameTest.cpp:980 > > + // FIXME: If we don't subtract 1 from the x- and y-coordinates of the > > + // selection bounds, selectRange() will select the *next* element. That's > > + // strictly correct, as hit-testing checks the pixel to the lower-right of > > + // the input coordinate, but it's a wart on the API. > > + return WebPoint(rect.x + rect.width - 1, rect.y + rect.height - 1); > > Interesting. I'm thinking an API change might improve this. I've posted some thoughts on https://code.google.com/p/chromium/issues/detail?id=138939
Adam Barth
Comment 15 2012-08-14 10:32:56 PDT
> I'm thinking an API change might improve this. I've posted some thoughts on https://code.google.com/p/chromium/issues/detail?id=138939 It's somewhat lame that the bug isn't publicly visible. Shall we have this discussion in a public forum, such as bugs.webkit.org?
Iain Merrick
Comment 16 2012-08-14 10:45:25 PDT
(In reply to comment #15) > > I'm thinking an API change might improve this. I've posted some thoughts on https://code.google.com/p/chromium/issues/detail?id=138939 > > It's somewhat lame that the bug isn't publicly visible. Shall we have this discussion in a public forum, such as bugs.webkit.org? Didn't realize it was locked. Nothing secret here, so I've moved my comments to https://bugs.webkit.org/show_bug.cgi?id=93998
WebKit Review Bot
Comment 17 2012-08-14 10:55:18 PDT
Comment on attachment 158348 [details] Patch Clearing flags on attachment: 158348 Committed r125581: <http://trac.webkit.org/changeset/125581>
WebKit Review Bot
Comment 18 2012-08-14 10:55:23 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.