Skip to content

Conversation

@TalbotG
Copy link
Contributor

@TalbotG TalbotG commented Oct 31, 2019

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

@wpt-pr-bot wpt-pr-bot requested review from frivoal and plinss October 31, 2019 18:01
@frivoal frivoal requested a review from fantasai December 3, 2019 05:03
@TalbotG TalbotG changed the title CSS4Pseudo-GT-PR7 [CSS4Pseudo] Added 14 new active selection tests and 11 references Dec 17, 2019
Copy link
Contributor

@fantasai fantasai left a 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:

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

@fantasai
Copy link
Contributor

(We might want to move active-selection-027 to its own PR, since it's dependent on the CSSWG resolving w3c/csswg-drafts#4625 )

@TalbotG
Copy link
Contributor Author

TalbotG commented Dec 23, 2019

I changed (or will change) "Text sample" for "Selected Text" in all tests, when and where it makes sense to.

active-selection-021
active-selection-022
active-selection-023

These three tests should be merged into one. (...)

Done:
http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-021-new.html

http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-021-ref-new.html

active-selection-024
active-selection-025

(...) I would merge these two tests as well.

http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-025.html
was failed by Firefox 73 because of what appears to be a font-kerning issue on the righthand side of the "T". Anyway...

Merging done:
http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-024-new.html

http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-024-ref-new.html

Now, with this new test, the font-kerning issue is avoided.

active-selection-031
active-selection-032
active-selection-033
active-selection-034
active-selection-035
active-selection-036

(...) Suggest compressing this down to a single file, maybe by floating them all alongside each other.

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
]

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.

That is why I added a 'should' flag in the test, to begin with.

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.

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

@TalbotG
Copy link
Contributor Author

TalbotG commented Dec 25, 2019

active-selection-041

the problem wth this test is that we don't actually know the exact color that the UA will use.

Actually, it is possible to mathematically predict the resulting rendered color in Firefox.
You add the red, green and blue respective values (hexadecimal or percentage) of the image to the red, green and blue values (hexadecimal or percentage) of the overlay, then divide the total by 2 and you get the resulting rendered color. This is definitely testable.
Eg http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-041.html
image is red: #FF0000 or rgb(100%, 0%, 0%)
overlay is green: #008000 or rgb(0%, 50%, 0%)
Resulting rendered color will be in Firefox:
#804000 or rgb(50%, 25%, 0%)
which is what
http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-041-2-ref.html
demonstrates.

You should also add another test for what happens if the highlight has 'color' but not 'background-color'.

Done:
http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-043-new.html

write two tests using your square image as the base, as follows:

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

I have tried to do what you wrote above and I never succeeded. I am not sure I understand your description to begin with.

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

That second test, on the other hand, I have succeeded:
http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/active-selection-045-new.html
There may be other ways to test this but this test is the best and most convincing one I could create.

@TalbotG
Copy link
Contributor Author

TalbotG commented Dec 28, 2019

(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 will remove all of the tests, references and support files except
active-selection-027.html
active-selection-027-ref.html

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.

@TalbotG
Copy link
Contributor Author

TalbotG commented Dec 28, 2019

another separate, distinct PR for the improved tests and references listed in previous comment

It is #20931

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants