Bug 61699 - Multiple failures in css2.1/t1504-c543-txt-decor-00-d-g.html
: Multiple failures in css2.1/t1504-c543-txt-decor-00-d-g.html
Status: RESOLVED DUPLICATE of bug 82120
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-05-28 20:57 PST by
Modified: 2012-03-26 10:36 PST (History)


Attachments
Test case to reproduce the issue. (1.95 KB, text/html)
2011-07-01 04:57 PST, SravanKumar
no flags Details
Patch (84.44 KB, patch)
2012-03-20 20:48 PST, SravanKumar S(:sravan)
no flags Review Patch | Details | Formatted Diff | Diff
patch after updating test_expectations (84.31 KB, patch)
2012-03-20 21:19 PST, SravanKumar S(:sravan)
no flags Review Patch | Details | Formatted Diff | Diff
patch after updating test_expectations (84.40 KB, patch)
2012-03-20 21:43 PST, SravanKumar S(:sravan)
no flags Review Patch | Details | Formatted Diff | Diff
Patch after removing <feff> UTF-8 BOM (84.40 KB, patch)
2012-03-22 17:21 PST, SravanKumar S(:sravan)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (108.12 KB, patch)
2012-03-23 22:44 PST, SravanKumar S(:sravan)
pfeldman: review-
ssandela: commit‑queue?
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2011-05-28 20:57:36 PST
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
------- Comment #1 From 2011-05-28 20:59:20 PST -------
(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.
------- Comment #2 From 2011-06-24 05:48:06 PST -------
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.
------- Comment #3 From 2011-06-24 10:46:57 PST -------
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!
------- Comment #4 From 2011-06-25 07:31:04 PST -------
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.
------- Comment #5 From 2011-06-29 06:40:40 PST -------
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
------- Comment #6 From 2011-07-01 04:48:48 PST -------
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 <strong> 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.
------- Comment #7 From 2011-07-01 04:57:20 PST -------
Created an attachment (id=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
------- Comment #8 From 2011-07-01 08:28:14 PST -------
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.
------- Comment #9 From 2011-07-08 09:51:05 PST -------
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.
------- Comment #10 From 2011-08-11 17:21:19 PST -------
Hi,

Pinging just to make sure that this thread does'nt die out.
Would be of lot help, if Hyatt can close this :) .
------- Comment #11 From 2011-09-21 20:52:14 PST -------
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 :) .
------- Comment #12 From 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.
------- Comment #13 From 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.
------- Comment #14 From 2012-03-20 04:31:04 PST -------
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?
------- Comment #15 From 2012-03-20 04:48:19 PST -------
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>
  <p class="one"> This sentence should be underlined. </p>
  <p class="two"> This sentence should be overlined. </p>
  <p class="three"> This sentence should be stricken out. </p>

  <p class="four"> This sentence should be blinking (if the UA supports that). </p>
  <p class="one"> The sentence should be underlined. <b class="five">This sentence should be underlined</b>. </p>
  <p class="six"> This sentence should be underlined and overlined. </p>
  <p class="seven"> This sentence should be underlined, overlined, and stricken. </p>

  <div class="seven"></div> <!-- there should be no red on this page -->
  <p> There should be no red at the end of this line.<img src="support/swatch-white.png" alt="FAIL: Images required." class="one">&nbsp;</p>
  <p class="one">
   The text of this sentence and all<span> </span>its<span>&nbsp;</span>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 <em>not</em> be underlined.
  </p>
  <p class="one">
   This sentence should have a long blue underline including between the two
   arrows here&rarr;<span class="eight"> FAIL FAIL FAIL FAIL </span>&larr;and here.
  </p>
  <p>There should be a long blue underline between here&rarr;<span class="one"> &nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp;&nbsp; </span> &larr;and here.</p>

</body>
</html>
------- Comment #16 From 2012-03-20 11:32:50 PST -------
Modifying the test case seems fine to me.
------- Comment #17 From 2012-03-20 12:07:34 PST -------
Thanks, as discussed on IRC will rename it to c543-txt-decor-000.htm, and submit the patch.
------- Comment #18 From 2012-03-20 20:48:16 PST -------
Created an attachment (id=132958) [details]
Patch

Added new test case with name c543-txt-decor-000.html, along with baselined outputs.
------- Comment #19 From 2012-03-20 21:19:32 PST -------
Created an attachment (id=132964) [details]
patch after updating test_expectations
------- Comment #20 From 2012-03-20 21:43:09 PST -------
Created an attachment (id=132968) [details]
patch after updating test_expectations
------- Comment #21 From 2012-03-22 17:02:57 PST -------
(From update of attachment 132968 [details])
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?
------- Comment #22 From 2012-03-22 17:21:10 PST -------
Created an attachment (id=133393) [details]
Patch after removing <feff> UTF-8 BOM
------- Comment #23 From 2012-03-22 19:58:37 PST -------
Hi, Can some please review the patch?
------- Comment #24 From 2012-03-23 22:44:40 PST -------
Created an attachment (id=133620) [details]
Patch 

Removed old test and corresponding expected files.
------- Comment #25 From 2012-03-23 23:52:55 PST -------
(From update of attachment 133620 [details])
Please provide the ChangeLog and file a separate bug for this patch.
------- Comment #26 From 2012-03-26 10:36:25 PST -------
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 ***