Bug 99584

I think I need to implement 'text'justify' property before implementing this bug. CSS3 spec says : "If ‘auto’ is specified, content on the affected line is aligned per ‘text-align’ unless ‘text-align’ is set to ‘justify’. In this case, content is justified if ‘text-justify’ is ‘distribute’ and start-aligned otherwise. All other values have the same meanings as in ‘text-align’." (http://www.w3.org/TR/css3-text/#text-align-last) So, this bug depends on https://bugs.webkit.org/show_bug.cgi?id=100058.

Created attachment 200882 [details] Patch

Is this actually working properly? I thought it should work like this: <http://www.webreference.com/programming/corecss/Figure11-14_text-align-last.jpg> Also, please consider turning these tests into ref tests instead of adding more pixel tests to the WebKit repository.

(In reply to comment #3) > Is this actually working properly? I thought it should work like this: <http://www.webreference.com/programming/corecss/Figure11-14_text-align-last.jpg> > > Also, please consider turning these tests into ref tests instead of adding more pixel tests to the WebKit repository. It work properly, as you can see the *-expected.png images. And, as I understood, I need another way to set text-align-last to test this property with ref tests. I'm not sure there is another way to do so. If not, pixel text would be necessary for this property.

(In reply to comment #4) > (In reply to comment #3) > > Is this actually working properly? I thought it should work like this: <http://www.webreference.com/programming/corecss/Figure11-14_text-align-last.jpg> > > > > Also, please consider turning these tests into ref tests instead of adding more pixel tests to the WebKit repository. > > It work properly, as you can see the *-expected.png images. > > And, as I understood, I need another way to set text-align-last to test this property with ref tests. > I'm not sure there is another way to do so. > > If not, pixel text would be necessary for this property. For example, in LayoutTests/platform/efl/fast/css3-text/css3-text-align-last/text-align-last-center-expected.png you are testing text-align-last: center; and none of the text is centered. Is that the correct result? You don't need another way to set text-align-last, you just need something that will give you the same layout.

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Is this actually working properly? I thought it should work like this: <http://www.webreference.com/programming/corecss/Figure11-14_text-align-last.jpg> > > > > > > Also, please consider turning these tests into ref tests instead of adding more pixel tests to the WebKit repository. > > > > It work properly, as you can see the *-expected.png images. > > > > And, as I understood, I need another way to set text-align-last to test this property with ref tests. > > I'm not sure there is another way to do so. > > > > If not, pixel text would be necessary for this property. > > For example, in LayoutTests/platform/efl/fast/css3-text/css3-text-align-last/text-align-last-center-expected.png you are testing text-align-last: center; and none of the text is centered. Is that the correct result? Oh.. test cases are old one.. I should use '-webkit-' prefix, but no prefix in the test cases. I can see this property work correctly with the prefix. I will modify the test cases. > > You don't need another way to set text-align-last, you just need something that will give you the same layout. I'm trying now. There are some cases which I cannot make same layout without this property. I will try to make most of cases as ref tests, and make one (or two) pixel test case(s).

Created attachment 203995 [details] Patch

Created attachment 203996 [details] test result png files

I've created 6 ref test cases and 1 pixel test case. You can refer the result of the 6 ref test cases by upzip the result file I've uploaded.

(In reply to comment #9) > I've created 6 ref test cases and 1 pixel test case. > > You can refer the result of the 6 ref test cases by upzip the result file I've uploaded. Test results look better now. I should have clarified that I was just doing a drive-by review and pointing out visual errors in the generated results -- I'm not the person to give final review here. Perhaps Mitz or Hyatt could take a look?

(In reply to comment #10) > (In reply to comment #9) > > I've created 6 ref test cases and 1 pixel test case. > > > > You can refer the result of the 6 ref test cases by upzip the result file I've uploaded. > > Test results look better now. > > I should have clarified that I was just doing a drive-by review and pointing out visual errors in the generated results -- I'm not the person to give final review here. > > Perhaps Mitz or Hyatt could take a look? Hmm. Then, I will try to ask them to review this on IRC. Thanks, Kling!!

Created attachment 212191 [details] Independently rewritten a similar patch, with no pixel-based tests (by use of the Ahem font) Independently rewritten a similar patch, with no pixel-based tests (by use of the Ahem font). Up for review.

Attachment 212191 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/text/text-align-last-expected.html', u'LayoutTests/css3/text/text-align-last.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp']" exit_code: 1 Source/WebCore/rendering/RenderBlockLineLayout.cpp:499: Missing space before ( in switch( [whitespace/parens] [5] Source/WebCore/rendering/RenderBlockLineLayout.cpp:503: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/rendering/RenderBlockLineLayout.cpp:511: One line control clauses should not use braces. [whitespace/braces] [4] LayoutTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 212191 [details] Independently rewritten a similar patch, with no pixel-based tests (by use of the Ahem font) Attachment 212191 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1803326

Comment on attachment 212191 [details] Independently rewritten a similar patch, with no pixel-based tests (by use of the Ahem font) Attachment 212191 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/2004283

Comment on attachment 212191 [details] Independently rewritten a similar patch, with no pixel-based tests (by use of the Ahem font) View in context: https://bugs.webkit.org/attachment.cgi?id=212191&action=review > LayoutTests/ChangeLog:3 > + Need a short description (OOPS!). Please update the changelog here too. > LayoutTests/css3/text/text-align-last-expected.html:1 > +<style> Both of the test and the expected are missing the doctype and the <html> starting tag. > LayoutTests/css3/text/text-align-last-expected.html:39 > +<body> Are you sure all of your tests fit into a 800*600px area? > LayoutTests/css3/text/text-align-last.html:1 > +<style> Since CSS3_TEXT is disabled by default, the test will fail. You should put it to the skipped list, and open a bug for it. > Source/WebCore/ChangeLog:1 > +2013-09-20 François REMY <francois.remy.dev@outlook.com> Utf8? > Source/WebCore/ChangeLog:8 > + -> Implemented the "text-align-last" property You really don't need an arrow. > Source/WebCore/ChangeLog:18 > + UNDER THE CSS3_TEXT FLAG: You should mention that this code is guarded by that tag above. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:492 > + // however, the last line can be special How can it be special? This comment doesn't say any useful information. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:538 > +#else // IF NOT ENABLE(CSS3_TEXT) if !endsWithSoftBreak is true, you will never get here. You don't need to add comments after an #else. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:546 > + Please don't put extra lines without a reason.

Comment on attachment 212191 [details] Independently rewritten a similar patch, with no pixel-based tests (by use of the Ahem font) Attachment 212191 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/2004286

Comment on attachment 212191 [details] Independently rewritten a similar patch, with no pixel-based tests (by use of the Ahem font) Attachment 212191 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1956399

Comment on attachment 212191 [details] Independently rewritten a similar patch, with no pixel-based tests (by use of the Ahem font) Attachment 212191 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1934018

Created attachment 212229 [details] Patch I think I addressed previous comments with this patch. I modified the patch to use two columns to be sure to fit the 800x600 area.

Created attachment 221413 [details] Patch There were no progress on this bug for a while, so I'm taking over. Patch is up for review, updated to the latest spec.

Comment on attachment 221413 [details] Patch r=me

Comment on attachment 221413 [details] Patch Clearing flags on attachment: 221413 Committed r162213: <http://trac.webkit.org/changeset/162213>

All reviewed patches have been landed. Closing bug.