Bug 105126 – Flexbox should ignore firstLetter pseudo element.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[huangxueqing]

The flex item should not apply flex container's '::first-line' and '::first-letter' pseudo elements.
testcase:
<style> 
    div { display: -webkit-flex;  display: -moz-flex; display: flex; } 
    div::first-letter { line-height: 100px;} 
    p:first-child { -webkit-order: 1; -moz-order: 1; order: 1;}
</style>
<div>
    <p>The first item.</p>
    <p>The second item.</p>
</div>
The property 'line-height: 100px' should apply to <p>, opera 12.11, IE 10 and firefox 20 alpha were correct.

'::first-line' see bug #104485.

[huangxueqing]

Created attachment 179643 [details]
patch

[WebKit Review Bot]

Comment on attachment 179643 [details]
patch

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

New failing tests:
css3/flexbox/inline-flex-crash2.html
fast/forms/week/week-appearance-pseudo-elements.html
fast/forms/time/time-appearance-pseudo-elements.html
fast/forms/month/month-appearance-pseudo-elements.html
css3/flexbox/flexbox-ignore-firstLetter.html
css3/flexbox/inline-flex-crash.html
fast/forms/date/date-appearance-pseudo-elements.html

[Build Bot]

Comment on attachment 179643 [details]
patch

Attachment 179643 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15360425

New failing tests:
css3/flexbox/inline-flex-crash2.html
fast/frames/sandboxed-iframe-attribute-parsing.html
css3/flexbox/flexbox-ignore-firstLetter.html
css3/flexbox/inline-flex-crash.html

[huangxueqing]

Created attachment 179646 [details]
patch

[huangxueqing]

Created attachment 179647 [details]
patch

Week, month, Time and date input element's render seems have relationship with RenderFlexibleBox(maybe RenderDeprecatedFlexibleBox), if so, the result of test cases used ::first-letter were wrong, we should rebase them.

[huangxueqing]

Created attachment 179648 [details]
patch

[Tony Chang]

Comment on attachment 179648 [details]
patch

See bug 104485 comment 14 for a clarification.

[Tony Chang]

Comment on attachment 179648 [details]
patch

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

Can you double check the results of the form pseudo element tests?  It looks like the checked in results don't currently apply style to the form controls.

Also, I would probably merge your new tests into a single reftest.  No need to create lots of separate test files.

> Source/WebCore/rendering/RenderBlock.cpp:6466
> +    if (isFlexibleBox())
> +        return;

This looks funny. Do we create the first letter renderer?

[huangxueqing]

(In reply to comment #8)
> (From update of attachment 179648 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179648&action=review
> 
> Can you double check the results of the form pseudo element tests?  It looks like the checked in results don't currently apply style to the form controls.
> 
Form input controls actually layout with flexbox since browser css defined in  WebCore/css/html.css as:
input[type="date"|"datetime"|"datatime-local"|"month"|"time"|"week"] {
    ... ...
    display: -webkit-inline-flex;
    ... ...
}

> Also, I would probably merge your new tests into a single reftest.  No need to create lots of separate test files.
> 
OK

> > Source/WebCore/rendering/RenderBlock.cpp:6466
> > +    if (isFlexibleBox())
> > +        return;
> 
> This looks funny. Do we create the first letter renderer?
This code just prevent RenderBlock update its first text child. No, the first letter renderer would not be created. The first letter renderer as the first text child's new parent used to update RenderText's first letter info.

[Tony Chang]

Comment on attachment 179648 [details]
patch

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

>>> Source/WebCore/rendering/RenderBlock.cpp:6466
>>> +        return;
>> 
>> This looks funny. Do we create the first letter renderer?
> 
> This code just prevent RenderBlock update its first text child. No, the first letter renderer would not be created. The first letter renderer as the first text child's new parent used to update RenderText's first letter info.

I think this check should go in findFirstLetterBlock.  Why do we check both if both |this| is a flexible box and if its parent is a flexible box?

[huangxueqing]

Created attachment 189723 [details]
patch

[huangxueqing]

(In reply to comment #10)
> (From update of attachment 179648 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179648&action=review
> 
> >>> Source/WebCore/rendering/RenderBlock.cpp:6466
> >>> +        return;
> >> 
> >> This looks funny. Do we create the first letter renderer?
> > 
> > This code just prevent RenderBlock update its first text child. No, the first letter renderer would not be created. The first letter renderer as the first text child's new parent used to update RenderText's first letter info.
> 
> I think this check should go in findFirstLetterBlock.  Why do we check both if both |this| is a flexible box and if its parent is a flexible box?

I moved this check in |findFirstLetterBlock|. RenderObject maybe apply first-letter from own style or inhretied from parent. For example:
<style>
   div::first-letter {color: red}
</style>
<div>
    <P>
</div>
"div" apply first-letter from own style while "p" inherited from "div".

In addition, the case marked as failling in Chromium TestExpectations apply first-letter psuedo-element but form controls render as flexbox, see
WebCore/css/html.css:
input[type="date"|"datetime"|"datatime-local"|"month"|"time"|"week"] {
    ... ...
    display: -webkit-inline-flex;
    ... ...
}
The testcase fast/forms/week/week-appearance-pseudo-elements.html apply first-letter as:
<style type="text/css">
... ...
.first-letter:first-letter { color: green; font-size: 200%; }
</style>
<ul>
    ... ...
    <li><input type="week" value="1982-W25" class="first-letter"></li>
</ul>

This change make "color: green; font-size: 200%;" did not apply week control anymore.

[Tony Chang]

(In reply to comment #12)
> I moved this check in |findFirstLetterBlock|. RenderObject maybe apply first-letter from own style or inherited from parent. For example:
> <style>
>    div::first-letter {color: red}
> </style>
> <div>
>     <P>
> </div>
> "div" apply first-letter from own style while "p" inherited from "div".
> 

I think you're saying that it's the difference between ::first-letter being on #container or #flexbox in this example:
<div id=container>
<div id=flexbox style="display:-webkit-flex">
  <p>
</div>
</div>

If so, can you add some test cases that for ::first-letter on #container?

> In addition, the case marked as failling in Chromium TestExpectations apply first-letter psuedo-element but form controls render as flexbox, see
> This change make "color: green; font-size: 200%;" did not apply week control anymore.

I'm confused since I don't see green or large fonts in any of the checked in results:
http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-linux/fast/forms/week/week-appearance-pseudo-elements-expected.png
http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-mac/fast/forms/week/week-appearance-pseudo-elements-expected.png
http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-win/fast/forms/week/week-appearance-pseudo-elements-expected.png

Why doesn't first-letter show up in these results?  Are you seeing something different on your machine?

[huangxueqing]

Created attachment 190009 [details]
patch

[huangxueqing]

(In reply to comment #13)
> (In reply to comment #12)
> > I moved this check in |findFirstLetterBlock|. RenderObject maybe apply first-letter from own style or inherited from parent. For example:
> > <style>
> >    div::first-letter {color: red}
> > </style>
> > <div>
> >     <P>
> > </div>
> > "div" apply first-letter from own style while "p" inherited from "div".
> > 
> 
> I think you're saying that it's the difference between ::first-letter being on #container or #flexbox in this example:
> <div id=container>
> <div id=flexbox style="display:-webkit-flex">
>   <p>
> </div>
> </div>
> 
> If so, can you add some test cases that for ::first-letter on #container?
> 
Done

> > In addition, the case marked as failling in Chromium TestExpectations apply first-letter psuedo-element but form controls render as flexbox, see
> > This change make "color: green; font-size: 200%;" did not apply week control anymore.
> 
> I'm confused since I don't see green or large fonts in any of the checked in results:
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-linux/fast/forms/week/week-appearance-pseudo-elements-expected.png
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-mac/fast/forms/week/week-appearance-pseudo-elements-expected.png
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-win/fast/forms/week/week-appearance-pseudo-elements-expected.png
> 
> Why doesn't first-letter show up in these results?  Are you seeing something different on your machine?
Sorry, these results were rebaselined in r141367, the reason please see bug#108069.

[Tony Chang]

Comment on attachment 190009 [details]
patch

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

> LayoutTests/css3/flexbox/flexbox-ignore-container-firstLetter.html:3
> +<!DOCTYPE html>
> +<html>
> +<link href="resources/flexbox.css" rel="stylesheet">

I would have put this in the same test as flexbox-ignore-firstLetter.html, but this is OK.

[WebKit Review Bot]

Comment on attachment 190009 [details]
patch

Clearing flags on attachment: 190009

Committed r143993: <http://trac.webkit.org/changeset/143993>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.