Bug 105126 - Flexbox should ignore firstLetter pseudo element.
Summary: Flexbox should ignore firstLetter pseudo element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 62048
  Show dependency treegraph
 
Reported: 2012-12-16 00:49 PST by huangxueqing
Modified: 2013-02-25 18:08 PST (History)
6 users (show)

See Also:


Attachments
patch (7.14 KB, patch)
2012-12-16 01:28 PST, huangxueqing
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch (7.14 KB, patch)
2012-12-16 06:06 PST, huangxueqing
no flags Details | Formatted Diff | Diff
patch (8.81 KB, patch)
2012-12-16 07:33 PST, huangxueqing
no flags Details | Formatted Diff | Diff
patch (8.81 KB, patch)
2012-12-16 07:35 PST, huangxueqing
tony: review-
tony: commit-queue-
Details | Formatted Diff | Diff
patch (5.87 KB, patch)
2013-02-22 02:10 PST, huangxueqing
no flags Details | Formatted Diff | Diff
patch (6.64 KB, patch)
2013-02-25 00:28 PST, huangxueqing
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description huangxueqing 2012-12-16 00:49:25 PST
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.
Comment 1 huangxueqing 2012-12-16 01:28:47 PST
Created attachment 179643 [details]
patch
Comment 2 WebKit Review Bot 2012-12-16 04:14:53 PST
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
Comment 3 Build Bot 2012-12-16 05:20:26 PST
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
Comment 4 huangxueqing 2012-12-16 06:06:38 PST
Created attachment 179646 [details]
patch
Comment 5 huangxueqing 2012-12-16 07:33:40 PST
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.
Comment 6 huangxueqing 2012-12-16 07:35:24 PST
Created attachment 179648 [details]
patch
Comment 7 Tony Chang 2012-12-18 14:11:18 PST
Comment on attachment 179648 [details]
patch

See bug 104485 comment 14 for a clarification.
Comment 8 Tony Chang 2013-02-15 13:01:25 PST
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?
Comment 9 huangxueqing 2013-02-17 18:41:29 PST
(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.
Comment 10 Tony Chang 2013-02-21 11:39:11 PST
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?
Comment 11 huangxueqing 2013-02-22 02:10:42 PST
Created attachment 189723 [details]
patch
Comment 12 huangxueqing 2013-02-22 02:29:52 PST
(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.
Comment 13 Tony Chang 2013-02-22 14:43:04 PST
(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?
Comment 14 huangxueqing 2013-02-25 00:28:42 PST
Created attachment 190009 [details]
patch
Comment 15 huangxueqing 2013-02-25 00:42:41 PST
(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.
Comment 16 Tony Chang 2013-02-25 11:53:58 PST
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.
Comment 17 WebKit Review Bot 2013-02-25 18:08:06 PST
Comment on attachment 190009 [details]
patch

Clearing flags on attachment: 190009

Committed r143993: <http://trac.webkit.org/changeset/143993>
Comment 18 WebKit Review Bot 2013-02-25 18:08:12 PST
All reviewed patches have been landed.  Closing bug.