text-align-last only affect to the last line when text-align is justify.

text-align-last only affect to the last line when text-align is justify.

CSS3 Text spec about text-align-last has been modified
as the property only can affect to the last line when test-align is justify.

http://www.w3.org/TR/css-text-3/#text-align-last-property

BUG=248839
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179082

[Zoltan, 2014-01-29 19:46:13]

https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-justify.html
(right):

https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-justify.html:4:
<link href="resources/text-align-last.css" rel="stylesheet" type="text/css">
I think it's easier to read, if you don't use a separate css file for the css
properties. I don't see any advantage using class='alignJustify' over
style='text-align: justify', furthermore the second case seems more readable,
since everything is at one place.

https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-justify.html:19:
<div dir='ltr' class='testDiv alignJustify alignLastEnd'>RTL align : justify
align-last : end</div>
The tests says RTL align, the div says dir='ltr'. That doesn't seem correct.

Do you think that we actually need to test for the directions? Since in these
cases the alignment is explicitly controlled by CSS properties, I think it's
unnecessary for testing it.

https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify-expected.html
(right):

https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify-expected.html:19:
<div dir='ltr' class='testDiv alignRight'>RTL align : right align-last :
left</div>
LTR vs RTL, and so on at the following test cases.

https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html
(right):

https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html:8:
text align is not justify, and all of the values of text-align-last<br><br>
Using a <p> tag?

You can really do a reftest for this property. Check out my test at WebKit
http://trac.webkit.org/browser/trunk/LayoutTests/fast/css3-text/css3-text-ali...

I know that Julien prefers readable content in the tests, since the default font
type should be the same for the actual and the expected result, it shouldn't be
a problem to do with it.

[dw.im, 2014-06-16 10:10:26]

On 2014/01/29 19:46:13, Zoltan wrote:
>
https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
> File
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-justify.html
> (right):
> 
>
https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-justify.html:4:
> <link href="resources/text-align-last.css" rel="stylesheet" type="text/css">
> I think it's easier to read, if you don't use a separate css file for the css
> properties. I don't see any advantage using class='alignJustify' over
> style='text-align: justify', furthermore the second case seems more readable,
> since everything is at one place.
> 
Except 'testDiv', those are not really re-used more then three times, 
so, writing as inline for readability would make sense.
Done.


>
https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-justify.html:19:
> <div dir='ltr' class='testDiv alignJustify alignLastEnd'>RTL align : justify
> align-last : end</div>
> The tests says RTL align, the div says dir='ltr'. That doesn't seem correct.
> 
> Do you think that we actually need to test for the directions? Since in these
> cases the alignment is explicitly controlled by CSS properties, I think it's
> unnecessary for testing it.
> 
Done. Only LTR test cases are remained.


>
https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
> File
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify-expected.html
> (right):
> 
>
https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify-expected.html:19:
> <div dir='ltr' class='testDiv alignRight'>RTL align : right align-last :
> left</div>
> LTR vs RTL, and so on at the following test cases.
> 
ditto.


>
https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
> File
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html
> (right):
> 
>
https://codereview.chromium.org/59753002/diff/40001/LayoutTests/fast/css3-tex...
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html:8:
> text align is not justify, and all of the values of text-align-last<br><br>
> Using a <p> tag?
> 
Seems not necessary br tags, so removed those.


> You can really do a reftest for this property. Check out my test at WebKit
>
http://trac.webkit.org/browser/trunk/LayoutTests/fast/css3-text/css3-text-ali...
> 
> I know that Julien prefers readable content in the tests, since the default
font
> type should be the same for the actual and the expected result, it shouldn't
be
> a problem to do with it.
>
I will go with readable content for this time.

[dw.im, 2014-06-16 10:31:02]

PTAL

[leviw_travelin_and_unemployed, 2014-06-17 22:14:31]

https://codereview.chromium.org/59753002/diff/190001/LayoutTests/fast/css3-te...
File
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html
(right):

https://codereview.chromium.org/59753002/diff/190001/LayoutTests/fast/css3-te...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html:20:
<div dir='ltr' class='testDiv' style='text-align:center;
text-align-last:left'>LTR align : center align-last : left</div>
Instead of a pixel test, could we wrap the last line in an element then check
its offsets in a script to see if it's correct?

[dw.im, 2014-06-23 02:10:11]

https://codereview.chromium.org/59753002/diff/190001/LayoutTests/fast/css3-te...
File
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html
(right):

https://codereview.chromium.org/59753002/diff/190001/LayoutTests/fast/css3-te...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html:20:
<div dir='ltr' class='testDiv' style='text-align:center;
text-align-last:left'>LTR align : center align-last : left</div>
On 2014/06/17 22:14:31, leviw wrote:
> Instead of a pixel test, could we wrap the last line in an element then check
> its offsets in a script to see if it's correct?

I think pixel test could be the choice for this.
And, instead of adding new png sets, ref test would be better way.
I made two tests - one ref test, and one pixel test.

Anyway, I will investigate that your suggestion can be applied.

[dw.im, 2014-06-23 10:13:31]

On 2014/06/23 02:10:11, dw.im wrote:
>
https://codereview.chromium.org/59753002/diff/190001/LayoutTests/fast/css3-te...
> File
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html
> (right):
> 
>
https://codereview.chromium.org/59753002/diff/190001/LayoutTests/fast/css3-te...
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html:20:
> <div dir='ltr' class='testDiv' style='text-align:center;
> text-align-last:left'>LTR align : center align-last : left</div>
> On 2014/06/17 22:14:31, leviw wrote:
> > Instead of a pixel test, could we wrap the last line in an element then
check
> > its offsets in a script to see if it's correct?
> 
> I think pixel test could be the choice for this.
> And, instead of adding new png sets, ref test would be better way.
> I made two tests - one ref test, and one pixel test.
> 
> Anyway, I will investigate that your suggestion can be applied.

I removed pixel test, and add ref test, instead.
I think this is also good way to go.

PTAL!

[dw.im, 2014-06-23 10:45:28]

Layouttest failure on win_blink_dbg seems not related to this patch, IMO.

[leviw_travelin_and_unemployed, 2014-06-23 15:26:11]

https://codereview.chromium.org/59753002/diff/210001/LayoutTests/fast/css3-te...
File
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html
(right):

https://codereview.chromium.org/59753002/diff/210001/LayoutTests/fast/css3-te...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html:19:
<div class='testDiv' style='text-align:start; text-align-last:end'>start
end</div>
This test doesn't actually validate that this only modifies the last line. It
could just be overriding text-align for all lines and the test would still pass.

[dw.im, 2014-06-24 03:54:32]

On 2014/06/23 15:26:11, leviw wrote:
>
https://codereview.chromium.org/59753002/diff/210001/LayoutTests/fast/css3-te...
> File
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html
> (right):
> 
>
https://codereview.chromium.org/59753002/diff/210001/LayoutTests/fast/css3-te...
>
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-with-text-align-non-justify.html:19:
> <div class='testDiv' style='text-align:start; text-align-last:end'>start
> end</div>
> This test doesn't actually validate that this only modifies the last line. It
> could just be overriding text-align for all lines and the test would still
pass.

Actually, text-align would override text-align-last if the value of text-align
is not justify.
And, there is another test case for the case - "Not overriding" - text-align:
justify in this patchset.

What is your exact expectation for this test?
Combining the two test cases as one so that we can see this is not just
overriding?
If it is, I simply can make it.

[dw.im, 2014-06-24 08:30:49]

Done. PTAL.

[Zoltan, 2014-07-24 20:57:15]

Personally, I'd use multiple lines in the test, but it doesn't really make a
difference. Both the change and the test looks good to me.
As the description says, text-align-last takes into account, only when
text-align is justify.
LGTM.

[leviw_travelin_and_unemployed, 2014-07-28 23:13:42]

lgtm2

[dw.im, 2014-07-28 23:44:59]

The CQ bit was checked by dw.im@samsung.com

[commit-bot: I haz the power, 2014-07-28 23:46:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dw.im@samsung.com/59753002/230001

[commit-bot: I haz the power, 2014-07-29 02:00:46]

Change committed as 179082