| Summary: | [chromium] Add a test to WebFrameTest for selectRange and visiblePositionForWindowPoint. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Oli Lan <olilan> | ||||||||
| Component: | New Bugs | Assignee: | 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
Oli Lan
2012-08-03 06:38:50 PDT
Created attachment 156354 [details]
Patch
As mentioned in the changelog, this tests https://bugs.webkit.org/show_bug.cgi?id=79117 Comment on attachment 156354 [details]
Patch
Thanks for the test!
Comment on attachment 156354 [details] Patch Clearing flags on attachment: 156354 Committed r124632: <http://trac.webkit.org/changeset/124632> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 93154 Created attachment 158330 [details]
Patch
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) 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. 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. 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) Created attachment 158348 [details]
Patch
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. > > 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 > 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?
(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 Comment on attachment 158348 [details] Patch Clearing flags on attachment: 158348 Committed r125581: <http://trac.webkit.org/changeset/125581> All reviewed patches have been landed. Closing bug. |