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

Description Oli Lan 2012-08-03 06:38:50 PDT
[chromium] Add a test to WebFrameTest for selectRange and visiblePositionForWindowPoint.
Comment 1 Oli Lan 2012-08-03 06:45:03 PDT
Created attachment 156354 [details]
Patch
Comment 2 Oli Lan 2012-08-03 06:46:43 PDT
As mentioned in the changelog, this tests https://bugs.webkit.org/show_bug.cgi?id=79117
Comment 3 Adam Barth 2012-08-03 09:51:45 PDT
Comment on attachment 156354 [details]
Patch

Thanks for the test!
Comment 4 WebKit Review Bot 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>
Comment 5 WebKit Review Bot 2012-08-03 11:36:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 WebKit Review Bot 2012-08-03 13:22:02 PDT
Re-opened since this is blocked by 93154
Comment 7 Iain Merrick 2012-08-14 08:00:17 PDT
Created attachment 158330 [details]
Patch
Comment 8 Iain Merrick 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)
Comment 9 Iain Merrick 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.
Comment 10 Peter Beverloo 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.
Comment 11 Iain Merrick 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)
Comment 12 Iain Merrick 2012-08-14 09:37:12 PDT
Created attachment 158348 [details]
Patch
Comment 13 Adam Barth 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.
Comment 14 Iain Merrick 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
Comment 15 Adam Barth 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?
Comment 16 Iain Merrick 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
Comment 17 WebKit Review Bot 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>
Comment 18 WebKit Review Bot 2012-08-14 10:55:23 PDT
All reviewed patches have been landed.  Closing bug.