| Summary: | [CSS Regions] Layout Test for selecting text in 2 regions | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||||||
| Component: | CSS | Assignee: | 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
Manuel Rego Casasnovas
2013-09-24 08:27:40 PDT
Created attachment 212470 [details]
Patch
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. (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. Created attachment 213233 [details]
Patch
Uploading new version of the test using helper.js and region-style.css.
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. Created attachment 213238 [details]
Patch
gtk-wk2 EWS error is unrelated to this patch. 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. Created attachment 213292 [details]
Patch
Patch for landing adding HTML5 doctype.
Comment on attachment 213292 [details] Patch Clearing flags on attachment: 213292 Committed r156857: <http://trac.webkit.org/changeset/156857> All reviewed patches have been landed. Closing bug. |