Bug 76173 - [meta] Implement CSS3 text-align-last feature on webkit
: [meta] Implement CSS3 text-align-last feature on webkit
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
: WebExposed
: 99439 99584 99804
:
  Show dependency treegraph
 
Reported: 2012-01-12 07:02 PST by
Modified: 2014-01-18 02:20 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2012-03-30 15:53:08 PST -------
Please see me, I am a CSS3 feature :D
------- Comment #2 From 2012-10-03 18:37:38 PST -------
I will look at this issue. ;-)
------- Comment #3 From 2012-10-09 01:21:40 PST -------
Created an attachment (id=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 From 2012-10-09 01:25:33 PST -------
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 From 2012-10-09 01:41:47 PST -------
(From update of attachment 167707 [details])
Attachment 167707 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14214647
------- Comment #6 From 2012-10-09 01:44:30 PST -------
(From update of attachment 167707 [details])
Attachment 167707 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14215670
------- Comment #7 From 2012-10-09 01:45:55 PST -------
Created an attachment (id=167711) [details]
WIP patch
------- Comment #8 From 2012-10-09 01:52:26 PST -------
(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 From 2012-10-09 02:15:41 PST -------
(From update of attachment 167711 [details])
Attachment 167711 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14218621
------- Comment #10 From 2012-10-09 02:22:36 PST -------
(From update of attachment 167711 [details])
Attachment 167711 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14238211
------- Comment #11 From 2012-10-09 02:23:54 PST -------
(From update of attachment 167711 [details])
Attachment 167711 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14215676
------- Comment #12 From 2012-10-09 02:28:16 PST -------
(From update of attachment 167711 [details])
Attachment 167711 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14223605
------- Comment #13 From 2012-10-09 02:29:10 PST -------
(From update of attachment 167711 [details])
Attachment 167711 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14223608
------- Comment #14 From 2012-10-09 05:11:04 PST -------
Created an attachment (id=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 From 2012-10-09 05:40:34 PST -------
(From update of attachment 167738 [details])
Attachment 167738 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14209872
------- Comment #16 From 2012-10-09 05:48:30 PST -------
(From update of attachment 167738 [details])
Attachment 167738 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14217719
------- Comment #17 From 2012-10-09 05:55:48 PST -------
(From update of attachment 167738 [details])
Attachment 167738 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14207889
------- Comment #18 From 2012-10-09 11:50:33 PST -------
(From update of attachment 167738 [details])
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 From 2012-10-11 02:34:30 PST -------
Created an attachment (id=168175) [details]
WIP patch
------- Comment #20 From 2012-10-11 03:05:29 PST -------
(From update of attachment 168175 [details])
Attachment 168175 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14245861
------- Comment #21 From 2012-10-11 03:14:13 PST -------
(From update of attachment 168175 [details])
Attachment 168175 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14257460
------- Comment #22 From 2012-10-11 03:21:51 PST -------
(From update of attachment 168175 [details])
Attachment 168175 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14255512
------- Comment #23 From 2012-10-11 23:00:19 PST -------
Created an attachment (id=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 From 2012-10-11 23:01:46 PST -------
Created an attachment (id=168364) [details]
test case

I made a test case for this property.
Please see this is enough.
------- Comment #25 From 2012-10-12 01:49:57 PST -------
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 From 2012-10-12 02:57:18 PST -------
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 From 2012-10-14 21:32:16 PST -------
(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 From 2012-10-14 22:14:55 PST -------
(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 From 2012-10-14 23:34:26 PST -------
(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 From 2012-10-15 04:28:37 PST -------
(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 From 2012-10-15 06:24:08 PST -------
Created an attachment (id=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 From 2012-10-15 06:46:47 PST -------
Created an attachment (id=168703) [details]
patch

I re-based the patch.
------- Comment #33 From 2012-10-15 13:29:10 PST -------
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 From 2012-10-15 17:46:09 PST -------
Created an attachment (id=168824) [details]
Patch
------- Comment #35 From 2012-10-15 21:20:04 PST -------
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 From 2012-10-17 04:39:15 PST -------
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 From 2012-10-18 21:59:28 PST -------
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 From 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 :)