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

On Tue, Aug 4, 2015 at 9:06 PM, Gérard Talbot
<css21testsuite@gtalbot.org> wrote:
> 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
<snip>
> Your test is a good test. Indeed iPhone 6, Safari 7+ fail your test. Here's
> some review comments.

Thank you very much for your comments! I've revised the pull request
accordingly:
https://github.com/w3c/csswg-test/pull/813

> 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.

A. This "testcase filename itself must be globally unique" requirement
doesn't seem to be documented anywhere on Test the Web Forward.
B. The "Automatic validation checks" performed by W3C Sync Bot
(https://github.com/w3c/csswg-test/pull/813#issuecomment-124963013 )
didn't complain about this. If it's really a requirement, this seems
like something that the bot could and should enforce.
C. Okay, I renamed it to "left-offset-position-fixed-001.xht"
following your example.

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

Done.

> 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.

Fixed.
Sorry, I had gotten confused since web-platform-tests uses the exact
opposite convention.

For the record, http://testthewebforward.org/docs/css-metadata.html
does not explicitly mention this rule, although its example does use
non-draft links.

Also, again, this seems like something that W3C Sync Bot ought to validate.

> 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.

Removed.
Yeah, I was grasping at straws a bit here for exactly what to cite.
Thanks for the guidance.

> 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",

Changed this to use the superior description from your version.

> 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.

Removed.
It was part of the original grid system CSS which the testcase was
derived from, but it turns out to be irrelevant/unnecessary for this
test.

> 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."

Removed. I was being paranoid.

> 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.

The position:fixed makes doing this somewhat tricky.
If you have a specific suggestion for how to achieve this, I'm fine
with changing the test, but I myself don't have any ideas.

> 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.

Added this `left:auto`.

> 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.

I'm not familiar enough with `direction`/RTL to write those variants,
but I'll be happy to write the `position:absolute` variant as soon as
the current testcase is approved.

Cheers,
Chris
--
Bootstrap Core Team member
Browser 🐛 of the day: http://wkbug.com/148061

Received on Monday, 17 August 2015 19:08:38 UTC