Bug 34753

Created attachment 48413 [details] test case

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

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

(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.

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 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.

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 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

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

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 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.

(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.

(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 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.

(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...

hi Eric, Robert, If it is ok with you guys can i work on this bug ??

(In reply to comment #16) > hi Eric, Robert, > If it is ok with you guys can i work on this bug ?? Feel free!

(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 :)

Created attachment 138531 [details] Proposed Patch

The patch has 2 seperate test cases one for text-decoration, and one for text-transform.

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.

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. :)

(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.

Can I upload another patch without the use of the extra member variable ??

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.

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.

(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.

(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

I am still able to reproduce this bug in Safari 15.5 on macOS 12.4. All other browsers Chrome Canary 104 and Firefox Nightly 103 behave same (different from Safari) and show text lines same as per attached test case. Thanks!

<rdar://problem/95705559>