Issue 766223004: Blocks should be aligned by style of parents

Issue 766223004: Blocks should be aligned by style of parents (Closed)

Created:
6 years ago by Kyungtae Kim

Modified:
6 years ago

Reviewers:
rhogan, mstensho (USE GERRIT)

CC:
blink-reviews, blink-reviews-rendering, zoltan1, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, Dominik Röttsches, jchaffraix+rendering

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Project:
blink

Visibility:
Public.

More Reviews

Description

After r176154, a block with dir=rtl inside a block with align=right is aligned left, and a block with dir=ltr inside a block with dir=rtl and align=left is aligned right. These margins should be calculated considering these inverted direction cases. BUG=438931 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186828

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Total comments: 6

Patch Set 3 : consider ltr in rtl #

Total comments: 5

Patch Set 4 : consider auto margins & add test cases for them #

Total comments: 4

Patch Set 5 : Split the test case #

Patch Set 6 : #

Created: 6 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+201 lines, -2 lines)			Patch
A	LayoutTests/fast/dom/margin-for-inverted-direction.html	View	1 2 3 4	1 chunk	+57 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/margin-for-inverted-direction-expected.txt	View	1 2 3 4	1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/margin-left-margin-right-auto.html	View	1 2 3 4	1 chunk	+102 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/margin-left-margin-right-auto-expected.txt	View	1 2 3 4	1 chunk	+20 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderBox.cpp	View	1 2 3 4 5	1 chunk	+10 lines, -2 lines	0 comments	Download

Messages

Total messages: 17 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

mstensho (USE GERRIT)

We need a more generic solution to the problem. Please add a test for regular ...

6 years ago (2014-12-04 09:47:57 UTC) #3

Kyungtae Kim

https://codereview.chromium.org/766223004/diff/1/LayoutTests/fast/table/rtl-within-right-alinged-div-expected.txt File LayoutTests/fast/table/rtl-within-right-alinged-div-expected.txt (right): https://codereview.chromium.org/766223004/diff/1/LayoutTests/fast/table/rtl-within-right-alinged-div-expected.txt#newcode2 LayoutTests/fast/table/rtl-within-right-alinged-div-expected.txt:2: RenderView at (0,0) size 800x600 On 2014/12/04 09:47:57, mstensho ...

6 years ago (2014-12-04 11:15:21 UTC) #4

mstensho (USE GERRIT)

The description needs to be fixed. It still says "alinged". You might as well just ...

6 years ago (2014-12-04 12:18:29 UTC) #5

Kyungtae Kim

https://codereview.chromium.org/766223004/diff/20001/LayoutTests/fast/dom/rtl-in-right-aligned-div.html File LayoutTests/fast/dom/rtl-in-right-aligned-div.html (right): https://codereview.chromium.org/766223004/diff/20001/LayoutTests/fast/dom/rtl-in-right-aligned-div.html#newcode14 LayoutTests/fast/dom/rtl-in-right-aligned-div.html:14: rtl_table_in_right_aligned_div = document.getElementById("rtl_table"); On 2014/12/04 12:18:29, mstensho wrote: > ...

6 years ago (2014-12-08 07:00:13 UTC) #6

mstensho (USE GERRIT)

https://codereview.chromium.org/766223004/diff/40001/LayoutTests/fast/dom/margin-for-inverted-direction.html File LayoutTests/fast/dom/margin-for-inverted-direction.html (right): https://codereview.chromium.org/766223004/diff/40001/LayoutTests/fast/dom/margin-for-inverted-direction.html#newcode28 LayoutTests/fast/dom/margin-for-inverted-direction.html:28: shouldBe("rtl_table_in_ltr.getBoundingClientRect().right","792"); Could you set a width on BODY or ...

6 years ago (2014-12-08 10:01:21 UTC) #7

mstensho (USE GERRIT)

On 2014/12/08 10:01:21, mstensho wrote: > https://codereview.chromium.org/766223004/diff/40001/LayoutTests/fast/dom/margin-for-inverted-direction.html > File LayoutTests/fast/dom/margin-for-inverted-direction.html (right): > > https://codereview.chromium.org/766223004/diff/40001/LayoutTests/fast/dom/margin-for-inverted-direction.html#newcode28 > ...

6 years ago (2014-12-08 12:17:25 UTC) #8

Kyungtae Kim

6 years ago (2014-12-09 04:53:08 UTC) #9

mstensho (USE GERRIT)

I think the code changes look good now. I'd rather get rid of that pushToEndFromTextAlign ...

6 years ago (2014-12-09 09:54:42 UTC) #10

I think the code changes look good now.

I'd rather get rid of that pushToEndFromTextAlign variable altogether, and just
set one of the margins to auto and let the machinery do the job, but I see that
that would change the behavior (although in a good way).

This fails in Blink (and probably WebKit), but not in Gecko, IE or Presto:

<p>Should see the word 'PASS' below.</p>
<div style="width:10em;" align="right">
    <div style="width:30em; text-align:left;">PASS</div>
</div>

It's pushToEndFromTextAlign that causes this special-behavior.

If there is no extra space to align within, we should refrain from aligning at
all, it seems, just like we already do for auto margins (per spec). But I think
that's worth a CL on its own, and that we shouldn't worry about it here.

I reported https://code.google.com/p/chromium/issues/detail?id=440320

https://codereview.chromium.org/766223004/diff/40001/LayoutTests/fast/dom/mar...
File LayoutTests/fast/dom/margin-for-inverted-direction.html (right):

https://codereview.chromium.org/766223004/diff/40001/LayoutTests/fast/dom/mar...
LayoutTests/fast/dom/margin-for-inverted-direction.html:28:
shouldBe("rtl_table_in_ltr.getBoundingClientRect().right","792");
On 2014/12/09 04:53:08, Kyungtae Kim wrote:
> On 2014/12/08 10:01:21, mstensho wrote:
> > Could you set a width on BODY or something, so that this test doesn't depend
> on
> > window size?
> 
> Done. Set a width on BODY.

Acknowledged.

https://codereview.chromium.org/766223004/diff/60001/LayoutTests/fast/dom/mar...
File LayoutTests/fast/dom/margin-for-inverted-direction.html (right):

https://codereview.chromium.org/766223004/diff/60001/LayoutTests/fast/dom/mar...
LayoutTests/fast/dom/margin-for-inverted-direction.html:28: 
This file is getting big. I suggest splitting it. Also, I don't think there's
need for all those table tests. There's nothing specific to tables in this bug.

Also, what I really wanted, was something with auto margins and no align
attributes.

https://codereview.chromium.org/766223004/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/766223004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2104: 
Couple of extraneous blank lines.

Kyungtae Kim

Also, I fixed the log to check auto margins first, then handle the 'pushToEnd' case. ...

6 years ago (2014-12-09 11:36:33 UTC) #11

mstensho (USE GERRIT)

On 2014/12/09 11:36:33, Kyungtae Kim wrote: > Also, I fixed the log to check auto ...

6 years ago (2014-12-09 12:00:55 UTC) #12

Kyungtae Kim

> Because otherwise something would break? Yes, in the below cases. <div dir="ltr" align="right"> <!--Should ...

6 years ago (2014-12-09 12:56:24 UTC) #13

mstensho (USE GERRIT)

On 2014/12/09 12:56:24, Kyungtae Kim wrote: > > Because otherwise something would break? > Yes, ...

6 years ago (2014-12-09 13:16:02 UTC) #14

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/766223004/90006

6 years ago (2014-12-10 04:49:05 UTC) #16

commit-bot: I haz the power

6 years ago (2014-12-10 06:11:49 UTC) #17

Message was sent while issue was closed.

Committed patchset #6 (id:90006) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186828

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages