Bug 121841

Summary: [CSS Regions] Layout Test for selecting text in 2 regions
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: CSSAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: achicu, commit-queue, darin, hyatt, jfernandez, mihnea, philn, WebkitBugTracker, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 122099, 122128    
Bug Blocks: 118668    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Manuel Rego Casasnovas
Reported 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.
Attachments
Patch (7.13 KB, patch)
2013-09-24 08:35 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (6.91 KB, patch)
2013-10-03 00:10 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (6.81 KB, patch)
2013-10-03 03:47 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (6.85 KB, patch)
2013-10-03 13:34 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1 2013-09-24 08:35:20 PDT
Created attachment 212470 [details] Patch
Mihnea Ovidenie
Comment 2 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.
Manuel Rego Casasnovas
Comment 3 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.
Manuel Rego Casasnovas
Comment 4 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.
Mihnea Ovidenie
Comment 5 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.
Manuel Rego Casasnovas
Comment 6 2013-10-03 03:47:47 PDT
Created attachment 213238 [details] Patch
Manuel Rego Casasnovas
Comment 7 2013-10-03 06:07:58 PDT
gtk-wk2 EWS error is unrelated to this patch.
Alexandru Chiculita
Comment 8 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.
Manuel Rego Casasnovas
Comment 9 2013-10-03 13:34:34 PDT
Created attachment 213292 [details] Patch Patch for landing adding HTML5 doctype.
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2013-10-03 14:05:47 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.