CSS 2.1 failure: first-letter-nested-004.htm fails

• Component: CSS
• Status: RESOLVED FIXED

[Robert Hogan]

Need to check each text child until a line-break or a text node that is not all whitespace is found.

[Robert Hogan]

Created attachment 116658 [details]
Patch

[Robert Hogan]

Hi Julien,

Can you take a look at this?

Thanks!

[Julien Chaffraix]

Comment on attachment 116658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116658&action=review

Robert, this is not an area of the code I know enough to review this patch. I just looked over the patch quickly.

> Source/WebCore/rendering/RenderBlock.cpp:-5746
> -

Unrelated change, it is also arguably better this way.

> Source/WebCore/rendering/RenderBlock.h:477
> +    RenderObject* firstLetterInChild(RenderObject* currChild, RenderObject* &firstLetterBlock);
> +    RenderObject* firstLetterInChildren(RenderObject* currChild, RenderObject* &firstLetterBlock);

I find the 2 functions' name confusing. Especially since the second calls the first.

Adding a verb would likely help. If those 2 functions had a really different name, it would also reduce the confusion between the 2.

[Robert Hogan]

Created attachment 134963 [details]
Patch

[WebKit Review Bot]

Comment on attachment 134963 [details]
Patch

Attachment 134963 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12253863

New failing tests:
fast/css/first-letter-capitalized-edit-select-crash.html

[WebKit Review Bot]

Created attachment 134969 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Robert Hogan]

Created attachment 134998 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 134998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134998&action=review

> Source/WebCore/rendering/RenderBlock.cpp:6046
> +RenderObject* RenderBlock::firstLetterInSibling(RenderObject* currChild, RenderObject* &firstLetterBlock)

I'm not sure "inSibling" is the right name here.  This method seems to have little to do with being a sibling to anything.  maybe InNode?  or underNode?  I also might call currChild "root" and have a separate currChild.

Also, I wonder if we could pull the addFirstLetter call outside of this function? Could it be done in the caller, right before returning currChild?

> Source/WebCore/rendering/RenderBlock.cpp:6055
> +            // If we call addFirstLetter(), currChild will be destroyed.

Always, or just could be?

> Source/WebCore/rendering/RenderBlock.cpp:6067
> +            if (currChild->style()->styleType() == FIRST_LETTER) {

I'm confused what this check is for.

> Source/WebCore/rendering/RenderBlock.cpp:6072
> +        } else if (currChild->isReplaced() || currChild->isRenderButton() || currChild->isMenuList())

Is button and menu-list not replaced?  Why do they need separate checks?

[Robert Hogan]

Created attachment 136967 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136967 [details]
Patch

Attachment 136967 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12390777

New failing tests:
media/video-loop.html
accessibility/aria-disabled.html
http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html
fast/canvas/webgl/shader-precision-format.html
media/video-pause-immediately.html
media/video-muted.html
fast/loader/text-document-wrapping.html

[WebKit Review Bot]

Created attachment 137027 [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: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Robert Hogan]

(In reply to comment #11)
> Created an attachment (id=137027) [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: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

These failures are unrelated to my patch - I don't see them locally.

[Robert Hogan]

(In reply to comment #8)
> (From update of attachment 134998 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134998&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:6046
> > +RenderObject* RenderBlock::firstLetterInSibling(RenderObject* currChild, RenderObject* &firstLetterBlock)
> 
> I'm not sure "inSibling" is the right name here.  This method seems to have little to do with being a sibling to anything.  maybe InNode?  or underNode?  I also might call currChild "root" and have a separate currChild.

I've improved the names I think.

> 
> Also, I wonder if we could pull the addFirstLetter call outside of this function? Could it be done in the caller, right before returning currChild?

Not really, I think. I've refactored the patch quite a bit so I'm not sure if this is still a concern.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:6055
> > +            // If we call addFirstLetter(), currChild will be destroyed.
> 
> Always, or just could be?

Refactoring has made this a non-issue.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:6067
> > +            if (currChild->style()->styleType() == FIRST_LETTER) {
> 
> I'm confused what this check is for.

Added a comment.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:6072
> > +        } else if (currChild->isReplaced() || currChild->isRenderButton() || currChild->isMenuList())
> 
> Is button and menu-list not replaced?  Why do they need separate checks?

This is existing code. As far as I can tell they will only be isReplaced() if they are inline, which I guess they will be most of the time, but not if display is set otherwise.

[Eric Seidel (no email)]

Comment on attachment 136967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136967&action=review

This looks good as far as I can tell.

> Source/WebCore/rendering/RenderBlock.cpp:5967
> +    if (child->isText()) {

Is this function hot?  Do we need to worry about ordering the cases such that it's most likely to return early?

> Source/WebCore/rendering/RenderBlock.cpp:5997
> +        // We found a lower-level node with first-letter, which supersedes the higher-level style

Should end with a period.

[Robert Hogan]

Committed r116438: <http://trac.webkit.org/changeset/116438>