Bug 76173 – [meta] Implement CSS3 text-align-last feature on webkit

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

[Ebrahim Byagowi]

While IE and Firefox implemented text-align-last, I hope to see this on Webkit also.
For more information:
1. http://www.w3.org/TR/css3-text/#text-align-last
2. https://bugzilla.mozilla.org/show_bug.cgi?id=536557

[Ebrahim Byagowi]

Please see me, I am a CSS3 feature :D

[Dongwoo Joshua Im (dwim)]

I will look at this issue. ;-)

[Dongwoo Joshua Im (dwim)]

Created attachment 167707 [details]
WIP patch

This is the WIP patch for the text-align-last property.
I'm trying to prepare some valuable test cases for this property now.

If anyone available, 
please review this patch and give some comments to me.
It will be very helpful to me.

Thanks.

[WebKit Review Bot]

Attachment 167707 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSParser.cpp:1767:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/rendering/style/RenderStyle.h:260:  _text_align_last is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 167707 [details]
WIP patch

Attachment 167707 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14214647

[Early Warning System Bot]

Comment on attachment 167707 [details]
WIP patch

Attachment 167707 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14215670

[Dongwoo Joshua Im (dwim)]

Created attachment 167711 [details]
WIP patch

[Dongwoo Joshua Im (dwim)]

(In reply to comment #4)
> Source/WebCore/rendering/style/RenderStyle.h:260:  _text_align_last is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

There are many identifiers which use underscore in the RenderStyle.h file.

Is it ok to file a new bug to fix those by myself?

[WebKit Review Bot]

Comment on attachment 167711 [details]
WIP patch

Attachment 167711 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14218621

[Build Bot]

Comment on attachment 167711 [details]
WIP patch

Attachment 167711 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14238211

[Early Warning System Bot]

Comment on attachment 167711 [details]
WIP patch

Attachment 167711 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14215676

[Peter Beverloo (cr-android ews)]

Comment on attachment 167711 [details]
WIP patch

Attachment 167711 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14223605

[Early Warning System Bot]

Comment on attachment 167711 [details]
WIP patch

Attachment 167711 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14223608

[Dongwoo Joshua Im (dwim)]

Created attachment 167738 [details]
WIP patch

WIP patch

This is the WIP patch for the text-align-last property.
I'm trying to prepare some valuable test cases for this property now.

If anyone available, 
please review this patch and give some comments to me.
It will be very helpful to me.

Thanks.

[WebKit Review Bot]

Comment on attachment 167738 [details]
WIP patch

Attachment 167738 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14209872

[Build Bot]

Comment on attachment 167738 [details]
WIP patch

Attachment 167738 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14217719

[Peter Beverloo (cr-android ews)]

Comment on attachment 167738 [details]
WIP patch

Attachment 167738 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14207889

[Alexis Menard (darktears)]

Comment on attachment 167738 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167738&action=review

> Source/WebCore/css/CSSParser.cpp:1771
> +        break;

You want to put this case in isValidKeywordPropertyAndValue as it will enjoy fast path from JS in some cases.

[Dongwoo Joshua Im (dwim)]

Created attachment 168175 [details]
WIP patch

[WebKit Review Bot]

Comment on attachment 168175 [details]
WIP patch

Attachment 168175 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14245861

[Build Bot]

Comment on attachment 168175 [details]
WIP patch

Attachment 168175 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14257460

[Peter Beverloo (cr-android ews)]

Comment on attachment 168175 [details]
WIP patch

Attachment 168175 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14255512

[Dongwoo Joshua Im (dwim)]

Created attachment 168363 [details]
patch without test case

I've tried to build this on mac, it have been succeeded.
I believe this patch will pass the ews this time.

[Dongwoo Joshua Im (dwim)]

Created attachment 168364 [details]
test case

I made a test case for this property.
Please see this is enough.

[Dongwoo Joshua Im (dwim)]

Do I need to make the expected results for all ports?
Or, can I add this test case as a skipped case for all port except EFL port which is the major port for me?

[Ebrahim Byagowi]

These are my proposes! :)
Could you borrow some tests from the Mozilla bug (https://bugzilla.mozilla.org/show_bug.cgi?id=536557 https://bug536557.bugzilla.mozilla.org/attachment.cgi?id=533614), Mozilla tests are under Public Domain (http://wiki.whatwg.org/wiki/Testsuite) so it would not be a problem if you want completely copy them! Their tests contain RTL text (hebrew, arabic, persian, ...) testing that is really important.
Also please check your patch with http://ie.microsoft.com/testdrive/Performance/TextJustificationAnimated/Default.html the first one is related to this bug and second one to https://bugs.webkit.org/show_bug.cgi?id=20606 that is of-course a separate bug :)

[Dongwoo Joshua Im (dwim)]

(In reply to comment #26)
> These are my proposes! :)
> Could you borrow some tests from the Mozilla bug (https://bugzilla.mozilla.org/show_bug.cgi?id=536557 https://bug536557.bugzilla.mozilla.org/attachment.cgi?id=533614), Mozilla tests are under Public Domain (http://wiki.whatwg.org/wiki/Testsuite) so it would not be a problem if you want completely copy them! Their tests contain RTL text (hebrew, arabic, persian, ...) testing that is really important.
> Also please check your patch with http://ie.microsoft.com/testdrive/Performance/TextJustificationAnimated/Default.html the first one is related to this bug and second one to https://bugs.webkit.org/show_bug.cgi?id=20606 that is of-course a separate bug :)

Oh. You've worked for this property on Mozilla! That's nice! :)

1. http://ie.microsoft.com/testdrive/Performance/TextJustificationAnimated/Default.html is working fine.

2. Test cases in https://bug536557.bugzilla.mozilla.org/attachment.cgi?id=533614 are quite a lot.
Do you really think we need to applying all of those in this patch?

[Dongwoo Joshua Im (dwim)]

(In reply to comment #27)
> (In reply to comment #26)
> > These are my proposes! :)
> > Could you borrow some tests from the Mozilla bug (https://bugzilla.mozilla.org/show_bug.cgi?id=536557 https://bug536557.bugzilla.mozilla.org/attachment.cgi?id=533614), Mozilla tests are under Public Domain (http://wiki.whatwg.org/wiki/Testsuite) so it would not be a problem if you want completely copy them! Their tests contain RTL text (hebrew, arabic, persian, ...) testing that is really important.
> > Also please check your patch with http://ie.microsoft.com/testdrive/Performance/TextJustificationAnimated/Default.html the first one is related to this bug and second one to https://bugs.webkit.org/show_bug.cgi?id=20606 that is of-course a separate bug :)
> 
> Oh. You've worked for this property on Mozilla! That's nice! :)
> 
> 1. http://ie.microsoft.com/testdrive/Performance/TextJustificationAnimated/Default.html is working fine.
> 
> 2. Test cases in https://bug536557.bugzilla.mozilla.org/attachment.cgi?id=533614 are quite a lot.
> Do you really think we need to applying all of those in this patch?

And, RTL cases are also included in my test case. ;-)

[Ebrahim Byagowi]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > These are my proposes! :)
> > > Could you borrow some tests from the Mozilla bug (https://bugzilla.mozilla.org/show_bug.cgi?id=536557 https://bug536557.bugzilla.mozilla.org/attachment.cgi?id=533614), Mozilla tests are under Public Domain (http://wiki.whatwg.org/wiki/Testsuite) so it would not be a problem if you want completely copy them! Their tests contain RTL text (hebrew, arabic, persian, ...) testing that is really important.
> > > Also please check your patch with http://ie.microsoft.com/testdrive/Performance/TextJustificationAnimated/Default.html the first one is related to this bug and second one to https://bugs.webkit.org/show_bug.cgi?id=20606 that is of-course a separate bug :)
> > 
> > Oh. You've worked for this property on Mozilla! That's nice! :)
> > 
> > 1. http://ie.microsoft.com/testdrive/Performance/TextJustificationAnimated/Default.html is working fine.
> > 
> > 2. Test cases in https://bug536557.bugzilla.mozilla.org/attachment.cgi?id=533614 are quite a lot.
> > Do you really think we need to applying all of those in this patch?
> 
> And, RTL cases are also included in my test case. ;-)

Thanks! :) IMHO it is enough if you tested Hebrew text tests manually and adding in tests is not very important (but would be cool)! Also another test that I proposed for Mozilla was this ( https://bugzilla.mozilla.org/show_bug.cgi?id=536557#c3 ) you can test it on this page http://fa.wikipedia.org/w/index.php?oldid=8222219#Test2 after installing this font http://www.scict.ir/portal/File/ShowFile.aspx?ID=bea5ca36-1fdf-41d4-8818-c1a4f9c71081 expected difference before/after the patch must be  https://bug536557.bugzilla.mozilla.org/attachment.cgi?id=534295 but please don't stop committing your patch for this test because for example font render engine on Chrome Linux can not render Nastaliq well and this is obviously is a separate bug :)

[Dongwoo Joshua Im (dwim)]

(In reply to comment #29)
> Thanks! :) IMHO it is enough if you tested Hebrew text tests manually and adding in tests is not very important (but would be cool)! Also another test that I proposed for Mozilla was this ( https://bugzilla.mozilla.org/show_bug.cgi?id=536557#c3 ) you can test it on this page http://fa.wikipedia.org/w/index.php?oldid=8222219#Test2 after installing this font http://www.scict.ir/portal/File/ShowFile.aspx?ID=bea5ca36-1fdf-41d4-8818-c1a4f9c71081 expected difference before/after the patch must be  https://bug536557.bugzilla.mozilla.org/attachment.cgi?id=534295 but please don't stop committing your patch for this test because for example font render engine on Chrome Linux can not render Nastaliq well and this is obviously is a separate bug :)

I've tested with the text-align-last-justify.html and text-align-last-justify-rtl.html in the Mozilla test cases,
and it works properly.

[Dongwoo Joshua Im (dwim)]

Created attachment 168699 [details]
patch

I've added total 8 test cases.

7 test cases  - each case tests one of the value of text-align-last property.
one more case - text-align-last:justify for one line, and Hebrew character also.

[Dongwoo Joshua Im (dwim)]

Created attachment 168703 [details]
patch

I re-based the patch.

[Eric Seidel (no email)]

Attachment 168703 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/mac/fast/css/text-align-last-end-expected.png:0:  Have to enable auto props in the subversion config file (/Users/eseidel/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/Users/eseidel/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongwoo Joshua Im (dwim)]

Created attachment 168824 [details]
Patch

[Rik Cabanier]

Attachment 168824 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
WARNING: Patch's size is 82 kbytes.
Patches 20k or smaller are more likely to be reviewed. Larger patches may sit unreviewed for a long time.
LayoutTests/platform/efl/fast/css/text-align-last-auto-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/efl/fast/css/text-align-last-auto-expected.png).  [image/png] [5]
LayoutTests/platform/mac/fast/css/text-align-last-end-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/fast/css/text-align-last-end-expected.png).  [image/png] [5]
LayoutTests/platform/efl/fast/css/text-align-last-start-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/efl/fast/css/text-align-last-start-expected.png).  [image/png] [5]
LayoutTests/platform/efl/fast/css/text-align-last-left-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/efl/fast/css/text-align-last-left-expected.png).  [image/png] [5]
LayoutTests/platform/efl/fast/css/text-align-last-center-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/efl/fast/css/text-align-last-center-expected.png).  [image/png] [5]
LayoutTests/platform/mac/fast/css/text-align-last-right-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/fast/css/text-align-last-right-expected.png).  [image/png] [5]
LayoutTests/platform/mac/fast/css/text-align-last-left-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/fast/css/text-align-last-left-expected.png).  [image/png] [5]
LayoutTests/platform/mac/fast/css/text-align-last-start-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/fast/css/text-align-last-start-expected.png).  [image/png] [5]
LayoutTests/platform/mac/fast/css/text-align-last-center-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/fast/css/text-align-last-center-expected.png).  [image/png] [5]
LayoutTests/platform/efl/fast/css/text-align-last-justify-one-line-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/efl/fast/css/text-align-last-justify-one-line-expected.png).  [image/png] [5]
LayoutTests/platform/efl/fast/css/text-align-last-justify-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/efl/fast/css/text-align-last-justify-expected.png).  [image/png] [5]
LayoutTests/platform/efl/fast/css/text-align-last-right-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/efl/fast/css/text-align-last-right-expected.png).  [image/png] [5]
LayoutTests/platform/mac/fast/css/text-align-last-auto-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/fast/css/text-align-last-auto-expected.png).  [image/png] [5]
LayoutTests/platform/mac/fast/css/text-align-last-justify-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/fast/css/text-align-last-justify-expected.png).  [image/png] [5]
LayoutTests/platform/efl/fast/css/text-align-last-end-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/efl/fast/css/text-align-last-end-expected.png).  [image/png] [5]
LayoutTests/platform/mac/fast/css/text-align-last-justify-one-line-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/fast/css/text-align-last-justify-one-line-expected.png).  [image/png] [5]
Total errors found: 16 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongwoo Joshua Im (dwim)]

I've split this patch into parsing and rendering, 
regarding jchaffraix's advise on irc.

* parsing : https://bugs.webkit.org/show_bug.cgi?id=99439
* rendering : https://bugs.webkit.org/show_bug.cgi?id=99584

[Dongwoo Joshua Im (dwim)]

I change this bug as a meta bug of implementing the text-align-last feature, regarding the suggestion of jchaffraix on the bug, https://bugs.webkit.org/show_bug.cgi?id=99439.

[Ebrahim Byagowi]

As Bug 6203 (kashida justification) is moved to CSS4 (http://dev.w3.org/csswg/css-text-4/) and CSS4 currently is in draft state and it is related to text-justify and not text-align-last anyway I am removing it from here. If depended bug (parsing and rendering parts) are resolved this is also resolved AFAIK.

Thank you very much for fixing this bug :)

[yisibl]

WebKit when is it going to support it?

[Steven Vachon]

Every other browser supports it.

[Ebrahim Byagowi]

Hey guys, can this get another boost?

[Chad von Nau]

Why is this marked "RESOLVED FIXED"? text-align-last still doesn't work in Safari 12.0.2, STP 73, or Mobile Safari 12.1.2.

[Sander]

16 months after the previous comment, this still does NOT work. Can someone please change it from "RESOLVED FIXED" to something else?

[Sam Sneddon [:gsnedders]]

(In reply to Chad von Nau from comment #42)
> Why is this marked "RESOLVED FIXED"? text-align-last still doesn't work in
> Safari 12.0.2, STP 73, or Mobile Safari 12.1.2.

(In reply to Sander from comment #43)
> 16 months after the previous comment, this still does NOT work. Can someone
> please change it from "RESOLVED FIXED" to something else?

As was CSS WG policy at the time of implementation, and as the spec (https://www.w3.org/TR/2013/WD-css-text-3-20131010/) said:

> Prior to a specification reaching the Candidate Recommendation stage in the W3C process, all implementations of a CSS feature are considered experimental. The CSS Working Group recommends that implementations use a vendor-prefixed syntax for such features, including those in W3C Working Drafts. This avoids incompatibilities with future changes in the draft.

See Bug 229083 for unprefixing it, in accordance with current spec status and CSS WG guidelines.