Issue 790943004: An align attribute should not be applied for wider children

Issue 790943004: An align attribute should not be applied for wider children (Closed)

Created:
6 years ago by Kyungtae Kim

Modified:
6 years ago

Reviewers:
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

An align attribute should not be applied for wider children If a child block to be aligned is wider than its containg block, the margin should not adjusted, just like the auto margin case. BUG=440320 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187157

Patch Set 1 #

Total comments: 13

Patch Set 2 : consider align:center #

Total comments: 8

Patch Set 3 : #

Total comments: 1

Patch Set 4 : #

Patch Set 5 : Rebaseline the failed testcase #

Total comments: 2

Patch Set 6 : Rebaseline the failed testcase (reftest) #

Created: 6 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+134 lines, -101 lines)			Patch
A	LayoutTests/fast/block/align-bigger-child.html	View	1 2	1 chunk	+73 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/block/align-bigger-child-expected.txt	View	1	1 chunk	+16 lines, -0 lines	0 comments	Download
D	LayoutTests/platform/linux/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right-expected.png	View	1 2 3 4	Binary file		0 comments	Download
D	LayoutTests/platform/linux/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right-expected.txt	View	1 2 3 4	1 chunk	+0 lines, -16 lines	0 comments	Download
D	LayoutTests/platform/mac/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right-expected.png	View	1 2 3 4	Binary file		0 comments	Download
D	LayoutTests/platform/mac/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right-expected.txt	View	1 2 3 4	1 chunk	+0 lines, -16 lines	0 comments	Download
D	LayoutTests/platform/win-xp/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right-expected.png	View	1 2 3 4	Binary file		0 comments	Download
D	LayoutTests/platform/win-xp/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right-expected.txt	View	1 2 3 4	1 chunk	+0 lines, -16 lines	0 comments	Download
D	LayoutTests/platform/win/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right-expected.png	View	1 2 3 4	Binary file		0 comments	Download
D	LayoutTests/platform/win/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right-expected.txt	View	1 2 3 4	1 chunk	+0 lines, -16 lines	0 comments	Download
A +	LayoutTests/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right-expected.html	View	1 2 3 4 5	2 chunks	+11 lines, -7 lines	0 comments	Download
M	Source/core/rendering/RenderBox.cpp	View	1 2 3	1 chunk	+34 lines, -30 lines	0 comments	Download

Messages

Total messages: 21 (6 generated)

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

mstensho (USE GERRIT)

https://codereview.chromium.org/790943004/diff/1/LayoutTests/fast/dom/margin-for-aligning-bigger-child.html File LayoutTests/fast/dom/margin-for-aligning-bigger-child.html (right): https://codereview.chromium.org/790943004/diff/1/LayoutTests/fast/dom/margin-for-aligning-bigger-child.html#newcode1 LayoutTests/fast/dom/margin-for-aligning-bigger-child.html:1: <!DOCTYPE html> Uh-oh - you put this in fast/dom/ ...

6 years ago (2014-12-10 10:45:05 UTC) #3

Kyungtae Kim

6 years ago (2014-12-10 11:50:38 UTC) #4

https://codereview.chromium.org/790943004/diff/1/LayoutTests/fast/dom/margin-...
File LayoutTests/fast/dom/margin-for-aligning-bigger-child.html (right):

https://codereview.chromium.org/790943004/diff/1/LayoutTests/fast/dom/margin-...
LayoutTests/fast/dom/margin-for-aligning-bigger-child.html:1: <!DOCTYPE html>
On 2014/12/10 10:45:05, mstensho wrote:
> Uh-oh - you put this in fast/dom/ ? Sorry, I didn't notice that you did the
same
> in https://codereview.chromium.org/766223004/ . I should have suggested a
better
> place. How about fast/block/ ? Also, I think the test name could be shortened
to
> "align-bigger-child".

Done.

https://codereview.chromium.org/790943004/diff/1/LayoutTests/fast/dom/margin-...
LayoutTests/fast/dom/margin-for-aligning-bigger-child.html:12: <div
align="right" style="width:100px">
On 2014/12/10 10:45:05, mstensho wrote:
> Could you move the width declarations to the style sheet above?
> 
> Inline style attributes is fine, but please be consistent about it.

Done.

https://codereview.chromium.org/790943004/diff/1/LayoutTests/fast/dom/margin-...
LayoutTests/fast/dom/margin-for-aligning-bigger-child.html:18: <div dir="rtl"
align="left" style="width:100px">
On 2014/12/10 10:45:05, mstensho wrote:
> Nice and important to test RTL, but it would be useful to test ltr as well. I
> realize that align="left" dir="ltr" has worked prior to your changes, but
> testing it would be nice nevertheless, after changes like this.

Done.

https://codereview.chromium.org/790943004/diff/1/LayoutTests/fast/dom/margin-...
LayoutTests/fast/dom/margin-for-aligning-bigger-child.html:22: 
On 2014/12/10 10:45:05, mstensho wrote:
> Should fix and add a test for align="center" too.

Done.

https://codereview.chromium.org/790943004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/790943004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2147: if (marginBoxWidth < availableWidth) {
On 2014/12/10 10:45:05, mstensho wrote:
> Should move this check to include center alignment (above) too.

Done.

https://codereview.chromium.org/790943004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2152: if (marginEndLength.isAuto() ||
(adjustFromTextAlign && hasInvertedDirection && !marginStartLength.isAuto())) {
On 2014/12/10 10:45:05, mstensho wrote:
> Now is a good time to get rid of adjustFromTextAlign and hasInvertedDirection,
> and just set marginEndLength and/or marginStartLength to Auto when align is
> left, center or right.
 I think it could be confusing.
Because auto margins always have a priority over align,
we need to check many combinations before setting it.
How do you think about that?

mstensho (USE GERRIT)

https://codereview.chromium.org/790943004/diff/1/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/790943004/diff/1/Source/core/rendering/RenderBox.cpp#newcode2152 Source/core/rendering/RenderBox.cpp:2152: if (marginEndLength.isAuto() || (adjustFromTextAlign && hasInvertedDirection && !marginStartLength.isAuto())) { ...

6 years ago (2014-12-10 23:27:01 UTC) #5

https://codereview.chromium.org/790943004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/790943004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2152: if (marginEndLength.isAuto() ||
(adjustFromTextAlign && hasInvertedDirection && !marginStartLength.isAuto())) {
On 2014/12/10 11:50:38, Kyungtae Kim wrote:
> On 2014/12/10 10:45:05, mstensho wrote:
> > Now is a good time to get rid of adjustFromTextAlign and
hasInvertedDirection,
> > and just set marginEndLength and/or marginStartLength to Auto when align is
> > left, center or right.
>  I think it could be confusing.
> Because auto margins always have a priority over align,
> we need to check many combinations before setting it.
> How do you think about that?

The code for WEBKIT_CENTER needs to remain special, because it actually aligns
the margin box, not the border box, in that case. But for WEBKIT_LEFT and
WEBKIT_RIGHT, something like this:

if (containingBlockStyle->textAlign() == WEBKIT_LEFT ||
containingBlockStyle->textAlign() == WEBKIT_RIGHT) {
    enum BlockAlign { None, Start, End };
    BlockAlign blockAlign = None;
    if (containingBlockStyle->textAlign() == WEBKIT_LEFT) {
        if (!containingBlockStyle->isLeftToRightDirection())
            blockAlign = style()->isLeftToRightDirection() ? Start : End;
    } else { // WEBKIT_RIGHT
        if (containingBlockStyle->isLeftToRightDirection())
            blockAlign = !style()->isLeftToRightDirection() ? Start : End;
    }
    if (blockAlign == Start) {
        if (!marginStartLength.isAuto())
            marginEndLength = Length(Auto);
    } else if (blockAlign == End) {
        if (!marginEndLength.isAuto())
            marginStartLength = Length(Auto);
    }
}

And then just handle the auto margins according to the CSS spec, without the
need for any of the two flags.

https://codereview.chromium.org/790943004/diff/20001/LayoutTests/fast/block/a...
File LayoutTests/fast/block/align-bigger-child.html (right):

https://codereview.chromium.org/790943004/diff/20001/LayoutTests/fast/block/a...
LayoutTests/fast/block/align-bigger-child.html:11: <body>
No need for the BODY tag, then.

https://codereview.chromium.org/790943004/diff/20001/LayoutTests/fast/block/a...
LayoutTests/fast/block/align-bigger-child.html:13: <div class="narrow_block">
How about an align="left" here?

https://codereview.chromium.org/790943004/diff/20001/LayoutTests/fast/block/a...
LayoutTests/fast/block/align-bigger-child.html:28: 
"left-aligned" -> "right-aligned"

https://codereview.chromium.org/790943004/diff/20001/LayoutTests/fast/block/a...
LayoutTests/fast/block/align-bigger-child.html:29: <div dir="rtl"
class="narrow_block">
How about an align="right" here?

Kyungtae Kim

https://codereview.chromium.org/790943004/diff/20001/LayoutTests/fast/block/align-bigger-child.html File LayoutTests/fast/block/align-bigger-child.html (right): https://codereview.chromium.org/790943004/diff/20001/LayoutTests/fast/block/align-bigger-child.html#newcode11 LayoutTests/fast/block/align-bigger-child.html:11: <body> On 2014/12/10 23:27:00, mstensho wrote: > No need ...

6 years ago (2014-12-11 03:32:47 UTC) #6

mstensho (USE GERRIT)

lgtm with one comment nit. https://codereview.chromium.org/790943004/diff/40001/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/790943004/diff/40001/Source/core/rendering/RenderBox.cpp#newcode2147 Source/core/rendering/RenderBox.cpp:2147: // Adjust margins from ...

6 years ago (2014-12-11 09:01:02 UTC) #7

commit-bot: I haz the power

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

6 years ago (2014-12-11 09:15:29 UTC) #9

commit-bot: I haz the power

Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/38020)

6 years ago (2014-12-11 12:22:38 UTC) #11

Kyungtae Kim

https://codereview.chromium.org/790943004/diff/80001/LayoutTests/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right.html File LayoutTests/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right.html (right): https://codereview.chromium.org/790943004/diff/80001/LayoutTests/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right.html#newcode75 LayoutTests/tables/mozilla_expected_failures/marvin/table_overflow_td_align_right.html:75: shouldBe("target.getBoundingClientRect().left","128"); I need to rebaseline this test because it ...

6 years ago (2014-12-12 02:42:57 UTC) #12

commit-bot: I haz the power

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

6 years ago (2014-12-15 00:06:59 UTC) #14

commit-bot: I haz the power

Try jobs failed on following builders: win_blink_rel on tryserver.blink ()

6 years ago (2014-12-15 06:26:44 UTC) #16

mstensho (USE GERRIT)

Looks like the trybots found something actually failing because of this CL?

6 years ago (2014-12-15 09:01:28 UTC) #17

mstensho (USE GERRIT)

6 years ago (2014-12-15 09:27:48 UTC) #18

commit-bot: I haz the power

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

6 years ago (2014-12-15 14:58:25 UTC) #20

commit-bot: I haz the power

6 years ago (2014-12-15 17:05:07 UTC) #21

Message was sent while issue was closed.

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

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