Bug 99584 – [CSS3] Add rendering support for -webkit-text-align-last

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dongwoo Joshua Im (dwim)]

I will implement the rendering part of text-align-last,
and proper test cases.

[Dongwoo Joshua Im (dwim)]

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.

[Dongwoo Joshua Im (dwim)]

Created attachment 200882 [details]
Patch

[Andreas Kling]

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.

[Dongwoo Joshua Im (dwim)]

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

[Andreas Kling]

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

[Dongwoo Joshua Im (dwim)]

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

[Dongwoo Joshua Im (dwim)]

Created attachment 203995 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 203996 [details]
test result png files

[Dongwoo Joshua Im (dwim)]

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.

[Andreas Kling]

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

[Dongwoo Joshua Im (dwim)]

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

[François REMY]

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.

[WebKit Commit Bot]

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.

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Zoltan Horvath]

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
> [phone]  François REMY  <[e-mail]>

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[François REMY]

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.

[Zoltan Horvath]

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.

[Dave Hyatt]

Comment on attachment 221413 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 221413 [details]
Patch

Clearing flags on attachment: 221413

Committed r162213: <http://trac.webkit.org/changeset/162213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.