Bug 34753 - CSS 2.1: first-line-selector-008.htm test fails
: CSS 2.1: first-line-selector-008.htm test fails
Status: NEW
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 3409 6047
: 47141
  Show dependency treegraph
 
Reported: 2010-02-09 07:33 PST by
Modified: 2012-05-23 11:02 PST (History)


Attachments
test case (44.33 KB, application/x-zip-compressed)
2010-02-09 07:34 PST, sangeetha.sugavanam@nokia.com
no flags Details
Proposed patch (50.55 KB, patch)
2010-12-14 12:20 PST, Julie Jeongeun Kim
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch(2nd) (62.67 KB, patch)
2010-12-15 00:33 PST, Julie Jeongeun Kim
no flags Review Patch | Details | Formatted Diff | Diff
path based on the latest source directory (67.03 KB, patch)
2011-05-15 23:47 PST, Julie Jeongeun Kim
eric: review-
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.19 MB, application/zip)
2011-05-16 00:54 PST, WebKit Review Bot
no flags Details
Proposed Patch (106.53 KB, patch)
2012-04-24 03:55 PST, Pravin D
eric: review-
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 2010-02-09 07:33:55 PST
Use case:
CSS 2.1

Steps to reproduce:
1) Launch QtLauncher.
2) Unzip contents of the test case.
3) Open the html page using QtLauncher.
4) Click on the link LINK.
5) Check the page. 

Actual outcome:
Two lines show different.

Expected outcome:
Two lines show the same.

This bug is also reproducible on Safari 4.0.4.
------- Comment #1 From 2010-02-09 07:34:38 PST -------
Created an attachment (id=48413) [details]
test case
------- Comment #2 From 2010-03-05 10:13:29 PST -------
Please, select the right component when reporting bugs. This is not specific to QtWebkit, it is a general problem of WebKit.

Also, why not give directly the link to the test case? A zip containing a html file with only a link to the test case is just a waste of time for everybody.

Here is the link:
http://www.w3.org/Style/CSS/Test/CSS2.1/20100127/html4/first-line-selector-008.htm
------- Comment #3 From 2010-12-14 12:20:17 PST -------
Created an attachment (id=76555) [details]
Proposed patch

I uploaded proposed patch.
Please take a look.

I generated png result from pixel test because it's related to drawing on screen.
I thought the result doesn't depend on platform because there are only texts on test page and I moved the result files, png, checksum and txt, to common directory.
I have just snow-leopard environment.
Please let me know if I have to move it to platform directory and add result files on other platforms.

Thanks
------- Comment #4 From 2010-12-14 14:49:11 PST -------
(In reply to comment #3)
> I generated png result from pixel test because it's related to drawing on screen.
> I thought the result doesn't depend on platform because there are only texts on test page and I moved the result files, png, checksum and txt, to common directory.
> I have just snow-leopard environment.
> Please let me know if I have to move it to platform directory and add result files on other platforms.

Text rendering is precisely what make most pixel test different from one platform to the next. Each platform, and sometime different port on the same platform, render text differently.
------- Comment #5 From 2010-12-15 00:33:38 PST -------
Created an attachment (id=76636) [details]
Proposed patch(2nd)

I modified some code and moved test results to 'platform/mac' directory.
There was a problem related to selection with the previous patch.
So, I fixed it.
I uploaded proposed patch again.
Please look into it.

Thanks.
------- Comment #6 From 2011-01-26 01:33:44 PST -------
(From update of attachment 76555 [details])
You can mark the old patch obsolete when you post a new one. This helps reviewers to find patches to review.
------- Comment #7 From 2011-05-15 23:47:23 PST -------
Created an attachment (id=93615) [details]
path based on the latest source directory

I've uploaded patch based on the latest source directory.
Review, please.

Thanks.
------- Comment #8 From 2011-05-16 00:54:06 PST -------
(From update of attachment 93615 [details])
Attachment 93615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8701669

New failing tests:
fast/css/first-line-text-transform.html
------- Comment #9 From 2011-05-16 00:54:11 PST -------
Created an attachment (id=93618) [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #10 From 2011-05-16 01:06:56 PST -------
Could anyone give me information?

"New failing tests:
fast/css/first-line-text-transform.html"

That test case is a new one added by me for this patch.
I added only expected result based on mac.
Do I need to add every expected result to each platform directory?
------- Comment #11 From 2011-08-05 17:24:17 PST -------
(From update of attachment 93615 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=93615&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:189
> +    const UChar* characters = transformed.characters() + m_start;

It looks like you should create a static helper function that does this rather than repeat it throughout the code.
------- Comment #12 From 2011-08-05 17:25:07 PST -------
(In reply to comment #10)
> 
> That test case is a new one added by me for this patch.
> I added only expected result based on mac.
> Do I need to add every expected result to each platform directory?

You should add the relevant css2.1 test under LayoutTests/css2.1/20110323/ I think. See bug 47141 for more information.
------- Comment #13 From 2011-08-05 17:26:53 PST -------
(In reply to comment #11)
> (From update of attachment 93615 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93615&action=review
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:189
> > +    const UChar* characters = transformed.characters() + m_start;
> 
> It looks like you should create a static helper function that does this rather than repeat it throughout the code.

Whoops, I meant to comment the quoted line and the line above it, i.e.:

 const String transformed = textRenderer()->getTransformedText(m_firstLine);
 const UChar* characters = transformed.characters() + m_start;
------- Comment #14 From 2012-01-30 15:09:27 PST -------
(From update of attachment 93615 [details])
This review requist is over 6-months old.  This patch also feels wrong.  Seems like a lot of code for very little gain.  Please post an updated patch if there is still interest in this bug.
------- Comment #15 From 2012-04-16 09:40:31 PST -------
(In reply to comment #14)
> (From update of attachment 93615 [details] [details])
> This review requist is over 6-months old.  This patch also feels wrong.  Seems like a lot of code for very little gain.  Please post an updated patch if there is still interest in this bug.

I was looking into the bug. Had a doubt. The issue seems to that the text-decoration and text-transform does not seem to be applied when used with the first-line selector. Can this bug be split into 2, one for text-decoration with first-line selector and another for text-transform with first-line selector?? 
Fix for text-decoration is going to be totally different from text-transform...
------- Comment #16 From 2012-04-19 03:18:51 PST -------
hi Eric, Robert,
If it is ok with you guys can i work on this bug ??
------- Comment #17 From 2012-04-19 03:46:48 PST -------
(In reply to comment #16)
> hi Eric, Robert,
> If it is ok with you guys can i work on this bug ??

Feel free!
------- Comment #18 From 2012-04-19 09:23:55 PST -------
(In reply to comment #17)
> (In reply to comment #16)
> > hi Eric, Robert,
> > If it is ok with you guys can i work on this bug ??
> 
> Feel free!

Thank you :)
------- Comment #19 From 2012-04-24 03:55:59 PST -------
Created an attachment (id=138531) [details]
Patch
------- Comment #20 From 2012-04-24 04:01:49 PST -------
The patch has 2 seperate test cases one for text-decoration, and one for text-transform.
------- Comment #21 From 2012-04-24 22:47:26 PST -------
(From update of attachment 138531 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=138531&action=review

> Source/WebCore/rendering/RenderText.cpp:59
> +    String previousText;

Making every RenderText bigger is bad-news bears.  There must be a better way to do this.
------- Comment #22 From 2012-04-24 22:53:56 PST -------
I'm happy to help you find a better way, if you can explain to me what you're trying to do.

First-letter works by splitting the RenderText into two RenderTextFragments.
first-line works by giving the first linebox a special style (or rather making style-access on the linebox be sensitive to the possibility of this special style).

It's unclear to me what functionality you're tryign to add to first-line here.

If you're just looking for an easy-patch to get started in WebKIt, this is anything but.  Workign on the linebox tree is hard. :)  If you want to work in the linebox tree or rendering, we can discuss lots of possible clean-up patches to do first.  Generally I find I learn code best by doing various cleanup patches in the area before trying to do systemic changes like this. :)
------- Comment #23 From 2012-04-25 02:42:46 PST -------
(In reply to comment #22)
> I'm happy to help you find a better way, if you can explain to me what you're trying to do.

> First-letter works by splitting the RenderText into two RenderTextFragments.
> first-line works by giving the first linebox a special style (or rather making style-access on the linebox be sensitive to the possibility of this special style).
> 
> It's unclear to me what functionality you're tryign to add to first-line here.
> 


Let explain the issue and how I'm trying to fix it.
This bug can be split into 2, one is text-decoration used in First-line selector and another text-transform used in First-line selector(both of which are not working).

Text-decoration are applied at the paint phase. The changes for the same are present in 
WebCore::InlineTextBox::paintDecoration
WebCore::decorationColor
and WebCore::RenderObject::getTextDecorationColors

In the above functions the style used to select the decoration color when first-line style is present was wrong. The fix is to basically make the functions us proper style i.e use the first-line style if present to retrieve the proper decoration colors and us it to paint the text-decoration.

Text-Transform are to be applied either before layout phase or during layouting. I have tried to apply the transforms during layout-ing phase.
The changes for the same are present in 
WebCore::RenderBlock::layoutRunsAndFloatsInRange
WebCore::RenderBlock::LineBreaker::nextLineBreak)
and in RenderText class.

layoutRunsAndFloatsInRange() is the function where the line boxes are created. It uses nextLineBreak() and bidiRun to do the same. 
nextLineBreak() function is used to find the line break location and the last Renderer that is possible to be present in a given line.

Let me explain nextLineBreak() in case of RenderText in a little detail. 
When it encounters a RenderText object, it basically reads all of the contents, retrieves its style(this can be first-line style or normal style), finds possible line break position/character , uses the associated font class to calculate the width of text(with proper metrics). Based on these information it calculates the line break location. 

What the fix does...
When the first line is created the following takes place:

When nextLineBreak() is called for the 1st line, we apply any first-line transforms on the entire text content of RenderText, as we are not yet sure how long the first line is going to be and how much of text can be fit into it. Now m_text of RenderText contains text with first-line transforms. 
nextLineBreak() calculates the line break position and also has information of how of the text content can be present.
This information is given back to layoutRunsAndFloatsInRange(). It creates the 1st linebox and re-applies the first-line style on the text content based on the break position. This is to revert the first-line transforms on the rest of the text content.

In short in nextLineBreak() the first-line transform is applied to the whole text content, assuming that the all the text will be on the 1st line. This is necessary for the font widths to be calculated properly.
Then in layoutRunsAndFloatsInRange() after the 1st line is created, we know the 1st line content, so we apply the 1st line transform only to the 1st line content.

Also maybe I can get rid of the extra string that I had added to renderText class. It was used to keep a copy of m_text so as reduce re-tranformations.


> If you're just looking for an easy-patch to get started in WebKIt, this is anything but.  Workign on the linebox tree is hard. :)  If you want to work in the linebox tree or rendering, we can discuss lots of possible clean-up patches to do first.  Generally I find I learn code best by doing various cleanup patches in the area before trying to do systemic changes like this. :)

Thanks for offering to help. I'd be more than happy to work on such bugs.
------- Comment #24 From 2012-04-26 02:10:12 PST -------
Can I upload another patch without the use of the extra member variable ??
------- Comment #25 From 2012-04-26 11:09:39 PST -------
(From update of attachment 138531 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=138531&action=review

> Source/WebCore/ChangeLog:3
> +        CSS 2.1: first-line-selector-008.htm test fails

You should add this test to css2.1/20110323

> Source/WebCore/ChangeLog:9
> +        Tests: css2.1/20110323/first-line-text-decoration.html
> +               css2.1/20110323/first-line-text-transform.html

If these are your own tests, you should put them in fast/css. Same applies if they are modified versions of the test from the test suite. css2.1/20110323 is for unmodified copies of css suite tests.

> Source/WebCore/ChangeLog:11
> +        The below changes are done for text-decoration to work with first-line style.

Definitely need a bit more on what the patch is trying to do, the problem its trying to solve. Also why first-line-selector-008 doesn't work on WebKit at the moment.
------- Comment #26 From 2012-04-26 11:50:24 PST -------
It looks like this is two separate issues rolled into a single bug. Text decorations not working and text-transform not working. These should be handled with two different dependent patches/bugs IMO, even if the same CSS 2.1 test suite case shows them both.
------- Comment #27 From 2012-04-26 12:55:51 PST -------
(In reply to comment #25)
> (From update of attachment 138531 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138531&action=review

> Definitely need a bit more on what the patch is trying to do, the problem its trying to solve. Also why first-line-selector-008 doesn't work on WebKit at the moment.

The issue is that the text-transforms and text-decorations used in first-line selectors are not applied (simply ignored). I have created 2 bugs for same. One for text-decoration and one for text-transform.
------- Comment #28 From 2012-04-26 12:56:21 PST -------
(In reply to comment #26)
> It looks like this is two separate issues rolled into a single bug. Text decorations not working and text-transform not working. These should be handled with two different dependent patches/bugs IMO, even if the same CSS 2.1 test suite case shows them both.

Done