Bug 94480

Summary:

[css3-text] Add repaint tests for -webkit-text-decoration-line

Product:

WebKit

Reporter:

Bruno Abinader (history only) <bruno.abinader>

Component:

CSS

Assignee:

Bruno Abinader (history only) <bruno.abinader>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, gyuyoung.kim, igor.oliveira, jchaffraix, kenneth, rakuco, vestbo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

94093

Bug Blocks:

58491

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch (EWS run only)	none
Archive of layout-test-results from gce-cr-linux-05	none
Archive of layout-test-results from gce-cr-linux-04	none
Patch	none
Patch	none

Bruno Abinader (history only)

Reported 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.

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Bruno Abinader (history only)

Comment 1 2012-08-20 08:41:56 PDT

Created attachment 159436 [details] Patch Proposed patch.

Julien Chaffraix

Comment 2 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.

Bruno Abinader (history only)

Comment 3 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?

Bruno Abinader (history only)

Comment 4 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.

Julien Chaffraix

Comment 5 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;

Bruno Abinader (history only)

Comment 6 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!

Bruno Abinader (history only)

Comment 7 2012-08-20 15:54:36 PDT

Created attachment 159549 [details] Patch Fixed typo in 'font' usage (now using Ahem - cross-platform).

Bruno Abinader (history only)

Comment 8 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.

Julien Chaffraix

Comment 9 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.

WebKit Review Bot

Comment 10 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

WebKit Review Bot

Comment 11 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

Build Bot

Comment 12 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

WebKit Review Bot

Comment 13 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

WebKit Review Bot

Comment 14 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

Bruno Abinader (history only)

Comment 15 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   is rendered erroneously.

Bruno Abinader (history only)

Comment 16 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.

Kenneth Rohde Christiansen

Comment 17 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?

Kenneth Rohde Christiansen

Comment 18 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?

Kenneth Rohde Christiansen

Comment 19 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

Bruno Abinader (history only)

Comment 20 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.

Kenneth Rohde Christiansen

Comment 21 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.

Bruno Abinader (history only)

Comment 22 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.

Bruno Abinader (history only)

Comment 23 2012-08-21 07:22:28 PDT

Created attachment 159684 [details] Patch Fixed issues pointed out by Kenneth in comment 17.

Bruno Abinader (history only)

Comment 24 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!

Julien Chaffraix

Comment 25 2012-08-21 10:43:30 PDT

Comment on attachment 159684 [details] Patch Fine with the latest change. Thanks for the help Kenneth.

WebKit Review Bot

Comment 26 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>

WebKit Review Bot

Comment 27 2012-08-21 16:55:09 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.