Issue 19471004: Rendering the property, text-align-last.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+525 lines, -2 lines)			Patch
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last.css	View	1 2 3 4 5	1 chunk	+65 lines, -0 lines	1 comment	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-auto.html	View	1 2 3 4 5	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-auto-expected.html	View	1 2 3 4 5	1 chunk	+45 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-center.html	View	1 2 3 4 5	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-center-expected.html	View	1 2 3 4 5	1 chunk	+45 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-end.html	View	1 2 3 4 5	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-end-expected.html	View	1 2 3 4 5	1 chunk	+45 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-left.html	View	1 2 3 4 5	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-left-expected.html	View	1 2 3 4 5	1 chunk	+45 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-right.html	View	1 2 3 4 5	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-right-expected.html	View	1 2 3 4 5	1 chunk	+45 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start.html	View	1 2 3 4 5	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html	View	1 2 3 4 5	1 chunk	+45 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderBlockLineLayout.cpp	View	1 2 3 4 5	1 chunk	+27 lines, -2 lines	0 comments	Download
M	Source/core/rendering/style/RenderStyle.cpp	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 18 (0 generated)

Expand Messages | Collapse Messages

dw.im

This patch is depend on the other one - https://codereview.chromium.org/17155007.

7 years, 5 months ago (2013-07-19 06:37:09 UTC) #2

dw.im

Dear Julien, I think this patch is kind of ready to get review from you, ...

7 years, 2 months ago (2013-10-22 01:15:20 UTC) #3

Julien - ping for review

I looked briefly at the test cases but they all deserve the same comment about ...

7 years, 2 months ago (2013-10-22 15:06:05 UTC) #4

dw.im

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html File LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html (right): https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html#newcode5 LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html:5: <style type="text/css"> .last {text-align:start} </style> On 2013/10/22 15:06:06, Julien ...

7 years, 2 months ago (2013-10-23 02:40:58 UTC) #5

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text...
File
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html
(right):

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html:5:
<style type="text/css"> .last {text-align:start} </style>
On 2013/10/22 15:06:06, Julien Chaffraix wrote:
> Let's remove type which is meaningless.

I see. will remove it.

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text...
File LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start.html
(right):

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start.html:2:
<html lang="en">
On 2013/10/22 15:06:06, Julien Chaffraix wrote:
> Let's remove the lang attribute.

will remove it.

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start.html:5:
<style type="text/css"> div {text-align-last:start} </style>
On 2013/10/22 15:06:06, Julien Chaffraix wrote:
> Again a super broad selector which makes the test confusing. I had missed it
in
> my first review and thus didn't understand why everything was text-align-last:
> start (and not auto the initial value).
> 
> I think this should be a class and you explicitly mark the divs so that the
> intent is clearer.

I want to test text-align-last:start works properly with various text-align
values, in this test case.
The other values of text-align-last are tested in its dedicated test case.

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start.html:10: 
On 2013/10/22 15:06:06, Julien Chaffraix wrote:
> What's expected? How can someone unfamiliar with the test knows if it passes?

There is *-expected.html file, 
but still I can upload the result png file as a reference.

I didn't make the test as pixel test because I don't want to add more png files
in trunk.

If you think pixel test could be more clear, I will try to make it in that way.

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text...
File LayoutTests/fast/css3-text/css3-text-align-last/text-align-last.css
(right):

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last.css:1: div
{width:150px; border:1px solid #000000; font-size:14pt}
On 2013/10/22 15:06:06, Julien Chaffraix wrote:
> Please one rule per line, it's way more readable.

will fix it.

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text...
LayoutTests/fast/css3-text/css3-text-align-last/text-align-last.css:4: 
On 2013/10/22 15:06:06, Julien Chaffraix wrote:
> FWIW I find this shared style sheet to be confusing as you apply very broad
> selectors instead of something related to text-align-last. See for example
> flexbox.css or grid.css for something that is more focused.
> 
> An example of what you share here is:
> 
> .textAlignLastStart {
>    text-align-last: start;
> }

will try. thanks!

https://codereview.chromium.org/19471004/diff/7001/Source/core/rendering/Rend...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/19471004/diff/7001/Source/core/rendering/Rend...
Source/core/rendering/RenderBlockLineLayout.cpp:716: alignment = TASTART;
On 2013/10/22 15:06:06, Julien Chaffraix wrote:
> return TASTART;
> 
> This could even be written:
> 
> return (alignment == JUSTIFY) ? TASTART: alignment;

Oh, it could be.

Julien - ping for review

https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start.html File LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start.html (right): https://codereview.chromium.org/19471004/diff/7001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start.html#newcode10 LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start.html:10: On 2013/10/23 02:40:58, dw.im wrote: > On 2013/10/22 15:06:06, ...

7 years, 2 months ago (2013-10-23 15:41:51 UTC) #6

dw.im

One question, Julien. I'm trying to make a test case for text-align-last:justify, but it is ...

7 years, 2 months ago (2013-10-25 10:54:54 UTC) #8

Julien - ping for review

On 2013/10/25 10:54:54, dw.im wrote: > One question, Julien. > > I'm trying to make ...

7 years, 1 month ago (2013-10-25 16:21:16 UTC) #9

Julien - ping for review

https://codereview.chromium.org/19471004/diff/107001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html File LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html (right): https://codereview.chromium.org/19471004/diff/107001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html#newcode15 LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html:15: <p class='testP'>LTR align is start</p> Why do we need ...

7 years, 1 month ago (2013-10-25 18:49:19 UTC) #10

dw.im

7 years, 1 month ago (2013-10-28 02:56:14 UTC) #11

dw.im

https://codereview.chromium.org/19471004/diff/107001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last.css File LayoutTests/fast/css3-text/css3-text-align-last/text-align-last.css (right): https://codereview.chromium.org/19471004/diff/107001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last.css#newcode4 LayoutTests/fast/css3-text/css3-text-align-last/text-align-last.css:4: font-size:14pt On 2013/10/25 18:49:19, Julien Chaffraix - PST wrote: ...

7 years, 1 month ago (2013-10-30 05:42:44 UTC) #12

Julien - ping for review

lgtm https://codereview.chromium.org/19471004/diff/107001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html File LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html (right): https://codereview.chromium.org/19471004/diff/107001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html#newcode15 LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-start-expected.html:15: <p class='testP'>LTR align is start</p> On 2013/10/28 02:56:14, ...

7 years, 1 month ago (2013-10-30 20:49:39 UTC) #13

dw.im

https://codereview.chromium.org/19471004/diff/657001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-auto.html File LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-auto.html (right): https://codereview.chromium.org/19471004/diff/657001/LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-auto.html#newcode11 LayoutTests/fast/css3-text/css3-text-align-last/text-align-last-auto.html:11: The last line of each case should be aligned ...

7 years, 1 month ago (2013-10-31 02:09:05 UTC) #14

dw.im

Dear Julien, I've used Ahem.ttf file to guarantee the line breaking point. Please review this ...

7 years, 1 month ago (2013-10-31 02:26:25 UTC) #15

Julien - ping for review

Note that the previous testing was OK, sometimes I like to nitpick but don't have ...

7 years, 1 month ago (2013-11-01 00:34:42 UTC) #16

commit-bot: I haz the power

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

7 years, 1 month ago (2013-11-01 00:50:37 UTC) #17

Message was sent while issue was closed.

Change committed as 161122

Expand Messages | Collapse Messages

Issue 19471004: Rendering the property, text-align-last. (Closed)

Description

Patch Set 1 #

Patch Set 2 : WIP patch #

Patch Set 3 : 1st patch for review #

Patch Set 4 : 2nd patch for review #

Patch Set 5 : 3rd patch for review #

Patch Set 6 : Using Ahem.ttf font #

Messages