New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[css-tables] Add tests for visibility: collapse, visibility:hidden #6199
[css-tables] Add tests for visibility: collapse, visibility:hidden #6199
Conversation
Firefox (nightly)Testing web-platform-tests at revision 0a9a7ba All results22 tests ran/css/css-tables-3/visibility-collapse-rowspan-005.html
/css/css-tables-3/visibility-collapse-col-001.html
/css/css-tables-3/visibility-collapse-col-002.html
/css/css-tables-3/visibility-collapse-col-003.html
/css/css-tables-3/visibility-collapse-col-004.html
/css/css-tables-3/visibility-collapse-non-rowcol-001.html
/css/css-tables-3/visibility-collapse-row-001.html
/css/css-tables-3/visibility-collapse-row-002-dynamic.html
/css/css-tables-3/visibility-collapse-row-003-dynamic.html
/css/css-tables-3/visibility-collapse-row-group-001.html
/css/css-tables-3/visibility-collapse-row-group-002.html
/css/css-tables-3/visibility-collapse-rowspan-001.html
/css/css-tables-3/visibility-collapse-rowspan-002-border-separate.html
/css/css-tables-3/visibility-collapse-rowspan-002.html
/css/css-tables-3/visibility-collapse-rowspan-003-border-separate.html
/css/css-tables-3/visibility-collapse-rowspan-003.html
/css/css-tables-3/visibility-collapse-rowspan-004-dynamic.html
/css/css-tables-3/visibility-hidden-col-001.html
/css/css-tables-3/visibility-hidden-nested-001.html
/css/css-tables-3/visibility-hidden-nested-002.html
/css/css-tables-3/visibility-hidden-row-001.html
/css/css-tables-3/visibility-hidden-row-002.html
|
Sauce (safari)Testing web-platform-tests at revision 0a9a7ba All results22 tests ran/css/css-tables-3/visibility-collapse-col-001.html
/css/css-tables-3/visibility-collapse-col-002.html
/css/css-tables-3/visibility-collapse-col-003.html
/css/css-tables-3/visibility-collapse-col-004.html
/css/css-tables-3/visibility-collapse-non-rowcol-001.html
/css/css-tables-3/visibility-collapse-row-001.html
/css/css-tables-3/visibility-collapse-row-002-dynamic.html
/css/css-tables-3/visibility-collapse-row-003-dynamic.html
/css/css-tables-3/visibility-collapse-row-group-001.html
/css/css-tables-3/visibility-collapse-row-group-002.html
/css/css-tables-3/visibility-collapse-rowspan-001.html
/css/css-tables-3/visibility-collapse-rowspan-002-border-separate.html
/css/css-tables-3/visibility-collapse-rowspan-002.html
/css/css-tables-3/visibility-collapse-rowspan-003-border-separate.html
/css/css-tables-3/visibility-collapse-rowspan-003.html
/css/css-tables-3/visibility-collapse-rowspan-004-dynamic.html
/css/css-tables-3/visibility-hidden-col-001.html
/css/css-tables-3/visibility-hidden-nested-001.html
/css/css-tables-3/visibility-hidden-nested-002.html
/css/css-tables-3/visibility-hidden-row-001.html
/css/css-tables-3/visibility-hidden-row-002.html
/css/css-tables-3/visibility-collapse-rowspan-005.html
|
Chrome (unstable)Testing web-platform-tests at revision 0a9a7ba All results22 tests ran/css/css-tables-3/visibility-collapse-col-001.html
/css/css-tables-3/visibility-collapse-col-002.html
/css/css-tables-3/visibility-collapse-col-003.html
/css/css-tables-3/visibility-collapse-col-004.html
/css/css-tables-3/visibility-collapse-non-rowcol-001.html
/css/css-tables-3/visibility-collapse-row-001.html
/css/css-tables-3/visibility-collapse-row-002-dynamic.html
/css/css-tables-3/visibility-collapse-row-003-dynamic.html
/css/css-tables-3/visibility-collapse-row-group-001.html
/css/css-tables-3/visibility-collapse-row-group-002.html
/css/css-tables-3/visibility-collapse-rowspan-001.html
/css/css-tables-3/visibility-collapse-rowspan-002-border-separate.html
/css/css-tables-3/visibility-collapse-rowspan-002.html
/css/css-tables-3/visibility-collapse-rowspan-003-border-separate.html
/css/css-tables-3/visibility-collapse-rowspan-003.html
/css/css-tables-3/visibility-collapse-rowspan-004-dynamic.html
/css/css-tables-3/visibility-hidden-col-001.html
/css/css-tables-3/visibility-hidden-nested-001.html
/css/css-tables-3/visibility-hidden-nested-002.html
/css/css-tables-3/visibility-hidden-row-001.html
/css/css-tables-3/visibility-hidden-row-002.html
/css/css-tables-3/visibility-collapse-rowspan-005.html
|
Sauce (MicrosoftEdge)Testing web-platform-tests at revision 0a9a7ba All results22 tests ran/css/css-tables-3/visibility-collapse-col-001.html
/css/css-tables-3/visibility-collapse-col-002.html
/css/css-tables-3/visibility-collapse-col-003.html
/css/css-tables-3/visibility-collapse-col-004.html
/css/css-tables-3/visibility-collapse-non-rowcol-001.html
/css/css-tables-3/visibility-collapse-row-001.html
/css/css-tables-3/visibility-collapse-row-002-dynamic.html
/css/css-tables-3/visibility-collapse-row-003-dynamic.html
/css/css-tables-3/visibility-collapse-row-group-001.html
/css/css-tables-3/visibility-collapse-row-group-002.html
/css/css-tables-3/visibility-collapse-rowspan-001.html
/css/css-tables-3/visibility-collapse-rowspan-002-border-separate.html
/css/css-tables-3/visibility-collapse-rowspan-002.html
/css/css-tables-3/visibility-collapse-rowspan-003-border-separate.html
/css/css-tables-3/visibility-collapse-rowspan-003.html
/css/css-tables-3/visibility-collapse-rowspan-004-dynamic.html
/css/css-tables-3/visibility-hidden-col-001.html
/css/css-tables-3/visibility-hidden-nested-001.html
/css/css-tables-3/visibility-hidden-nested-002.html
/css/css-tables-3/visibility-hidden-row-001.html
/css/css-tables-3/visibility-hidden-row-002.html
/css/css-tables-3/visibility-collapse-rowspan-005.html
|
w3c-test:mirror |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good @joysu. One thing that I'm not seeing is any testing of replaced elements nor dynamic modifications. Knowing that Edge currently has a bug in invalidation it would be useful to ensure that we're mimicking a user invalidation of some type. Thanks.
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use generate_tests()
- we are actively discouraging it. I know that we have throughout the testcases that are currently in WIP and you probably got that pattern from us. To fix this, use a for loop and then assert_equals()
. You can see more on this on the WPT Documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, I know that I initially spoke with @davidsgrogan and we planned to put them in WIP, and he had the good point of just putting them into CSS3 tables path. We should begin on doing this, the path is css/css-tables-3
. I won't block on this as @FremyCompany or myself will move them a bit later. I also think it's imperative that we resolve on the spec language for the sizing and painting of visibility: collapse
within issue 478. @FremyCompany and myself will try to land this, so if you could please stage these in the actual tables folder that would be greatly appreciated, but not required for my sign off. Thanks!
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
</main> | ||
|
||
<script> | ||
generate_tests(assert_equals, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
Thanks so much @gregwhitworth for the code review! In addition to moving the tests to css-tables-3 and taking out generate_tests, I added three row span tests. I also added three tests with dynamic changes: visibility-collapse-col-004.html, visibility-collapse-row-004.html, and visibility-collapse-row-005.html (this also collapses an image). I'm not too sure what you mean by replaced elements; does collapsing an image count? |
Also, visibility-collapse-nested-001.html addresses the Edge bug :) |
8f34bba
to
615bd55
Compare
615bd55
to
403532f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joysyu Overall this is really great. We're almost there, a few more changes as I tested them in FF/Edge and cross compared with the spec. Thank you so much - great work!
<script src='/resources/testharnessreport.js'></script> | ||
<link rel='stylesheet' href='../support/base.css' /> | ||
<link rel="author" title="Joy Yu" href="mailto:joysyu@mit.edu"> | ||
<link rel="help" href="https://drafts.csswg.org/css-x-tables-3/#computing-the-x-table-height"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there an x
in here - this makes the url fail. Seems like a find/replace issue as the hash has -x-
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
</x-table> | ||
<p> | ||
Row 1 and Row 2 are both visibility:visible. Row 1 and Row 2 are within the same row group | ||
which is visibility: collapse. Rows 1 and 2 should not be visible.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test needs to be a bit more complex as Edge technically passes this as the table is sized to correct height, but row 1 & 2 are visible in Edge and this test didn't catch it.
], | ||
[ | ||
document.getElementById('two').offsetHeight, | ||
116, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, this test doesn't catch that Edge paints row 1 & 2 over top of 3. Additionally, if you're actually testing that setting collapse on row group doesn't make rows appear, then change the file name to something like ...-row-group-YYY.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this is actually the exact same test as visibility-collapse-nested-001, so I've deleted that and fixed this test to be more complex to catch Edge's bug. Thanks for catching this!
@@ -0,0 +1,89 @@ | |||
<!doctype html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add dynamic to the end of this file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
</main> | ||
|
||
<script> | ||
colgroup = document.getElementById("collapse"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are getting a row, not a colgroup - change the variable name accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,105 @@ | |||
<!doctype html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please create a copy of rowspan-002
& rowspan-003
and create ones that are in border-collapse: seperate
mode? Make sure this is reflected in the name so it's easy to differentiate the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
f7bfa83
to
c41c565
Compare
c41c565
to
706eb44
Compare
Thank you @gregwhitworth! I've made the changes :) Let me know what you think! |
52dd95d
to
ad5b42d
Compare
@gregwhitworth @FremyCompany ping? |
Joy eventually landed these through chromium's sync. This can be closed. |
No description provided.