Bug 34753 - CSS 2.1: first-line-selector-008.htm test fails
: CSS 2.1: first-line-selector-008.htm test fails
Status: NEW
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on: 3409 6047
Blocks: 47141
  Show dependency treegraph
 
Reported: 2010-02-09 07:33 PST by sangeetha.sugavanam
Modified: 2012-05-23 11:02 PDT (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description sangeetha.sugavanam 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 sangeetha.sugavanam 2010-02-09 07:34:38 PST
Created attachment 48413 [details]
test case
Comment 2 Benjamin Poulain 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 Julie Jeongeun Kim 2010-12-14 12:20:17 PST
Created attachment 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 Benjamin Poulain 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 Julie Jeongeun Kim 2010-12-15 00:33:38 PST
Created attachment 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 Hajime Morrita 2011-01-26 01:33:44 PST
Comment on attachment 76555 [details]
Proposed patch

You can mark the old patch obsolete when you post a new one. This helps reviewers to find patches to review.
Comment 7 Julie Jeongeun Kim 2011-05-15 23:47:23 PDT
Created attachment 93615 [details]
path based on the latest source directory

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

Thanks.
Comment 8 WebKit Review Bot 2011-05-16 00:54:06 PDT
Comment on attachment 93615 [details]
path based on the latest source directory

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 WebKit Review Bot 2011-05-16 00:54:11 PDT
Created attachment 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 Julie Jeongeun Kim 2011-05-16 01:06:56 PDT
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 Robert Hogan 2011-08-05 17:24:17 PDT
Comment on attachment 93615 [details]
path based on the latest source directory

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 Robert Hogan 2011-08-05 17:25:07 PDT
(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 Robert Hogan 2011-08-05 17:26:53 PDT
(In reply to comment #11)
> (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.

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 Eric Seidel 2012-01-30 15:09:27 PST
Comment on attachment 93615 [details]
path based on the latest source directory

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 Pravin D 2012-04-16 09:40:31 PDT
(In reply to comment #14)
> (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.

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 Pravin D 2012-04-19 03:18:51 PDT
hi Eric, Robert,
If it is ok with you guys can i work on this bug ??
Comment 17 Robert Hogan 2012-04-19 03:46:48 PDT
(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 Pravin D 2012-04-19 09:23:55 PDT
(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 Pravin D 2012-04-24 03:55:59 PDT
Created attachment 138531 [details]
Proposed Patch
Comment 20 Pravin D 2012-04-24 04:01:49 PDT
The patch has 2 seperate test cases one for text-decoration, and one for text-transform.
Comment 21 Eric Seidel 2012-04-24 22:47:26 PDT
Comment on attachment 138531 [details]
Proposed Patch

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 Eric Seidel 2012-04-24 22:53:56 PDT
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 Pravin D 2012-04-25 02:42:46 PDT
(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 Pravin D 2012-04-26 02:10:12 PDT
Can I upload another patch without the use of the extra member variable ??
Comment 25 Robert Hogan 2012-04-26 11:09:39 PDT
Comment on attachment 138531 [details]
Proposed Patch

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 Dave Hyatt 2012-04-26 11:50:24 PDT
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 Pravin D 2012-04-26 12:55:51 PDT
(In reply to comment #25)
> (From update of attachment 138531 [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 Pravin D 2012-04-26 12:56:21 PDT
(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