Skip to content
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

Added 7 new CSS4Pseudo tests and 3 references #20030

Merged
merged 4 commits into from May 5, 2020

Conversation

TalbotG
Copy link
Contributor

@TalbotG TalbotG commented Oct 31, 2019

cascade-highlight-001.html
cascade-highlight-001-ref.html
cascade-highlight-002.html
cascade-highlight-004.html
cascade-highlight-004-ref.html
grammar-error-002-manual.html
grammar-error-003-manual.html
highlight-z-index-001.html
highlight-z-index-001-ref.html
highlight-z-index-002.html

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.

The rest of the tests look correct, however I think the z-index tests need some changes:

  • The descendant comes before the selected text (makes the test stronger, since usually text after paints over anyway).
  • The descendant's text is not highlighted. (Currently you are highlighting the entire contents of #test, including the positioned descendant.)

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.

Well, now you say you're testing a positioned descendant, but you don't have a descendant. :) I'm not sure which is better, to have a descendant or to have a previous sibling, but either way the comment and the code should match.

I might also suggest combining both tests into one file. They're very closely related, almost all the code is duplicated, so I think it will be easier to understand if they are in one file.

@TalbotG
Copy link
Contributor Author

TalbotG commented Feb 22, 2020

Well, now you say you're testing a positioned descendant, but you don't have a descendant. :) I'm not sure which is better, to have a descendant or to have a previous sibling, but either way the comment and the code should match.

The statement that I was trying to test is:

Each highlight pseudo-element draws its background over the corresponding portion of the highlight overlay, painting it immediately below any positioned descendants

coming from § 3.5 Painting the Highlight

At first, such statement was not (to me) perfectly explicit or clear since a descendant could refer to the body element (as the referring ancestor) or to the highlighted element itself (as the referring ancestor). Anyway... So, I created 2 tests (z-index-001 involving 'position: relative' and z-index-002 involving 'position: absolute'). And now, I have tuned and tweaked accordingly the css identifiers and the meta assert text of those 2 tests at my website:

http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/highlight-z-index-001-new.html

http://www.gtalbot.org/BrowserBugsSection/CSS4Pseudo/highlight-z-index-002-new.html

I might also suggest combining both tests into one file. They're very closely related, almost all the code is duplicated, so I think it will be easier to understand if they are in one file.

I think it would be difficult and not best to combine both tests (abs pos and rel pos) into 1 ... but it's doable. I prefer not to combine both tests into 1.

In a few min., I will try to make the changes into
highlight-z-index-001.html
highlight-z-index-002.html
in this PR. If it fails because of Community-TC integration check and the need to rebase, then I will just close this PR and create a new PR. So, stay tuned ...

@TalbotG
Copy link
Contributor Author

TalbotG commented Feb 22, 2020

@fantasai , the modifications I did to the PR went as hoped. So, please review the z-index tests one last time. Thank you!

@fantasai fantasai merged commit 3adb0f8 into web-platform-tests:master May 5, 2020
@TalbotG TalbotG deleted the CSS4Pseudo-GT-PR8 branch May 5, 2020 16:20
@TalbotG TalbotG removed the request for review from plinss May 5, 2020 19:34
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.

None yet

4 participants