Consider text alignment and direction when computing the left offset for horizontal writing modes.

Consider text alignment and direction when computing the left offset for horizontal writing modes.

To properly layout a positioned box, we need to consider special cases where the direction of
the text for that object flows towards the opposite direction the text inside that box would be
aligned to (e.g. right aligned text for LTR boxes and left aligned text for RTL boxes).

Not doing this this ways yields to undesired effects, such as having the absolutely positioned
text inside a table cell with relative positioned being wrongly rendered outside of the cell.

BUG=157539
TEST=Try rendering absolutely positioned text inside table cells with relative positioning.
R=eae@chromium.org

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

[mario.prada, 2014-01-30 18:24:39]

Attaching a patch + a new layout test that fixes the issue 157539 without
breaking any other test.

Still, not 100% sure this is the right approach, so any feedback is welcome.
Thanks!

[eae, 2014-01-30 18:38:27]

This looks right to me but I'd like Eric or Levi to take a look.

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

https://codereview.chromium.org/150403003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:3454: if (containerBlockIsLTR &&
containerBlock->simplifiedTextAlign(blockStyle->textAlign()) == RIGHT) {
How about using a local variable to hold the
containerBlock->simplifiedTextAlign(blockStyle->textAlign())? Would make the
code easier to read.

https://codereview.chromium.org/150403003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:3456: logicalLeftPos +=
containerBlock->borderLogicalRight();
Shouldn't this be borderAndPaddingLogicalRight?

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

https://codereview.chromium.org/150403003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:3335: return textAlign;
Why not change this to "return JUSTIFY" to clarify that?

[mario.prada, 2014-02-04 10:40:23]

Now including the layout test (html) I forgot to add in the previous patch set

[mario.prada, 2014-02-04 14:30:12]

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

https://codereview.chromium.org/150403003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:3454: if (containerBlockIsLTR &&
containerBlock->simplifiedTextAlign(blockStyle->textAlign()) == RIGHT) {
On 2014/01/30 18:38:27, eae wrote:
> How about using a local variable to hold the
> containerBlock->simplifiedTextAlign(blockStyle->textAlign())? Would make the
> code easier to read.

Ok

https://codereview.chromium.org/150403003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:3456: logicalLeftPos +=
containerBlock->borderLogicalRight();
On 2014/01/30 18:38:27, eae wrote:
> Shouldn't this be borderAndPaddingLogicalRight?

I've thought the same and actually tried it, but that change was breaking some
other tests so I fell back to only considering the border _for the container_,
which in the end I think it makes sense because we are positioning here the
child object and so we only want to consider the additional space that falls out
of the container block, while still being a part of it: the border.

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

https://codereview.chromium.org/150403003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:3335: return textAlign;
On 2014/01/30 18:38:27, eae wrote:
> Why not change this to "return JUSTIFY" to clarify that?

Ok :)

[mario.prada, 2014-02-04 16:59:38]

Addressing comments from the initial review

[mario.prada, 2014-02-06 14:18:10]

From the linux_layout bot:

Regressions: Unexpected crashes (4)
  storage/quota/storageinfo-no-callbacks.html [ Crash ]
  storage/quota/storageinfo-request-quota.html [ Crash ]
  storage/quota/storagequota-request-persistent-quota.html [ Crash ]
  storage/quota/storagequota-request-quota.html [ Crash ]

...I don't think these failures are related to the patch. Any hint on how to
proceed now?

Thanks

[mario.prada, 2014-02-06 16:03:16]

On 2014/02/06 14:18:10, mario.prada wrote:
> From the linux_layout bot:
> 
> Regressions: Unexpected crashes (4)
>   storage/quota/storageinfo-no-callbacks.html [ Crash ]
>   storage/quota/storageinfo-request-quota.html [ Crash ]
>   storage/quota/storagequota-request-persistent-quota.html [ Crash ]
>   storage/quota/storagequota-request-quota.html [ Crash ]
> 
> ...I don't think these failures are related to the patch. Any hint on how to
> proceed now?
> 
> Thanks

Ah, I see [1] this seems to be a problem present upstream. Guess the right thing
to do is to wait until is fixed there and ask the bots to try again?

[1]
http://build.chromium.org/p/chromium.webkit/waterfall?category=layout&failure...

[mario.prada, 2014-02-13 20:20:09]

Adding the new test to TestExpectations as NeedsRebaseline

[mario.prada, 2014-02-13 20:20:11]

Adding the new test to TestExpectations as NeedsRebaseline

[mario.prada, 2014-02-13 20:20:11]

Adding the new test to TestExpectations as NeedsRebaseline

[mario.prada, 2014-02-13 20:20:12]

Adding the new test to TestExpectations as NeedsRebaseline

[mario.prada, 2014-02-13 20:30:22]

Adding the new test to TestExpectations as NeedsRebaseline (second try)

[mario.prada, 2014-02-13 20:30:23]

Adding the new test to TestExpectations as NeedsRebaseline (second try)

[mario.prada, 2014-02-13 20:30:25]

Adding the new test to TestExpectations as NeedsRebaseline (second try)

[mario.prada, 2014-02-13 20:30:27]

Adding the new test to TestExpectations as NeedsRebaseline (second try)

[mario.prada, 2014-02-13 20:38:31]

Adding the new test to TestExpectations as NeedsRebaseline (third try)

[mario.prada, 2014-02-13 20:40:35]

Adding the new test to TestExpectations as NeedsRebaseline

[mario.prada, 2014-02-13 20:45:11]

On 2014/02/13 20:40:35, mario.prada wrote:
> Adding the new test to TestExpectations as NeedsRebaseline

Apologies for the crazyness here. Apparently I moved out of the expected git
workflow and git cl got crazy when uploading the patch. It seems to be fixed
now, after manually fixing my .git/config file and removing the wrong patch
sets, so that's good.

Anyway, sorry for the noise. At least I hope this new patch will give better
results in the try bots than before :)

[eae, 2014-02-14 10:22:55]

LGTM

[eae, 2014-02-14 10:39:08]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-02-14 10:39:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/150403003/220003

[commit-bot: I haz the power, 2014-02-14 12:10:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 12:10:23]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[mario.prada, 2014-02-14 16:37:25]

Rebased patch against master

[mario.prada, 2014-02-24 17:01:48]

On 2014/02/14 16:37:25, mario.prada wrote:
> Rebased patch against master

By looking at the output of the win_layout bots in different tries, I don't
really think the current failure is related to this patch.

Any suggestion on how to move forward? (this patch has been uploaded here for a
while already)

[eae, 2014-02-24 19:55:11]

On 2014/02/24 17:01:48, mario.prada wrote:
> On 2014/02/14 16:37:25, mario.prada wrote:
> > Rebased patch against master
> 
> By looking at the output of the win_layout bots in different tries, I don't
> really think the current failure is related to this patch.
> 
> Any suggestion on how to move forward? (this patch has been uploaded here for
a
> while already)

I agree. Lets give the CQ one more chance then I'll commit it manually for you.

[eae, 2014-02-24 19:55:17]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-02-24 19:55:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/150403003/650001

[commit-bot: I haz the power, 2014-02-25 01:04:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/150403003/650001

[commit-bot: I haz the power, 2014-02-25 05:16:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/150403003/650001

[commit-bot: I haz the power, 2014-02-25 06:45:16]

Change committed as 167732

[mario.prada, 2014-02-25 14:27:41]

On 2014/02/25 06:45:16, I haz the power (commit-bot) wrote:
> Change committed as 167732

Thanks!