Bug 118723

Summary:

[CSS Regions] Convert percentage-margins-* tests to ref-tests

Product:

WebKit

Reporter:

Andrei Bucur <abucur>

Component:

WebKit Misc.

Assignee:

Andrei Bucur <abucur>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, commit-queue, gyuyoung.kim, mihnea, rakuco, WebkitBugTracker

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

118975

Bug Blocks:

57312

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Description Andrei Bucur 2013-07-16 02:21:31 PDT

fast/regions/percentage-margins-variable-width-regions.html
fast/regions/percentage-margins-rtl-variable-width-regions.html
fast/regions/percentage-margins-mixed-rtl-dominant-regions.html
fast/regions/percentage-margins-mixed-ltr-dominant-regions.html

Should be ref tests

Comment 1 Andrei Bucur 2013-07-22 04:52:28 PDT

Created attachment 207239 [details]
Patch

Comment 2 WebKit Commit Bot 2013-07-22 07:01:22 PDT

Comment on attachment 207239 [details]
Patch

Clearing flags on attachment: 207239

Committed r152968: <http://trac.webkit.org/changeset/152968>

Comment 3 WebKit Commit Bot 2013-07-22 07:01:24 PDT

All reviewed patches have been landed.  Closing bug.

Comment 4 WebKit Commit Bot 2013-07-22 10:58:30 PDT

Re-opened since this is blocked by bug 118975

Comment 5 Andrei Bucur 2013-07-23 00:51:47 PDT

Created attachment 207308 [details]
Patch

Comment 6 Mihnea Ovidenie 2013-07-23 04:12:02 PDT

(In reply to comment #5)
> Created an attachment (id=207308) [details]
> Patch

Looks good. I have 3 comments:
-> using Ahem font, makes the test less readable, maybe you should add a description to the tests about the expected behavior? Or a note in changelog about what these tests are intended to check? 
-> I prefer to use the monospace font instead of Ahem, usually font: 16px/1.25 monospace, but it is just a preference.
-> why are you using <span style="line-height: 100px;">XXXXXX</span> in *-expected.html files? Do you need them in all the boxes?

Comment 7 Andrei Bucur 2013-07-23 08:53:53 PDT

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=207308) [details] [details]
> > Patch
> 
> Looks good. I have 3 comments:
> -> using Ahem font, makes the test less readable, maybe you should add a description to the tests about the expected behavior? Or a note in changelog about what these tests are intended to check? 
Will do before landing the patch.
> -> I prefer to use the monospace font instead of Ahem, usually font: 16px/1.25 monospace, but it is just a preference.
I've noticed differences between platforms. It's important to really have the same text in each region as the ref test.
> -> why are you using <span style="line-height: 100px;">XXXXXX</span> in *-expected.html files? Do you need them in all the boxes?
To push the bottom border further down and then clip it with overflow: hidden. This way the ref test divs look like they are fragmented.

Comment 8 Andrei Bucur 2013-07-23 08:58:50 PDT

Created attachment 207331 [details]
Patch

Comment 9 WebKit Commit Bot 2013-07-23 10:35:34 PDT

Comment on attachment 207331 [details]
Patch

Clearing flags on attachment: 207331

Committed r153057: <http://trac.webkit.org/changeset/153057>

Comment 10 WebKit Commit Bot 2013-07-23 10:35:38 PDT

All reviewed patches have been landed.  Closing bug.