-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[CSS4Pseudo] Added 14 new active selection tests and 11 references #20028
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
Conversation
1539131 to
05a67d7
Compare
fantasai
left a comment
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.
20028.diff
active-selection-021
active-selection-022
active-selection-023
These three tests should be merged into one. Reftests are expensive to run, we shouldn't have more of them than is really necessary for understandability.
active-selection-024
active-selection-025
I'm glad you thought to test this :) But again, I would merge these two tests as well.
active-selection-027
Hm, I'm not sure this test is backed up by the spec. We'll need to file an issue on it, basically to clarify specifically that non-tree-abiding pseudo-elements like ::first-line cannot originate a ::selection pseudo-element and therefore a currentColor lookup on ::selection never looks at their styles...
active-selection-031
active-selection-032
active-selection-033
active-selection-034
active-selection-035
active-selection-036
These are all correct tests, but I don't think there's anything very particularly tricky about writing-modes interaction, so having six separate reftests about it seems like a lot. Suggest compressing this down to a single file, maybe by floating them all alongside each other.
active-selection-041
So... the problem wth this test is that we don't actually know the exact color that the UA will use. It could be an 0.5 wash, but it doesn't have to be. We can make what you have as a MAY test, and it will be correct. I would use a blue highlight, though, to make the color purple. This will be more clearly not red than the warm brown that results from mixing the green and red.
You should also add another test for what happens if the highlight has 'color' but not 'background-color'.
Now, we want to also write a test for the full SHOULD, we will need to take a different approach.
Here is my suggestion: write two tests using your square image as the base, as follows:
- For the first test, mask over the image with an inline element that has 4px top+bottom padding and a background. (If the UA highlights only this area, as if it was a regular inline element, it's insufficient. I think this would be the most likely error.) Use a "mismatch" reference against a version of the test without the highlight. This ensures that the UA has highlighted the image somehow, including outside the masked area.
- For the second test, mask over the entire square image. Use a "match" reference against a version of the test without the highlight. This ensures the highlight doesn't leak outside the image area.
Think about this, and let me know if you think it is a good approach or if you have some other ideas. It's a tricky thing to test because we don't know what will be the exact rendering. :)
One last comment on this image highlights topic... the spec currently doesn't allow painting outside the content box. You have some notes about Chrome doing that on inline images, which is interesting. We might need to adjust the spec to allow it, so I've filed issue about that. w3c/csswg-drafts#4624
(However, it should definitely not be allowed to paint outside non-inline replaced elements!)
Thanks again for your tests~
~fantasai
|
(We might want to move active-selection-027 to its own PR, since it's dependent on the CSSWG resolving w3c/csswg-drafts#4625 ) |
|
I changed (or will change) "Text sample" for "Selected Text" in all tests, when and where it makes sense to.
Done: http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-021-ref-new.html
http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-025.html Merging done: http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-024-ref-new.html Now, with this new test, the font-kerning issue is avoided.
This works in Firefox 73: http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-031-new.html but does not work in Chromium 81. [Addendum: this works in Chromium 81 as well: http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-031-newest.html
That is why I added a 'should' flag in the test, to begin with.
Great idea! Agreed. http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-041-new.html http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-041-ref-new.html More later... |
Actually, it is possible to mathematically predict the resulting rendered color in Firefox.
Done:
I have tried to do what you wrote above and I never succeeded. I am not sure I understand your description to begin with.
That second test, on the other hand, I have succeeded: |
I will remove all of the tests, references and support files except Therefore active-selection-027 will have its own PR, which will be this one, 20028. And I will create another separate, distinct PR for the improved tests and references listed in previous comment. |
It is #20931 |
active-selection-021.html
active-selection-021-ref.html
active-selection-022.html
active-selection-022-ref.html
active-selection-023.html
active-selection-023-ref.html
active-selection-024.html
active-selection-025.html
active-selection-025-ref.html
active-selection-026.html
active-selection-026-ref.html
active-selection-027.html
active-selection-027-ref.html
active-selection-031.html
active-selection-031-ref.html
active-selection-032.html
active-selection-032-ref.html
active-selection-033.html
active-selection-033-ref.html
active-selection-034.html
active-selection-035.html
active-selection-036.html
active-selection-036-ref.html
active-selection-041.html
active-selection-041-ref.html
support/60x60-red.png
active-selection-011-ref.html