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

Le 2015-08-17 15:08, Chris Rebert a écrit :
> 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

I do not use github. I'll try to find your test anyway...

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

When the first version of the file format documentation was created, 
zero-filled number suffix was explained to keep filename unique. As more 
and more test suites for 40+ specifications were created, there is a 
need to keep filename unique... otherwise test searching (with CSS Test 
Suite Manager Shepherd system: http://test.csswg.org/shepherd/ ) gets 
more difficult, confusing and returning several occurences.

But, you are right; I agree with you that this "testcase filename itself 
must be globally unique" guideline or requirement should be stated 
somewhere in the Test the Web Forward documentation.

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

Then, in my opinion, it should state such guideline and then be updated 
accordingly.

Such guideline is not mentioned in the documentation. As a test author, 
you want to provide a link to the most stable version of a spec so that 
your test remains as much reliable and complete as possible. Once a spec 
becomes Proposed Recommendation or Technical Recommendation, then *I 
think (speculation)* working drafts no longer exist and links are not 
redirected.

"
This is a draft document and may be updated, replaced or obsoleted by 
other documents at any time. It is inappropriate to cite this document 
as other than work in progress.
"
https://drafts.csswg.org/css2/#status


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

I do not use or know about that W3C Sync Bot; I think (just a 
hunch/speculation) such software only synchronized between github and 
Mercurial ... but I really don't know...

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

The modified test I created made that 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.
> 
> 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,

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

iPhone 6, Safari 7+ fail this additional test.
The text assert and a few other things in that 
right-offset-position-fixed-001 would need to be adjusted, tweaked.

This new test proves that
http://test.csswg.org/suites/css2.1/nightly-unstable/html4/bidi-position-fixed-001.htm
was basic or was not very challenging.

> but I'll be happy to write the `position:absolute` variant as soon as
> the current testcase is approved.

I use Mercurial. If your test(s) is(are) not in

http://test.csswg.org/source/

or not in

http://test.csswg.org/source/css21/

then I can not approve them. Right now, I do not see your test anywhere 
in Shepherd system and in test.csswg.org/source/ .
I speculate this is what W3C Sync Bot ... it should make submitted tests 
in both systems ... but if there is no 
http://test.csswg.org/source/css22/ then the test can not be made 
available.

Gérard

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

-- 
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 Monday, 17 August 2015 20:49:51 UTC