Bug 99584 - [CSS3] Add rendering support for -webkit-text-align-last
Summary: [CSS3] Add rendering support for -webkit-text-align-last
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
Depends on: 100058
Blocks: 76173
  Show dependency treegraph
 
Reported: 2012-10-17 04:38 PDT by Dongwoo Joshua Im (dwim)
Modified: 2014-01-17 12:28 PST (History)
18 users (show)

See Also:


Attachments
Patch (267.39 KB, patch)
2013-05-07 01:00 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
Patch (65.05 KB, patch)
2013-06-06 21:52 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
test result png files (110.05 KB, application/gzip)
2013-06-06 22:03 PDT, Dongwoo Joshua Im (dwim)
no flags Details
Independently rewritten a similar patch, with no pixel-based tests (by use of the Ahem font) (9.46 KB, patch)
2013-09-20 10:22 PDT, François REMY
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (4.20 KB, patch)
2013-09-20 15:30 PDT, François REMY
no flags Details | Formatted Diff | Diff
Patch (9.76 KB, patch)
2014-01-16 13:58 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongwoo Joshua Im (dwim) 2012-10-17 04:38:41 PDT
I will implement the rendering part of text-align-last,
and proper test cases.
Comment 1 Dongwoo Joshua Im (dwim) 2012-11-13 02:22:06 PST
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.
Comment 2 Dongwoo Joshua Im (dwim) 2013-05-07 01:00:56 PDT
Created attachment 200882 [details]
Patch
Comment 3 Andreas Kling 2013-05-07 17:15:54 PDT
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.
Comment 4 Dongwoo Joshua Im (dwim) 2013-05-07 18:26:39 PDT
(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.
Comment 5 Andreas Kling 2013-05-08 05:08:37 PDT
(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.
Comment 6 Dongwoo Joshua Im (dwim) 2013-05-29 03:44:31 PDT
(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).
Comment 7 Dongwoo Joshua Im (dwim) 2013-06-06 21:52:38 PDT
Created attachment 203995 [details]
Patch
Comment 8 Dongwoo Joshua Im (dwim) 2013-06-06 22:03:39 PDT
Created attachment 203996 [details]
test result png files
Comment 9 Dongwoo Joshua Im (dwim) 2013-06-06 22:07:18 PDT
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.
Comment 10 Andreas Kling 2013-06-19 03:32:34 PDT
(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?
Comment 11 Dongwoo Joshua Im (dwim) 2013-06-24 00:42:31 PDT
(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!!
Comment 12 François REMY 2013-09-20 10:22:58 PDT
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.
Comment 13 WebKit Commit Bot 2013-09-20 10:25:29 PDT
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 14 Early Warning System Bot 2013-09-20 10:28:34 PDT
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 15 Early Warning System Bot 2013-09-20 10:31:10 PDT
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 16 Zoltan Horvath 2013-09-20 10:36:27 PDT
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 17 Build Bot 2013-09-20 10:52:04 PDT
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 18 Build Bot 2013-09-20 11:05:09 PDT
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 19 Build Bot 2013-09-20 14:04:38 PDT
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
Comment 20 François REMY 2013-09-20 15:30:58 PDT
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.
Comment 21 Zoltan Horvath 2014-01-16 13:58:33 PST
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 22 Dave Hyatt 2014-01-17 09:54:24 PST
Comment on attachment 221413 [details]
Patch

r=me
Comment 23 WebKit Commit Bot 2014-01-17 12:28:50 PST
Comment on attachment 221413 [details]
Patch

Clearing flags on attachment: 221413

Committed r162213: <http://trac.webkit.org/changeset/162213>
Comment 24 WebKit Commit Bot 2014-01-17 12:28:55 PST
All reviewed patches have been landed.  Closing bug.