New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 695270 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

overconstrained position:relative in vertical-xx is not in compliance with spec

Project Member Reported by atotic@chromium.org, Feb 23 2017

Issue description

Chrome Version:  56.0.2924.87 (64-bit)
OS: Linux

What steps will reproduce the problem?
(1) Load the attached test

What is the expected result?

No red rectangles.

What happens instead?

There are red rectangles.

This happens because we do not take writing-mode into account when
calculating offsets.

For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.

 
box-offsets-rel-pos-orho-001.xht
2.9 KB Download

Comment 1 by kojii@chromium.org, Feb 23 2017

Cc: goo...@gtalbot.org

Comment 2 by kojii@chromium.org, Feb 23 2017

Cc: kojii@chromium.org
Gérard, do you happen to remember if we have "overconstrained" + "position:relative" in the test suites?

Comment 3 by goo...@gtalbot.org, Feb 23 2017

Koji,

Thank you for adding me in the CC list.

No, we do not have any "position: relative" test where box offsets are over-constrained.

We only have 4 basic tests:

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/box-offsets-rel-pos-vrl-002.htm
and
box-offsets-rel-pos-vlr-003 , box-offsets-rel-pos-vrl-004 , box-offsets-rel-pos-vlr-005

- - - - - - -

The CSS2.1, CSS2.2 and CSS3 Positioned Layout specs give:

"
If neither 'left' nor 'right' is 'auto', the position is over-constrained, and one of them has to be ignored. If the 'direction' property of the containing block is 'ltr', the value of 'left' wins and 'right' becomes -'left'. If 'direction' of the containing block is 'rtl', 'right' wins and 'left' is ignored.
(...)
If neither is 'auto', 'bottom' is ignored (i.e., the used value of 'bottom' will be minus the value of 'top').
"
CSS2.1 & CSS2.2 section 9.4.3 Relative positioning
https://www.w3.org/TR/CSS21/visuren.html#relative-positioning
https://www.w3.org/TR/CSS22/visuren.html#relative-positioning
CSS3, 6.1. Relative positioning
https://www.w3.org/TR/css-position-3/#rel-pos

- - - - - - -

Aleks Totic, 

after a quick look, your test seems good. Although I would split it in 8 distinct, separate smaller tests: 
2 vertical writing-modes combinations [vertical-rl | vertical-lr] and 
2 overconstrained situations [left and right are not 'auto' | top and bottom are not 'auto'] and 
2 directions [ltr | rtl]
========================
8 possible combinations

that way, each and all browser manufacturers would know where to look (clue) in their implementation by knowing which of the 8 smaller tests fail. Splitting in 8 subtests also makes things easier to manage (code maintenance), to figure out, to review. If you test all combinations at the same time, in the same test, it is possible that this could create side effects.

- - - - - - -
 
We already have 7 tests checking overconstrained box offsets with 'position: relative' with both directions in:
http://test.csswg.org/suites/css2.1/nightly-unstable/html4/chapter-9.htm#s9.4.3

eg
http://test.csswg.org/suites/css2.1/nightly-unstable/html4/position-relative-009.htm and position-relative-010 , position-relative-037 , position-relative-038 , relpos-calcs-005 , relpos-calcs-006 , relpos-calcs-007

so we do not need testing 'horizontal-tb' as this has been already done.

- - - - - - - 

4 of the 6 <main>s are in orthogonal flow but the overconstraining relatively position tests are inside those <main>s, so the target of those tests does not involve orthogonal flow. So, I would remove "orho" from the test filename.

- - - - - - -

line 12: <link rel="match" href="box-offsets-rel-pos-ortho-001.xht" />
The reference file needs to be created.

line 14: <meta content="image" name="flags" />
There is no image in the test.

line 22: #target
I would probably rename that id attribute to something that better describes the building logic of the test. Something like: div#reference-green-overlapping
and would try to not use 'position: relative' for it. The only <div>s which would have 'position: relative' would be the <div>s with pairs of box offsets with a non-auto value. Another design would be to leave it in normal flow (position: static), make it red and then have the tested relatively positioned div (with green background) overlap it.

line 58 and 63: "margins" should be "box offsets" or just "offsets"

Comment 4 by goo...@gtalbot.org, Feb 23 2017

line 7: "precedence" should be instead "over-constrained"

When searching for tests, people will use, re-use (or even copy and paste) the same determinant word that the spec mentions. So, "over-constrained" is very important to be in the <title> text of the test. 

"over-constrained" is also a bit better than "overconstrained" (line 81). Again, if I do a search in Shepherd for "over-constrained", the search results will miss the tests that use "overconstrained" in <title> or in /* comments */. This may look like nitpicking but it's not... as the number of test suites increases and the number of tests increases with time, details like that have their importance.

Aleks, these are just comments to make your test as best as possible. Like I said, your test seems correct to me (haven't had a chance to scrutinize it) and it has useful comments too.

Comment 5 by atotic@chromium.org, Feb 23 2017

Your comments are great. I will create 8 tests, titled:

box-offsets-rel-precedence-<writing-mode>-<direction>-<lr|tb>.xhr

box-offsets-rel-precedence-vrl-ltr-lr.xhr
box-offsets-rel-precedence-vrl-ltr-tb.xhr
box-offsets-rel-precedence-vrl-rtl-lr.xhr
box-offsets-rel-precedence-vrl-rtl-tb.xhr
box-offsets-rel-precedence-vrl-ltr-lr.xhr
box-offsets-rel-precedence-vrl-ltr-tb.xhr
box-offsets-rel-precedence-vrl-rtl-lr.xhr
box-offsets-rel-precedence-vrl-rtl-tb.xhr

Do I need a numeric suffix? That seems to be the standard. Do numbers have any special meaning?

> Another design would be to leave it in normal flow (position: static), make it
> red and then have the tested relatively positioned div

That's a good idea. Target green box was relative because it needed to be displayed on top.

Should I just create a pull request on github?
 

Comment 6 by atotic@chromium.org, Feb 23 2017

Aside, while I have your ear, one suggestion for csswg tests:

The existing tests often reuse same numeric values in unrelated different places throughout the test. For example, position-relative-009.htm 

div { height: 1in; width: 1in; }
#div1 {
  background: orange;
}
#div2
{
  background: blue;
  left: 1in;
  position: relative;
  right: 1in;
  top: -1in;
}

Here, 1in is reused for many different purposes, and when debugging it, it can be hard to keep a mental map of what each 1in represents, and how they relate. If this test was rewritten as:

div { height: 1in; width: 2in; }
#div1 {
  background: orange;
}
#div2
{
  background: blue;
  left: 2in;
  position: relative;
  right: 0.1in;
  top: -1in;
}

it is easier to see what dimensions are related. 

Comment 8 by goo...@gtalbot.org, Feb 23 2017

> Do I need a numeric suffix? 

Yes. It is best.

> Do numbers have any special meaning?

They help to know, to count how many tests we have in a serie of tests but it also helps to easily differentiate tests in a serie. If the first test of a serie starts with 001 and the last test of a serie is 0ij, then we know there is ij tests in such serie of tests. There is 230 tests testing absolute positioning of non-replaced block (abs-pos-non-replaced-v*) because the first one is abs-pos-non-replaced-vrl-002 and the last one is abs-pos-non-replaced-vlr-229

> Here, 1in is reused for many different purposes, and when debugging it, it 
> can be hard to keep a mental map of what each 1in represents, and how 
> they relate. If this test was rewritten as:
> (...)
>  left: 2in;
>  position: relative;
>  right: 0.1in;

Since one of the 2, 'left' or 'right', is going to win, then you want each of 'left' and 'right' to use the same value so that when the test fails, it will fail, it can fail, it should fail by such value. You always want to maximize (for many reasons) the cause of failure and the graphical distance of test failure (noticeability of test failure). 

Eg: 
http://test.csswg.org/suites/css2.1/20110323/html4/max-height-017.htm
is a bad test. If the test fails, if 'max-height' is not implemented or incorrectly implemented, then the height difference between the 2 black shapes will be only 1.3px. Again, this is a bad test.

- - - - - -

Generally speaking, an ideal test is
a) is a test that has a very narrow corridor of success and very large corridor of failure ,
b) is very demanding, requiring ,
c) is a test that checks, verifies one and only one single aspect of the spec.
(but you can also create compound tests and more complex tests: yours was a compound and complex test, I would say. Since separate tests are passed by Firefox 54, then my experience leads me to believe that it is possible your test was causing some unanticipated, unexpected side effects or your test was inaccurate somewhere... this is not clear right now).

Comment 9 by atotic@chromium.org, Feb 23 2017

That is even better. I am perfectly happy if you put your name down as creator too.

My patched Chrome passes all the new tests. The Chrome56 fails them all as expected.

> Firefox 54.0a1 buildID=20170223110219 passes all 8 tests ... while I see a red stripe with your test when using Firefox 54

Ooops, I attached a buggy test. Originally, I was using CSS variables, and forgot a '-' sign on rewrite. Attaching corrected test just in case someone uses it.

When will the new tests land?
box-offsets-rel-pos-orho-001.xht
2.9 KB Download

Comment 10 by goo...@gtalbot.org, Feb 24 2017

From what I can see from a browser shots service, MS-Edge 13 (presumably 13.10586) passes the 8 tests in comment 7. This would have to be confirmed by someone really using Windows 10 and, preferably, Edge 15 (15.14986). Koji, if Firefox 54 and Edge 15 (15.14986) pass those 8 tests, then that would mean 2 implementations.

Comment 11 by goo...@gtalbot.org, Feb 24 2017

> I am perfectly happy if you put your name down as creator too.

It's maybe better that you are author because I am a skilled reviewer.

> My patched Chrome passes all the new tests.

You already patched Chrome?! Really?

> When will the new tests land?

Aleks, I could do that as soon as this evening. But I have urgent and personal tasks for the next 2 days. I will create, submit (I use Mercurial v.4.1) the 8 tests with your name as author and with me as reviewer on Saturday February 26th 2017 ... if that is okay with you...

I do not see your name as test author in Shepherd management system:
http://test.csswg.org/shepherd/search/
Are you registered as an author over there?
No hurry about checking in, just wanted to know when to start watching the csswg imports. This was so much faster than I expected already.

Edge and FF passed all my tests too.

> I do not see your name as test author in Shepherd management system:

I have not contributed before. I've just registered as atotic@chromium

> You already patched Chrome?! Really?

As soon as kojii approves:

https://codereview.chromium.org/2712773003
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f97b71bdc98d3c785b329c21134f3925f51ff3a

commit 0f97b71bdc98d3c785b329c21134f3925f51ff3a
Author: atotic <atotic@chromium.org>
Date: Fri Feb 24 05:19:46 2017

Fix precedence of relative position in vertical modes.

Original code did not take writing mode into account.

BUG= 695270 

Review-Url: https://codereview.chromium.org/2712773003
Cr-Commit-Position: refs/heads/master@{#452758}

[modify] https://crrev.com/0f97b71bdc98d3c785b329c21134f3925f51ff3a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp

Comment 15 by goo...@gtalbot.org, Feb 26 2017

Aleks, 

I have created the reference files:

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-overconstrained-rel-pos-ltr-top-bottom-vrl-002-ref.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-overconstrained-rel-pos-ltr-top-bottom-vlr-003-ref.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-overconstrained-rel-pos-ltr-left-right-vrl-004-ref.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-overconstrained-rel-pos-ltr-left-right-vlr-005-ref.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-overconstrained-rel-pos-rtl-top-bottom-vrl-006-ref.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-overconstrained-rel-pos-rtl-top-bottom-vlr-007-ref.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-overconstrained-rel-pos-rtl-left-right-vrl-008-ref.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-overconstrained-rel-pos-rtl-left-right-vlr-009-ref.xht

Unfortunately, while developing the reference files, I believe I have stumbled on an issue or bug involving how inline replaced elements are supposed to be vertically (logically) aligned inside a tall (logically) line box (where line-height of block box containing the inline replaced element is greater than 1). I need to make sure this is a bug (I believe it is) and then spend time to find how to adjust both the tests and the reference files so that they both avoid or work around this... 

Comment 16 by goo...@gtalbot.org, Feb 28 2017

Aleks, Koji,

I have submitted the 8 tests with their associated reference files a few min. ago:

http://hg.csswg.org/test/rev/49c472839bba

http://test.csswg.org/source/css-writing-modes-3/?C=M;O=D

So, they will be added to the build system tonight. 

Aleks Totić: I added a credit note toward you into each submitted tests. Since there is a possible problem with how inline replaced are supposed to be aligned inside the line box, I decided to not make you author and to not make myself reviewer. If the 8 tests are correct, your involvement ( Issue 695270  reporter, original excellent test) is recognized and acknowledged. If the 8 tests are not perfectly reliable for the reason I mentioned in comment 15, then I will be the only author to blame. The 8 tests and associated reference files need to be reviewed now.

The tests listed in comment 7 now have the same credit note.

Comment 17 by goo...@gtalbot.org, Feb 28 2017

> when to start watching the csswg imports

This is  Issue 492664  and Koji may be able to answer this.
The import is failing only for Linux, the result and match differ by 1px. See "images" or "diff" links on:
https://storage.googleapis.com/chromium-layout-test-archives/linux_trusty_blink_rel/5791/layout-test-results/results.html
(ignore the last one, it's unrelated)

I don't know what the bot will do, it'll probably import them, but bots will skip testing them on Linux until it's fixed.

Gérard, is it possible for you to look into why this is happening?
Koji, Aleks,

Thank you for your prompt, diligent feedback on this.

First of all, last night, right after the system built the test suite, I tried the tests with the test harness and then realized that 4 required support images were missing! My fault, here. So, I quickly "hg-add"-ed them:

date: Wed, 01 Mar 2017 03:46:47 -0500
http://hg.csswg.org/test/rev/4500204f4a8a

> The import is failing only for Linux, the result and match differ 
> by 1px. See "images" or "diff" links on:
https://storage.googleapis.com/chromium-layout-test-archives/linux_trusty_blink_rel/5791/layout-test-results/results.html

Thanks for the link (a very useful page too). Yes, I see the 1px or 2px problem. 

The reference files, as coded, may be inaccurate: I haven't had time to investigate this and haven't had time to further investigate what I reported in comment 15. And the thing is: I will not be able to before March 6th (next week). But this will be on my to-do-list and top priority. Also, Mercurial is soon to be dropped; this makes things more difficult for me :( .
The overconstrained-rel-pos-ltr-top-bottom-vlr-003.xht and overconstrained-rel-pos-ltr-top-bottom-vrl-002.xht were not missing the support images. So, the 1px difference reported in [...]/5791/layout-test-results/results.html has to be some kind of rounding issue or miscalculation or (my current top hypothesis) related to baseline alignment of image in the reference file. In the reference file overconstrained-rel-pos-ltr-top-bottom-vrl-002-ref.xht, 

  div
    {
      margin-right: 270px; /* 278px minus 8px */
      margin-top: 40px; /* 8px + 40px + 8px + 40px == 88px */
    }

40px is estimated to be: 36px (image [1] height) plus 4px descender space but this 4px descender space is not reliable; that 4px descender space is not the result of a calculation but rather an approximation . I would need to set font-size (say to 16px) and then set line-height to 1 in the reference file to ensure that descender space below the image is 0px; that way, I would be sure that the img is "sitting" at the bottom of line box, is "sitting" on the bottom of line box. 

[1]: <img src="support/pass-cdts-abs-pos-non-replaced.png" width="246" height="36"

In conclusion, I believe I know (or at least, I have a strong clue, hint) how to make the tests more reliable but this will have to wait until March 6th. And I could also create 4 tests related to what I wrote in comment 15.
[Deep breath] Okay. I finally figured out how to make 1 reference file more precise, and this, without having to specify 'font-size' and 'line-height'. The error I was making was indeed related to how the images were vertically "sitting" (or vertically not "sitting") inside the line box.

  p
    {
      bottom: 0px;
      margin: 4px 8px;
    }

has been elegantly replaced with

  p
    {
      bottom: -8px;
      right: 16px;
    }

  img
    {
      vertical-align: bottom;
    }

in the reference file

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-overconstrained-rel-pos-rtl-top-bottom-vrl-006-ref.xht

So, I modified accordingly the reference file 

http://test.csswg.org/source/css-writing-modes-3/overconstrained-rel-pos-rtl-top-bottom-vrl-006-ref.xht

at the test repository in this manner:

http://hg.csswg.org/test/rev/92c886b0e1b2

If this modification is accurate and works for the 006-ref, then the needed changes for all the other reference files (causing a 1px vertical offset) will be easy to make. 

Koji, please reimport the reference file for overconstrained-rel-pos-rtl-top-bottom-vrl-006 and then please let me know if this works for Windows, MacOSX and Linux.

Comment 22 by goo...@gtalbot.org, Mar 22 2017

Koji,

I modified accordingly the following 5 reference files:

http://hg.csswg.org/test/rev/f86f08483d02

http://test.csswg.org/source/css-writing-modes-3/overconstrained-rel-pos-ltr-top-bottom-vrl-002-ref.xht

http://test.csswg.org/source/css-writing-modes-3/overconstrained-rel-pos-ltr-top-bottom-vlr-003-ref.xht

http://test.csswg.org/source/css-writing-modes-3/overconstrained-rel-pos-rtl-top-bottom-vlr-007-ref.xht

http://test.csswg.org/source/css-writing-modes-3/overconstrained-rel-pos-rtl-left-right-vrl-008-ref.xht

http://test.csswg.org/source/css-writing-modes-3/overconstrained-rel-pos-rtl-left-right-vlr-009-ref.xht

So, all 6 tests and associated reference files mentioned in comment 18 should *not cause* a 1px offset.

Koji, please re-import the beforementioned 6 tests and 6 reference files that caused an annoying 1px offset and then please let me know if these 6 tests work accurately, perfectly for Windows, MacOSX *and* Linux.

Mercurial will be removed in 2-3 days from now.
Since /source/ tests have been moved due to merger into the Web Platform Tests repository and due to migration into GitHub, I am updating the links to the tests and associated reference files:

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/overconstrained-rel-pos-ltr-top-bottom-vrl-002.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/reference/overconstrained-rel-pos-ltr-top-bottom-vrl-002-ref.htm


http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/overconstrained-rel-pos-ltr-top-bottom-vlr-003.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/reference/overconstrained-rel-pos-ltr-top-bottom-vlr-003-ref.htm


http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/overconstrained-rel-pos-ltr-left-right-vrl-004.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/reference/overconstrained-rel-pos-ltr-left-right-vrl-004-ref.htm


http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/overconstrained-rel-pos-ltr-left-right-vlr-005.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/reference/overconstrained-rel-pos-ltr-left-right-vlr-005-ref.htm


http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/overconstrained-rel-pos-rtl-top-bottom-vrl-006.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/reference/overconstrained-rel-pos-rtl-top-bottom-vrl-006-ref.htm


http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/overconstrained-rel-pos-rtl-top-bottom-vlr-007.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/reference/overconstrained-rel-pos-rtl-top-bottom-vlr-007-ref.htm


http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/overconstrained-rel-pos-rtl-left-right-vrl-008.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/reference/overconstrained-rel-pos-rtl-left-right-vrl-008-ref.htm


http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/overconstrained-rel-pos-rtl-left-right-vlr-009.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/reference/overconstrained-rel-pos-rtl-left-right-vlr-009-ref.htm


As far as I can say and see, this Issue has been FIXED: Chrome 58.0.3029.96 passes all 8 tests.

Gérard
Status: Fixed (was: Assigned)
Thanks for all the help Gerard. You've made it so easy to add tests, I am looking forward to clarifying more DOM edge cases. Currenly on my list are overflow, and static position for position:absolute.

Sign in to add a comment