Bug 105126

Created attachment 179643 [details] patch

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

Created attachment 179646 [details] patch

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.

Created attachment 179648 [details] patch

Comment on attachment 179648 [details] patch See bug 104485 comment 14 for a clarification.

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?

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

Created attachment 189723 [details] patch

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

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

Created attachment 190009 [details] patch

(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 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 on attachment 190009 [details] patch Clearing flags on attachment: 190009 Committed r143993: <http://trac.webkit.org/changeset/143993>

All reviewed patches have been landed. Closing bug.