Bug 93108 - [chromium] Add a test to WebFrameTest for selectRange and visiblePositionForWindowPoint.
Summary: [chromium] Add a test to WebFrameTest for selectRange and visiblePositionForW...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oli Lan
URL:
Keywords:
Depends on: 93154
Blocks: 93998
  Show dependency treegraph
 
Reported: 2012-08-03 06:38 PDT by Oli Lan
Modified: 2012-08-14 10:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.02 KB, patch)
2012-08-03 06:45 PDT, Oli Lan
no flags Details | Formatted Diff | Diff
Patch (9.34 KB, patch)
2012-08-14 08:00 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff
Patch (9.03 KB, patch)
2012-08-14 09:37 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.