Bug 94480 - [css3-text] Add repaint tests for -webkit-text-decoration-line
Summary: [css3-text] Add repaint tests for -webkit-text-decoration-line
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bruno Abinader (history only)
URL:
Keywords:
Depends on: 94093
Blocks: 58491
  Show dependency treegraph
 
Reported: 2012-08-20 07:36 PDT by Bruno Abinader (history only)
Modified: 2012-08-21 16:55 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.52 KB, patch)
2012-08-20 08:41 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (16.69 KB, patch)
2012-08-20 12:00 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2012-08-20 15:54 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (EWS run only) (17.29 KB, patch)
2012-08-20 16:29 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (636.27 KB, application/zip)
2012-08-20 18:17 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-04 (430.68 KB, application/zip)
2012-08-20 19:28 PDT, WebKit Review Bot
no flags Details
Patch (6.20 KB, patch)
2012-08-21 06:26 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (6.44 KB, patch)
2012-08-21 07:22 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bruno Abinader (history only) 2012-08-20 07:36:19 PDT
Changeset r125205 introduced new CSS3 property "-webkit-text-decoration-line". This bug intends to add repaint layout tests for this property.
Comment 1 Bruno Abinader (history only) 2012-08-20 08:41:56 PDT
Created attachment 159436 [details]
Patch

Proposed patch.
Comment 2 Julien Chaffraix 2012-08-20 10:35:22 PDT
Comment on attachment 159436 [details]
Patch

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

Where is the expected result? Also your test - as it is written - is not cross-platform. It would be better if it were, see http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree.

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:20
> +        <p><span id="test-underline" style="text-decoration: none;">underline</span></p>

Please add a description about what you are testing.
Comment 3 Bruno Abinader (history only) 2012-08-20 11:22:53 PDT
(In reply to comment #2)
> (From update of attachment 159436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159436&action=review
> 
> Where is the expected result? Also your test - as it is written - is not cross-platform. It would be better if it were, see http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree.

All tests in fast/css3-text-decoration are skipped by all platforms for now. I'll add the Ahem/non-smooth trick as described on wiki, however, as discussed with James on IRC, even this won't make sure every platform gets the same expected result. Shall I create them anyway?
Comment 4 Bruno Abinader (history only) 2012-08-20 12:00:16 PDT
Created attachment 159483 [details]
Patch

Added expected results (using Qt platform, might vary from platform) and test description as HTML comments.
Comment 5 Julien Chaffraix 2012-08-20 15:01:22 PDT
Comment on attachment 159483 [details]
Patch

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

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line-expected.txt:1
> +layer at (0,0) size 800x600

I am fine with checking this result next to the test if it is more or less platform independent of course.

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:19
> +    <body onload="runRepaintTest();" style="font: Ahem 10px; -webkit-font-smoothing: none;">

The 'font' property is not applied above, Ahem is a very recognizable font (all the characters are squares which explains its properties). This is because your 'font' declaration is wrong: 'font-size' and 'font-family' should be inverted:

font: 10px Ahem;
Comment 6 Bruno Abinader (history only) 2012-08-20 15:17:32 PDT
(In reply to comment #5)
> (From update of attachment 159483 [details])
> > LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:19
> > +    <body onload="runRepaintTest();" style="font: Ahem 10px; -webkit-font-smoothing: none;">
> 
> The 'font' property is not applied above, Ahem is a very recognizable font (all the characters are squares which explains its properties). This is because your 'font' declaration is wrong: 'font-size' and 'font-family' should be inverted:
> 
> font: 10px Ahem;

Oops :\ indeed, fixing right away!
Comment 7 Bruno Abinader (history only) 2012-08-20 15:54:36 PDT
Created attachment 159549 [details]
Patch

Fixed typo in 'font' usage (now using Ahem - cross-platform).
Comment 8 Bruno Abinader (history only) 2012-08-20 16:29:01 PDT
Created attachment 159558 [details]
Patch (EWS run only)

Patch for EWS run with CSS3_TEXT_DECORATION feature flag enabled.
Comment 9 Julien Chaffraix 2012-08-20 16:41:54 PDT
Comment on attachment 159549 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        * fast/css3-text-decoration/repaint/repaint-text-decoration-line-expected.png: Added.

Btw, the pixel output is not that of a repaint test. It looks like Qt doesn't implement the repainting logic (see other repaint/ results on Chromium or Mac). I would advise generating it from a Chromium or Mac build.

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line-expected.txt:1
> +layer at (0,0) size 800x600

Technically we don't need this tree dump so we may as well disable the dumping using:

if (window.testRunner)
    testRunner.dumpAsText(true);

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:26
> +        <p><span id="test-invalid-inherit" style="text-decoration: underline;">undecorated text</span></p>

Nit: it would be better just to use space or &nbsp so that we properly see the line.
Comment 10 WebKit Review Bot 2012-08-20 18:17:12 PDT
Comment on attachment 159558 [details]
Patch (EWS run only)

Attachment 159558 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13547235

New failing tests:
fast/css3-text-decoration/repaint/repaint-text-decoration-line.html
Comment 11 WebKit Review Bot 2012-08-20 18:17:19 PDT
Created attachment 159583 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 12 Build Bot 2012-08-20 18:41:36 PDT
Comment on attachment 159558 [details]
Patch (EWS run only)

Attachment 159558 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13547240
Comment 13 WebKit Review Bot 2012-08-20 19:27:58 PDT
Comment on attachment 159558 [details]
Patch (EWS run only)

Attachment 159558 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13538893

New failing tests:
fast/css3-text-decoration/repaint/repaint-text-decoration-line.html
Comment 14 WebKit Review Bot 2012-08-20 19:28:04 PDT
Created attachment 159590 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 15 Bruno Abinader (history only) 2012-08-21 06:26:02 PDT
Created attachment 159676 [details]
Patch

Fixes: Now adding expected results from Chromium Linux build (matches build bot expectations); "Using testRunner.dumpAsText(true);" to avoid tree dump; Using "> <" as Ahem font does not render a glyph with only empty spaces and &nbsp; is rendered erroneously.
Comment 16 Bruno Abinader (history only) 2012-08-21 06:33:55 PDT
Btw: Don't bother about the cr-linux failures, as the previous ESW run was using the Qt-based expected results, which differs from cr-linux. I am not going to upload a new EWS run for the new patch because it might conflict with the soon-to-be-landed bug 94483 - [EFL] Enable CSS Text Decoration by default.
Comment 17 Kenneth Rohde Christiansen 2012-08-21 06:35:57 PDT
Comment on attachment 159676 [details]
Patch

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

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line-expected.txt:3
> +> <
> +
> +> <

Isn't it possible to avoid it writing things? how you are using dumpAsText?

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:3
> +    <head>

What about adding <link rel="help" href="RELEVANT_SPEC_SECTION"> here? as suggested by http://wiki.csswg.org/test/format

http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree also suggests to write the bug number. You could use the same solution

<link rel="help" href="http://webkit.org/b/94480"> I guess

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:6
> +            if (window.testRunner)
> +            testRunner.dumpAsText(true);

indentation issue?

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:9
> +        <script type="text/javascript">

why specifying that it is javascript here?
Comment 18 Kenneth Rohde Christiansen 2012-08-21 06:36:59 PDT
Comment on attachment 159676 [details]
Patch

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

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:24
> +    <body onload="runRepaintTest();" style="font: 10px Ahem; -webkit-font-smoothing: none;">

why this style when you are not writing any text?
Comment 19 Kenneth Rohde Christiansen 2012-08-21 06:43:35 PDT
(In reply to comment #18)
> (From update of attachment 159676 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159676&action=review
> 
> > LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:24
> > +    <body onload="runRepaintTest();" style="font: 10px Ahem; -webkit-font-smoothing: none;">
> 
> why this style when you are not writing any text?

Ah this might affect the size of the space right
Comment 20 Bruno Abinader (history only) 2012-08-21 06:47:50 PDT
(In reply to comment #17)
> (From update of attachment 159676 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159676&action=review
> 
> > LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line-expected.txt:3
> > +> <
> > +
> > +> <
> 
> Isn't it possible to avoid it writing things? how you are using dumpAsText?

I am using it as Julien suggested in comment 9 (setting it to true). Basically we don't need any output from DRT, however it might still require some textual expectation file otherwise the layout test would fail.

> 
> > LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:3
> > +    <head>
> 
> What about adding <link rel="help" href="RELEVANT_SPEC_SECTION"> here? as suggested by http://wiki.csswg.org/test/format
> 
> http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree also suggests to write the bug number. You could use the same solution
> 
> <link rel="help" href="http://webkit.org/b/94480"> I guess

This would be a great idea if we're using a readable font. However, for cross-platform purposes, Julien suggested us to use "Ahem" font (see comment 2). This would cause the bug link to be unreadable, but I can still add it as a HTML comment though.

> 
> > LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:6
> > +            if (window.testRunner)
> > +            testRunner.dumpAsText(true);
> 
> indentation issue?

Actually, I've seen some layout tests which follows the indentation from HTML to JavaScript, and some others who doesn't. In this case I try to follow from HTML, but I can change it if it sounds wrong :)

> 
> > LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-line.html:9
> > +        <script type="text/javascript">
> 
> why specifying that it is javascript here?

No specific reason, just following template from other repaint tests, I guess we can safely remove this as well.
Comment 21 Kenneth Rohde Christiansen 2012-08-21 06:52:48 PDT
> > > +> <
> > 
> > Isn't it possible to avoid it writing things? how you are using dumpAsText?
> 
> I am using it as Julien suggested in comment 9 (setting it to true). Basically we don't need any output from DRT, however it might still require some textual expectation file otherwise the layout test would fail.

I means "now" not "how". I understand what you are doing, I just wondered whether we could get rid of the > <'s

> This would be a great idea if we're using a readable font. However, for cross-platform purposes, Julien suggested us to use "Ahem" font (see comment 2). This would cause the bug link to be unreadable, but I can still add it as a HTML comment though.

I guess that is fine as it can be seem from the source, which is what matters.
Comment 22 Bruno Abinader (history only) 2012-08-21 07:04:59 PDT
(In reply to comment #21)
> I means "now" not "how". I understand what you are doing, I just wondered whether we could get rid of the > <'s

This looks like necessary evil, indeed :/ I could only produce visible feedback on text decoration using Ahem font by adding spaces between characters, otherwise it would be either black squares or transparent spaces.
Comment 23 Bruno Abinader (history only) 2012-08-21 07:22:28 PDT
Created attachment 159684 [details]
Patch

Fixed issues pointed out by Kenneth in comment 17.
Comment 24 Bruno Abinader (history only) 2012-08-21 07:42:50 PDT
Note: Kept Julien's review as there were only cosmetic changes (expectation results were the same). Thanks Kenneth for the last minute check!
Comment 25 Julien Chaffraix 2012-08-21 10:43:30 PDT
Comment on attachment 159684 [details]
Patch

Fine with the latest change. Thanks for the help Kenneth.
Comment 26 WebKit Review Bot 2012-08-21 16:55:03 PDT
Comment on attachment 159684 [details]
Patch

Clearing flags on attachment: 159684

Committed r126224: <http://trac.webkit.org/changeset/126224>
Comment 27 WebKit Review Bot 2012-08-21 16:55:09 PDT
All reviewed patches have been landed.  Closing bug.