Make css-properties-position-relative-as-parent-fixed.html a ref-test

Make css-properties-position-relative-as-parent-fixed.html a ref-test

Convert fast/css/css-properties-position-relative-as-parent-fixed.html
in a ref-test test avoiding the need to rebaseline the expected results
if we do any modification.

BUG=31286, 425991
TEST=fast/css/css-properties-position-relative-as-parent-fixed.html

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

[Manuel Rego, 2014-10-22 14:51:06]

rego@igalia.com changed reviewers:
+ eseidel@chromium.org, jchaffraix@chromium.org

[Julien - ping for review, 2014-10-23 01:22:27]

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
File LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html
(right):

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:55:
<span class="fixed red" data-offset-x="350">
Nit: I would change red for another color (blue, purple, orange, ...). Red
should be reserved for failures.

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:110:
<span class="fixed red" data-offset-x="600">
That doesn't match the description and old result (red is *after* green)

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:136:
<span class="fixed red" data-offset-x="300">
This doesn't seem to match the description / visual result (red is *after*
green).

[Manuel Rego, 2014-10-23 08:12:22]

Thanks for the review.

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
File LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html
(right):

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:55:
<span class="fixed red" data-offset-x="350">
On 2014/10/23 01:22:27, Julien Chaffraix - PST wrote:
> Nit: I would change red for another color (blue, purple, orange, ...). Red
> should be reserved for failures.

I agree, I was just using red because of it was in the previous test
but you're right.
I've changed it for blue. Thanks for the suggestion.

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:110:
<span class="fixed red" data-offset-x="600">
On 2014/10/23 01:22:27, Julien Chaffraix - PST wrote:
> That doesn't match the description and old result (red is *after* green)


The offset in the fixed elements is absolute, so it's not relative
to the container.
In addition, being RTL makes it more complex to understand the
offset values.
It's also important remember that the container already has
"left: 150px;".

So, the result actually matches the description. If the offsets are
hard to understand we might think in doing a ref-test instead of a
checkLayout() one.
The other option would be to keep the old test, but I think changing
it is better in the long run.

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:136:
<span class="fixed red" data-offset-x="300">
On 2014/10/23 01:22:27, Julien Chaffraix - PST wrote:
> This doesn't seem to match the description / visual result (red is *after*
> green).

Ditto.

[Julien - ping for review, 2014-10-23 15:54:21]

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
File LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html
(right):

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:110:
<span class="fixed red" data-offset-x="600">
On 2014/10/23 at 08:12:21, Manuel Rego wrote:
> On 2014/10/23 01:22:27, Julien Chaffraix - PST wrote:
> > That doesn't match the description and old result (red is *after* green)
> 
> 
> The offset in the fixed elements is absolute, so it's not relative
> to the container.
> In addition, being RTL makes it more complex to understand the
> offset values.
> It's also important remember that the container already has
> "left: 150px;".

You're right, the offset is defined with respect to the offsetParent, which is
close to the containing block with some differences.

> 
> So, the result actually matches the description. If the offsets are
> hard to understand we might think in doing a ref-test instead of a
> checkLayout() one.

The offset are hard to understand as they refer to different containing blocks.
The old test had the upside of being visual and thus easy to visually check.

I would support a ref-test.

> The other option would be to keep the old test, but I think changing
> it is better in the long run.

We're on the same page, let's change this test for something better!

[Manuel Rego, 2014-10-24 10:07:37]

Thanks for the review. Now I'm trying with a ref-test.

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
File LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html
(right):

https://codereview.chromium.org/669143002/diff/1/LayoutTests/fast/css/css-pro...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:110:
<span class="fixed red" data-offset-x="600">
On 2014/10/23 15:54:21, Julien Chaffraix - PST wrote:
> On 2014/10/23 at 08:12:21, Manuel Rego wrote: 
> > So, the result actually matches the description. If the offsets are
> > hard to understand we might think in doing a ref-test instead of a
> > checkLayout() one.
> 
> The offset are hard to understand as they refer to different containing
blocks.
> The old test had the upside of being visual and thus easy to visually check.

FWIW, the checkLayout() test could also be checked visually, as despite of
using Ahem, the different colors make it possible.

> I would support a ref-test.

I've just uploaded a new version using a ref-test.

[Julien - ping for review, 2014-11-10 21:56:28]

lgtm.

> Become css-properties-position-relative-as-parent-fixed.html a ref-test

Let's use some good English here (I don't think you can become in this way):

Make css-properties-position-relative-as-parent-fixed.html a ref-test
Transform css-properties-position-relative-as-parent-fixed.html into a ref-test

https://codereview.chromium.org/669143002/diff/40001/LayoutTests/fast/css/css...
File LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html
(right):

https://codereview.chromium.org/669143002/diff/40001/LayoutTests/fast/css/css...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:144:
<span class="relative green" style="left: 50px;">
Is it expected for you to change this value? (it was left: 100px)

The question applies to the other similar changes below.

[Manuel Rego, 2014-11-11 10:14:19]

Thanks for the review.

I've updated the title as suggested.

https://codereview.chromium.org/669143002/diff/40001/LayoutTests/fast/css/css...
File LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html
(right):

https://codereview.chromium.org/669143002/diff/40001/LayoutTests/fast/css/css...
LayoutTests/fast/css/css-properties-position-relative-as-parent-fixed.html:144:
<span class="relative green" style="left: 50px;">
On 2014/11/10 21:56:27, Julien Chaffraix - PST wrote:
> Is it expected for you to change this value? (it was left: 100px)
> 
> The question applies to the other similar changes below.

Yeah, you're right. These changes are not needed anymore, they were
modified when I was using the Ahem font to make things fit better,
but now we can keep the old values.

[Manuel Rego, 2014-11-11 10:15:36]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-11-11 10:16:13]

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

[commit-bot: I haz the power, 2014-11-11 11:34:39]

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

[commit-bot: I haz the power, 2014-11-11 11:34:40]

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

[Manuel Rego, 2014-11-11 11:40:58]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-11-11 11:47:00]

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

[commit-bot: I haz the power, 2014-11-11 13:04:12]

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

[commit-bot: I haz the power, 2014-11-11 13:04:13]

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

[Manuel Rego, 2014-11-12 11:28:11]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-11-12 11:28:28]

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

[commit-bot: I haz the power, 2014-11-12 14:02:41]

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

[commit-bot: I haz the power, 2014-11-12 14:02:42]

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

[Manuel Rego, 2014-11-12 14:12:37]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-11-12 14:13:13]

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

[commit-bot: I haz the power, 2014-11-12 15:46:16]

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

[commit-bot: I haz the power, 2014-11-12 15:46:17]

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

[Manuel Rego, 2014-11-12 16:18:54]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-11-12 16:20:03]

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

[commit-bot: I haz the power, 2014-11-12 16:52:35]

Committed patchset #5 (id:80001) as 185219