Re: [CSS21][CSS22] Seeking review of position:fixed edge case test

Le 2015-07-27 03:21, Chris Rebert a écrit :
> Hi folks,
> 
> I'm trying to get a reviewer for a CSS2 regression test:
> https://github.com/w3c/csswg-test/pull/813
> 
> Basically, WebKit doesn't currently handle position:fixed correctly
> when it's also top,bottom,left,right:auto and a child of a
> position:relative element. The test asserts the spec'd behavior which
> Chrome, Firefox, IE11, and Presto adhere to.
> 
> Thanks,
> Chris


Chris,

Your test is a good test. Indeed iPhone 6, Safari 7+ fail your test. 
Here's some review comments.


1- There is already a test with the filename position-fixed-001

http://test.csswg.org/suites/css2.1/nightly-unstable/html4/position-fixed-001.htm

in the test suite. Fortunately for you, your test, in my opinion, is 
really about 'left: auto' offset. So, in all fairness, you were destined 
for a test filename change anyway.

2-
<!DOCTYPE html SYSTEM "about:legacy-compat">

Please use

<!DOCTYPE html>
<meta charset="utf-8">
etc..
just like in the test template:
http://testthewebforward.org/docs/test-templates.html#reftest-including-metadata

or use

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" 
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">


3-
     <link rel="help" 
href="https://drafts.csswg.org/css2/visuren.html#choose-position" />
     <link rel="help" 
href="https://drafts.csswg.org/css2/visudet.html#abs-non-replaced-width" 
/>
     <link rel="help" 
href="https://drafts.csswg.org/css2/visudet.html#abs-non-replaced-height" 
/>

Do not link to draft spec as they can change or will change in the 
future. Shepherd also warns against this. And do not use too many links 
to spec: here, the test would be inserted into 3 sections of the CSS2.1 
(or 2.2) test suite... In my opinion, the last one 
(visudet.html#abs-non-replaced-height) is not relevant to your test.

4-
     <meta name="assert" content="An element which is position:fixed 
should have its position calculated correctly using the absolute model 
when its top/bottom/left/right properties all use the default `auto` 
value and its parent is position:relative." />

Avoid use of expressions like "should work correctly", "should be 
implemented correctly", "should be supported correctly", "should be 
calculated correctly" and expressions like those in the meta text assert 
because they do not describe or explain in useful terms what the test is 
supposed to be testing, checking.

5-
#shifted-column {
     left: 50%;
     width: 50%;
     position: relative;
     float: left;
}

Please explain what the float declaration is supposed to be doing in 
that test. Is it really required by your test? Does it do something in 
particular in your test? Is it a necessary and relevant component of 
your test?
I believe the 'float: left' declaration should not be in your test at 
all.

6-
#green {
   background-color: green;
   position: absolute;
   left: 50%;
   z-index: 2;
}

Since the green square is supposed to overlap the red square and since 
it comes after the red square, then you do not need to set a z-index 
since "Boxes with the same stack level in a stacking context are stacked 
back-to-front according to document tree order."

7-
p {
   position: absolute;
   top: 100px;
}

It is recommended to start all tests with the pass-fail-conditions 
sentence and, since it should not be part of the test itself, then we do 
not want to style it in any way. Sometimes, this is not possible but, 
I'd say 99% of the time, it is possible.



Here's what could be your test with all these changes:

http://www.gtalbot.org/BrowserBugsSection/css21testsuite/left-offset-position-fixed-001.xht

The 'left: auto' declaration was added on purpose, deliberately, even 
though we know it is the default, initial box offset.

The reference file for that test has not been created yet.

One last thing. One other equivalent test could be created with 
'direction: rtl', 'position fixed', 'right: auto' for many reasons. 
Also, same 2 tests but with 'position: absolute'. So, 3 other additional 
tests with the same basis code are possible here.

Gérard
-- 
Test Format Guidelines
http://testthewebforward.org/docs/test-format-guidelines.html

Test Style Guidelines
http://testthewebforward.org/docs/test-style-guidelines.html

Test Templates
http://testthewebforward.org/docs/test-templates.html

CSS Naming Guidelines
http://testthewebforward.org/docs/css-naming.html

Test Review Checklist
http://testthewebforward.org/docs/review-checklist.html

CSS Metadata
http://testthewebforward.org/docs/css-metadata.html

Received on Wednesday, 5 August 2015 04:07:22 UTC