Bug 76173 - [meta] Implement CSS3 text-align-last feature on webkit
: [meta] Implement CSS3 text-align-last feature on webkit
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To: Dongwoo Joshua Im (dwim)
: WebExposed
Depends on: 99439 99584 99804
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-12 07:02 PST by ebraminio
Modified: 2014-01-18 02:20 PST (History)
28 users (show)

See Also:


Attachments
WIP patch (14.04 KB, patch)
2012-10-09 01:21 PDT, Dongwoo Joshua Im (dwim)
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
WIP patch (14.04 KB, patch)
2012-10-09 01:45 PDT, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
WIP patch (14.92 KB, patch)
2012-10-09 05:11 PDT, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
WIP patch (14.99 KB, patch)
2012-10-11 02:34 PDT, Dongwoo Joshua Im (dwim)
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
patch without test case (15.47 KB, patch)
2012-10-11 23:00 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
test case (2.39 KB, text/html)
2012-10-11 23:01 PDT, Dongwoo Joshua Im (dwim)
no flags Details
patch (562.86 KB, patch)
2012-10-15 06:24 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
patch (562.68 KB, patch)
2012-10-15 06:46 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff
Patch (565.48 KB, patch)
2012-10-15 17:46 PDT, Dongwoo Joshua Im (dwim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ebraminio 2012-01-12 07:02:25 PST
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
Comment 1 ebraminio 2012-03-30 15:53:08 PDT
Please see me, I am a CSS3 feature :D
Comment 2 Dongwoo Joshua Im (dwim) 2012-10-03 18:37:38 PDT
I will look at this issue. ;-)
Comment 3 Dongwoo Joshua Im (dwim) 2012-10-09 01:21:40 PDT
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.
Comment 4 WebKit Review Bot 2012-10-09 01:25:33 PDT
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.
Comment 5 Early Warning System Bot 2012-10-09 01:41:47 PDT
Comment on attachment 167707 [details]
WIP patch

Attachment 167707 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14214647
Comment 6 Early Warning System Bot 2012-10-09 01:44:30 PDT
Comment on attachment 167707 [details]
WIP patch

Attachment 167707 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14215670
Comment 7 Dongwoo Joshua Im (dwim) 2012-10-09 01:45:55 PDT
Created attachment 167711 [details]
WIP patch
Comment 8 Dongwoo Joshua Im (dwim) 2012-10-09 01:52:26 PDT
(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?
Comment 9 WebKit Review Bot 2012-10-09 02:15:41 PDT
Comment on attachment 167711 [details]
WIP patch

Attachment 167711 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14218621
Comment 10 Build Bot 2012-10-09 02:22:36 PDT
Comment on attachment 167711 [details]
WIP patch

Attachment 167711 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14238211
Comment 11 Early Warning System Bot 2012-10-09 02:23:54 PDT
Comment on attachment 167711 [details]
WIP patch

Attachment 167711 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14215676
Comment 12 Peter Beverloo (cr-android ews) 2012-10-09 02:28:16 PDT
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
Comment 13 Early Warning System Bot 2012-10-09 02:29:10 PDT
Comment on attachment 167711 [details]
WIP patch

Attachment 167711 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14223608
Comment 14 Dongwoo Joshua Im (dwim) 2012-10-09 05:11:04 PDT
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.
Comment 15 WebKit Review Bot 2012-10-09 05:40:34 PDT
Comment on attachment 167738 [details]
WIP patch

Attachment 167738 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14209872
Comment 16 Build Bot 2012-10-09 05:48:30 PDT
Comment on attachment 167738 [details]
WIP patch

Attachment 167738 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14217719
Comment 17 Peter Beverloo (cr-android ews) 2012-10-09 05:55:48 PDT
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
Comment 18 Alexis Menard (darktears) 2012-10-09 11:50:33 PDT
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.
Comment 19 Dongwoo Joshua Im (dwim) 2012-10-11 02:34:30 PDT
Created attachment 168175 [details]
WIP patch
Comment 20 WebKit Review Bot 2012-10-11 03:05:29 PDT
Comment on attachment 168175 [details]
WIP patch

Attachment 168175 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14245861
Comment 21 Build Bot 2012-10-11 03:14:13 PDT
Comment on attachment 168175 [details]
WIP patch

Attachment 168175 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14257460
Comment 22 Peter Beverloo (cr-android ews) 2012-10-11 03:21:51 PDT
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
Comment 23 Dongwoo Joshua Im (dwim) 2012-10-11 23:00:19 PDT
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.
Comment 24 Dongwoo Joshua Im (dwim) 2012-10-11 23:01:46 PDT
Created attachment 168364 [details]
test case

I made a test case for this property.
Please see this is enough.
Comment 25 Dongwoo Joshua Im (dwim) 2012-10-12 01:49:57 PDT
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?
Comment 26 ebraminio 2012-10-12 02:57:18 PDT
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 :)
Comment 27 Dongwoo Joshua Im (dwim) 2012-10-14 21:32:16 PDT
(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?
Comment 28 Dongwoo Joshua Im (dwim) 2012-10-14 22:14:55 PDT
(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. ;-)
Comment 29 ebraminio 2012-10-14 23:34:26 PDT
(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 :)
Comment 30 Dongwoo Joshua Im (dwim) 2012-10-15 04:28:37 PDT
(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.
Comment 31 Dongwoo Joshua Im (dwim) 2012-10-15 06:24:08 PDT
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.
Comment 32 Dongwoo Joshua Im (dwim) 2012-10-15 06:46:47 PDT
Created attachment 168703 [details]
patch

I re-based the patch.
Comment 33 Eric Seidel 2012-10-15 13:29:10 PDT
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.
Comment 34 Dongwoo Joshua Im (dwim) 2012-10-15 17:46:09 PDT
Created attachment 168824 [details]
Patch
Comment 35 Rik Cabanier 2012-10-15 21:20:04 PDT
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.
Comment 36 Dongwoo Joshua Im (dwim) 2012-10-17 04:39:15 PDT
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
Comment 37 Dongwoo Joshua Im (dwim) 2012-10-18 21:59:28 PDT
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.
Comment 38 ebraminio 2014-01-18 02:20:28 PST
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 :)