Bug 61699

Summary:

Multiple failures in css2.1/t1504-c543-txt-decor-00-d-g.html

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

darin, eric, hyatt, jchaffraix, joepeck, mustaf.here, pfeldman, ssandela

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Test case to reproduce the issue.	none
Patch	none
patch after updating test_expectations	none
patch after updating test_expectations	none
Patch after removing <feff> UTF-8 BOM	none
Patch	pfeldman: review-

Joseph Pecoraro

Reported 2011-05-28 20:57:36 PDT

In <LayoutTests/css2.1/t1504-c543-txt-decor-00-d-g.html> there are 2 failures: 1. There should not be red at the end of a line, but there is. 2. There should be an underline under images, but there is not. See the pixel test results: http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/css2.1/t1504-c543-txt-decor-00-d-g-expected.png

Attachments
Test case to reproduce the issue. (1.95 KB, text/html) 2011-07-01 04:57 PDT, SravanKumar	no flags	Details
Patch (84.44 KB, patch) 2012-03-20 20:48 PDT, SravanKumar S(:sravan)	no flags	Details Formatted Diff Diff
patch after updating test_expectations (84.31 KB, patch) 2012-03-20 21:19 PDT, SravanKumar S(:sravan)	no flags	Details Formatted Diff Diff
patch after updating test_expectations (84.40 KB, patch) 2012-03-20 21:43 PDT, SravanKumar S(:sravan)	no flags	Details Formatted Diff Diff
Patch after removing <feff> UTF-8 BOM (84.40 KB, patch) 2012-03-22 17:21 PDT, SravanKumar S(:sravan)	no flags	Details Formatted Diff Diff
Patch (108.12 KB, patch) 2012-03-23 22:44 PDT, SravanKumar S(:sravan)	pfeldman: review-	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Joseph Pecoraro

Comment 1 2011-05-28 20:59:20 PDT

(In reply to comment #0) > 1. There should not be red at the end of a line, but there is. There is a comment in the source that seems to indicate that the test was wrong for this one. But no such comment for the 2nd failure.

SravanKumar

Comment 2 2011-06-24 05:48:06 PDT

The test is failing, however, the expected file and actual.png files are same w.r.t content. I mean there is a red line at the end of line in both expected.png and actual.png files, and images are not underlined in both the files. So, My doubt is 1. did we check-in incorrect -expected file for this test? If yes then is there a reason behind doing this?(Like, did we compromise on the behaviour and went ahead) 2. If no then the present issue cannot be treated as a BUG as far as content is concerned, which is what you have described. 3. In such a case the only buggy behaviour i see between -expected.png and -actual.png file is some off-set in the lay-out w.r.t each line of output. Please confirm if we have to file a different bug for point# 3.

Joseph Pecoraro

Comment 3 2011-06-24 10:46:57 PDT

Hey! Thanks for taking a look. I stick by what I said. The test says that there should be an underline under the images, but there is not. So to answer you questions: > 1. did we check-in incorrect -expected file for this test? Yes, it appears so. Probably so that the test did not show up as Red/Yellow on the bots. Failures are often checked in, with bugs filed, so that the bots are green and the issue is tracked. I did not see a bug filed on this issue, so I opened this one. Another temporary solution is to skip the test. > 2. If no then the present issue cannot be treated as a BUG as far as content is concerned, which is what you have described. I don't quite follow. Its not always the case that checked in expected results are perfect. Due to accidents, or attempts to keep the bots green failing results are sometimes checked in. I don't have any specific examples right now to support this though. > 3. In such a case the only buggy behaviour i see between -expected.png and -actual.png file is some off-set in the lay-out w.r.t each line of output. I was not seeing an issue between expected/actual. I was just seeing an issue with the expected. The checked in expected results DO NOT match what the test says it is trying to do. This bug tracks investigating why. To move this bug forward: • Are the expected results actually correct? -> Yes. Make a comment in the test saying "the test was wrong in this regard...." so that other people that come across this don't get tripped up. -> No. Fix it!

SravanKumar

Comment 4 2011-06-25 07:31:04 PDT

Hi, Yes, that is how any bug needs to be analyzed and not by just looking at what is in "expected" files. I wanted to confirm this before analyzing on this bug. Regarding "Incorrect expected" files may be we can start a different e-mail thread and have it cleaned up, and i think i can just go ahead and start analyzing this issue. Will keep posted once i get some thing concrete on this issue.

SravanKumar

Comment 5 2011-06-29 06:40:40 PDT

Hi, After Investigating the second failure (" There should be an underline under images, but there is not."), following are the observations 1. "GraphicsContext::drawLineForText" is the function that is responsible for drawing the line. 2.This function is called only by RenderText objects and not by RenderImage objects. Hence we are not seeing underline below images. Other interesting observations on Mozilla browser for same Test. The test fully passes in Mozilla browser with "<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">" line in the html file. If we remove it we see the same bugs as observed in CHROME BROWSER, IE, SAFARI(as reported in this bug). This line mentions strict DTD standard for HTML. Anybody

SravanKumar

Comment 6 2011-07-01 04:48:48 PDT

Please find the attached test case for the issue. For Issue #1(1. There should not be red at the end of a line, but there is.) As per http://www.w3.org/TR/CSS21/text.html#lining-striking-props, ("Underlines, overlines, and line-throughs are applied only to text (including white space, letter spacing, and word spacing)"), the line should draw red line as the strong tag includes a white space. Therefore, it is not an issue. We should remove the space in between the tag. For Issue #2(There should be an underline under images, but there is not.) As per http://www.w3.org/TR/CSS21/text.html#lining-striking-props, ("For example, images and inline blocks must not be underlined."), images should not be underlined. So, this is not an issue. We should modify the html test case description accordingly. If any one can confirm our analysis above, we can submit a patch after modifying the test html accordingly.

SravanKumar

Comment 7 2011-07-01 04:57:20 PDT

Created attachment 99458 [details] Test case to reproduce the issue. Attached existing test case for reference. This should be modified accordingly if our analysis above is correct

Darin Adler

Comment 8 2011-07-01 08:28:14 PDT

I believe the contents of the css2.1 are not a test suite specific to WebKit, but a test suite that comes from W3C. Changing our copy of the test suite is not helpful; it works fine as a regression test even if it states something wrong about the expected results. If the test suite is wrong we should talk to the CSS working group and figure out how to get it fixed. Also, I am not certain the behavior here is OK. I would like to hear Hyatt’s take on it.

Dave Hyatt

Comment 9 2011-07-08 09:51:05 PDT

text decoration got changed at some point. I'm not sure if the test is actually accurate. I'll need to take a look. It's definitely the case that older text decoration tests will be incorrect.

SravanKumar

Comment 10 2011-08-11 17:21:19 PDT

Hi, Pinging just to make sure that this thread does'nt die out. Would be of lot help, if Hyatt can close this :) .

SravanKumar

Comment 11 2011-09-21 20:52:14 PDT

Hi Darin/Dave Hyatt, Not sure if this thread got forgotten, Can some one among you please take the initiative of kindly taking this forward. I think Dave last mentioned that he needs to take a look if testcase is accurate or not. After that there is no progress on this bug. Kindly, please allocate some time to close this bug since most of the analysis is done. If it is being held because of some other reason, kindly let me know. Thanks in anticipation. (In reply to comment #10) > Hi, > > Pinging just to make sure that this thread does'nt die out. > Would be of lot help, if Hyatt can close this :) .

Darin Adler

Comment 12 2011-12-04 14:13:44 PST

I think Dave Hyatt just hasn’t had time to work on this yet. I don’t think this is particularly high priority unless there is some real-world problem being caused here.

SravanKumar S(:sravan)

Comment 13 2012-02-21 20:06:59 PST

Hi Darin, Thanks for responding, i understand its less priority issue for Dave. Now i have some time. I think as you mentioned Dave Hyatt should be really busy on some thing more important, i understand. So, can u please let me know what and where i should check for, to confirm ". I'm not sure if the test is actually accurate. I'll need to take a look. It's definitely the case that older text decoration tests will be incorrect." comments from Dave Hyatt. I think i can take this to closure by giving full information that is needed for any other reviewer to close this. I am just trying to close what i have started , as i read some where in some of the webkit articles that no small bug is too small, and it can hide other big bugs :). I just want to confirm that i am big fan of Dave of what he has contributed to Webkit code, and even some technical articles (like http://www.webkit.org/blog/114/webcore-rendering-i-the-basics/) that helped me a lot to get started quickly on webkit, and i am only intending to close as many bugs as possible to make WebKit better.

SravanKumar S(:sravan)

Comment 14 2012-03-20 04:31:04 PDT

Hi, The text decorations got changed. Following are the relevant links to confirm. http://lists.w3.org/Archives/Public/public-css-testsuite/2010Jan/0032.html (note that the first link there gives 301 Moved Permanently) http://test.csswg.org/suites/css2.1/20110323/html4/c543-txt-decor-000.htm http://test.csswg.org/suites/css2.1/nightly-unstable/html4/c543-txt-decor-000.htm Should i upload the patch modifying the test case in accordance to above links? or is the process different in webkit?

SravanKumar S(:sravan)

Comment 15 2012-03-20 04:48:19 PDT

Following is the new test case. <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> <html> <head> <title>CSS Test: text-decoration</title> <meta name="flags" content="image"> <link rel="help" href="http://www.w3.org/TR/REC-CSS1#text-decoration"> <link rel="author" title="CSS1 Test Suite Contributors" href="http://www.w3.org/Style/CSS/Test/CSS1/current/tsack.html"> <link rel="author" title="Ian Hickson" href="mailto:ian@hixie.ch"> <style type="text/css"> p { color: navy; } .one {text-decoration: underline;} .two {text-decoration: overline;} .three {text-decoration: line-through;} .four {text-decoration: blink;} b.five {text-decoration: none;} .six {text-decoration: underline overline;} .seven {text-decoration: underline overline line-through;} div, strong, img { color: red; } .eight { color: white; } </style> <link rel="help" href="http://www.w3.org/TR/CSS21/fonts.html#font-styling" title="15.4 Font styling: the 'font-style' property"> </head> <body> This sentence should be underlined. This sentence should be overlined. This sentence should be stricken out. This sentence should be blinking (if the UA supports that). The sentence should be underlined. This sentence should be underlined. This sentence should be underlined and overlined. This sentence should be underlined, overlined, and stricken. <div class="seven"></div>  There should be no red at the end of this line.<img src="support/swatch-white.png" alt="FAIL: Images required." class="one">  The text of this sentence and all its spaces (including the space between the images) <img src="support/square-teal.png" alt="FAIL: Images required."> <img src="support/square-purple.png" alt="FAIL: Images required."> should be underlined, but the images themselves should not be underlined. This sentence should have a long blue underline including between the two arrows here→ FAIL FAIL FAIL FAIL ←and here. There should be a long blue underline between here→                      ←and here. </body> </html>

Dave Hyatt

Comment 16 2012-03-20 11:32:50 PDT

Modifying the test case seems fine to me.

SravanKumar S(:sravan)

Comment 17 2012-03-20 12:07:34 PDT

Thanks, as discussed on IRC will rename it to c543-txt-decor-000.htm, and submit the patch.

SravanKumar S(:sravan)

Comment 18 2012-03-20 20:48:16 PDT

Created attachment 132958 [details] Patch Added new test case with name c543-txt-decor-000.html, along with baselined outputs.

SravanKumar S(:sravan)

Comment 19 2012-03-20 21:19:32 PDT

Created attachment 132964 [details] patch after updating test_expectations

SravanKumar S(:sravan)

Comment 20 2012-03-20 21:43:09 PDT

Created attachment 132968 [details] patch after updating test_expectations

Simon Fraser (smfr)

Comment 21 2012-03-22 17:02:57 PDT

Comment on attachment 132968 [details] patch after updating test_expectations View in context: https://bugs.webkit.org/attachment.cgi?id=132968&action=review > LayoutTests/css2.1/c543-txt-decor-000.html:1 > +ï»¿<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> Do you have a BOM here or something?

SravanKumar S(:sravan)

Comment 22 2012-03-22 17:21:10 PDT

Created attachment 133393 [details] Patch after removing <feff> UTF-8 BOM

SravanKumar S(:sravan)

Comment 23 2012-03-22 19:58:37 PDT

Hi, Can some please review the patch?

SravanKumar S(:sravan)

Comment 24 2012-03-23 22:44:40 PDT

Created attachment 133620 [details] Patch Removed old test and corresponding expected files.

Pavel Feldman

Comment 25 2012-03-23 23:52:55 PDT

Comment on attachment 133620 [details] Patch Please provide the ChangeLog and file a separate bug for this patch.

Julien Chaffraix

Comment 26 2012-03-26 10:36:25 PDT

Sravan told me he filed a new bug per Pavel's comment (bug 82120) to cover super-seeding. However this was unneeded as this bug was also about that but without ChangeLog Pavel was confused by the change. Anyway, closing this bug as a duplicate of bug 82120 as the patch there landed in trunk already. *** This bug has been marked as a duplicate of bug 82120 ***

Note You need to log in before you can comment on or make changes to this bug.