|
WebKit Bugzilla |
| 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: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Joseph Pecoraro
2011-05-28 20:57:36 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. 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. 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! 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. 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
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. 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
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. 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. Hi, Pinging just to make sure that this thread does'nt die out. Would be of lot help, if Hyatt can close this :) . 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 :) . 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. 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. 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? 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"> </p> <p class="one"> The text of this sentence and all<span> </span>its<span> </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→<span class="eight"> FAIL FAIL FAIL FAIL </span>←and here. </p> <p>There should be a long blue underline between here→<span class="one"> </span> ←and here.</p> </body> </html> Modifying the test case seems fine to me. Thanks, as discussed on IRC will rename it to c543-txt-decor-000.htm, and submit the patch. Created attachment 132958 [details]
Patch
Added new test case with name c543-txt-decor-000.html, along with baselined outputs.
Created attachment 132964 [details]
patch after updating test_expectations
Created attachment 132968 [details]
patch after updating test_expectations
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? Created attachment 133393 [details]
Patch after removing <feff> UTF-8 BOM
Hi, Can some please review the patch? Created attachment 133620 [details]
Patch
Removed old test and corresponding expected files.
Comment on attachment 133620 [details]
Patch
Please provide the ChangeLog and file a separate bug for this patch.
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 *** |