Bug 121841 - [CSS Regions] Layout Test for selecting text in 2 regions
Summary: [CSS Regions] Layout Test for selecting text in 2 regions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on: 122099 122128
Blocks: 118668
  Show dependency treegraph
 
Reported: 2013-09-24 08:27 PDT by Manuel Rego Casasnovas
Modified: 2013-10-03 14:05 PDT (History)
9 users (show)

See Also:


Attachments
Patch (7.13 KB, patch)
2013-09-24 08:35 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (6.91 KB, patch)
2013-10-03 00:10 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (6.81 KB, patch)
2013-10-03 03:47 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (6.85 KB, patch)
2013-10-03 13:34 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2013-09-24 08:27:40 PDT
As part of the effort to improve test coverage for selection in regions I'm providing a new ref-test using regions and absolute positions.
Comment 1 Manuel Rego Casasnovas 2013-09-24 08:35:20 PDT
Created attachment 212470 [details]
Patch
Comment 2 Mihnea Ovidenie 2013-09-25 00:51:56 PDT
Comment on attachment 212470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212470&action=review

> LayoutTests/ChangeLog:1
> +2013-09-24  Manuel Rego Casasnovas  <rego@igalia.com>

(Informal review)
Looks good!
Since we will probably add more tests for selection, should we create a folder for these tests inside fast/regions?

> LayoutTests/fast/regions/selecting-text-in-2-regions-expected.html:42
> +        function select(fromId, toId) {

Since you will probably use this function in more than one place, it makes sense to add a helper js file to use it for selection tests.
The same thing goes for the styles used, if you plan to use them further, maybe you should add a css file that you can include.
Comment 3 Manuel Rego Casasnovas 2013-09-26 01:49:44 PDT
(In reply to comment #2)
> Looks good!
> Since we will probably add more tests for selection, should we create a folder for these tests inside fast/regions?

Yes, I agree. I'll do it in a separate bug before landing this.

> > LayoutTests/fast/regions/selecting-text-in-2-regions-expected.html:42
> > +        function select(fromId, toId) {
> 
> Since you will probably use this function in more than one place, it makes sense to add a helper js file to use it for selection tests.
> The same thing goes for the styles used, if you plan to use them further, maybe you should add a css file that you can include.

I agree too. I'll move common stuff to separated files.

Thanks for the review.
Comment 4 Manuel Rego Casasnovas 2013-10-03 00:10:48 PDT
Created attachment 213233 [details]
Patch

Uploading new version of the test using helper.js and region-style.css.
Comment 5 Mihnea Ovidenie 2013-10-03 00:37:09 PDT
Comment on attachment 213233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213233&action=review

> LayoutTests/ChangeLog:3
> +        [CSS Regions] Layout Test for selecting text in 2 regions

Looks good.

> LayoutTests/ChangeLog:18
> +        (.break):

Please remove these from changelog.

> LayoutTests/fast/regions/resources/helper.js:244
> +function onMouseUpSetSelection(elementId) {

This function name is a bit misleading, you are logging the content of window selection object inside the passed element. Maybe you can find a better name?

> LayoutTests/fast/regions/resources/region-style.css:92
> +.cyanBigBox {

My opinion is that using lightgray as a background instead of cyan makes selection easier to spot when you load the test into the browser.
Comment 6 Manuel Rego Casasnovas 2013-10-03 03:47:47 PDT
Created attachment 213238 [details]
Patch
Comment 7 Manuel Rego Casasnovas 2013-10-03 06:07:58 PDT
gtk-wk2 EWS error is unrelated to this patch.
Comment 8 Alexandru Chiculita 2013-10-03 13:26:41 PDT
Comment on attachment 213238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213238&action=review

> LayoutTests/fast/regions/selection/selecting-text-in-2-regions.html:1
> +<html>

nit: I've seen that reviewers usually ask for html5 doctype.

We should add some tests for the other writting modes too.
Comment 9 Manuel Rego Casasnovas 2013-10-03 13:34:34 PDT
Created attachment 213292 [details]
Patch

Patch for landing adding HTML5 doctype.
Comment 10 WebKit Commit Bot 2013-10-03 14:05:43 PDT
Comment on attachment 213292 [details]
Patch

Clearing flags on attachment: 213292

Committed r156857: <http://trac.webkit.org/changeset/156857>
Comment 11 WebKit Commit Bot 2013-10-03 14:05:47 PDT
All reviewed patches have been landed.  Closing bug.