[css-ui-3] Cursor test udpate#7894
Conversation
The assertion in this test did not match what the test was doing, and was probably a copy+paste leftover.
This adjusts and completes the test suite, reflecting the changes resolved in w3c/csswg-drafts#1598 (comment)
Build PASSEDStarted: 2017-10-19 06:51:15 View more information about this build on: |
mrego
left a comment
There was a problem hiding this comment.
LGTM, just some minor comments inline feel free to ignore them.
| <title>CSS Basic User Interface Test: cursor:auto on unselectable text</title> | ||
| <link rel="author" title="Florian Rivoal" href="https://florian.rivoal.net/"> | ||
| <link rel="help" href="http://www.w3.org/TR/css3-ui/#cursor"> | ||
| <link rel="help" href="https://drafts.csswg.org/css-ui-4/#content-selection"> |
There was a problem hiding this comment.
Just a question, this is linking to css-ui-4 but this folder is css-ui-3.
I think the current agreement is that we'll end up having only one folder for each spec,
but letting you know just to be sure this is not a mistake.
There was a problem hiding this comment.
This is primarily a test about css-ui-3, so it is in the level 3 directory, and has a link to level 3. But it does (optionally, thanks to @supports) depend on a level 4 feature, so I'm linking to that too.
| /* Hide the text if we cannot make it unselectable. | ||
| user-select is not part of css-ui level 3, so we should not depend on it | ||
| but it is nice to test it if it is supported. | ||
| Test for level 4 can take the conditional out. */ |
There was a problem hiding this comment.
Ok, I guess this explains that the link to css-ui-4 was done in purpose. 😉
| .unselectable { | ||
| display: block; | ||
| user-select: none; | ||
| -webkit-user-select: none; /* Yes, vendor prefixes are ugly. But this one was grandfathered in and support is required by spec. */ |
There was a problem hiding this comment.
Nit: I usually see vendor prefix before the actual property like:
-webkit-user-select: none;
user-select: none;There was a problem hiding this comment.
Right. In this case, they are (by spec) required to do the same thing, so it doesn't matter much. And making a change will trigger a new round of review, which I'd rather skip.
| <p>Place the cursor into the orange box for a reference of what this should look like.</p> | ||
| <div id=test> | ||
| <!-- text in a button is either not considered text because it's in a replaced element, | ||
| or it is text but it is unselectable, eitherway, we should get the default cursor--> |
There was a problem hiding this comment.
Super-nit: Dunno if there are any guidelines about this, but the rest of the comments in the test are full sentences (starting uppercase and finishing with a dot). Maybe you want to do the same here.
There was a problem hiding this comment.
I'll be careful next time. But I don't think that's worth another round trip through the review system, so the world will have to live with my sub-par capitalization.
This adjusts and completes the test suite, reflecting the changes resolved in w3c/csswg-drafts#1598 (comment)
(Also fixes a nearby test description while we're at it).