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

Split the big test() up into multiple small ones #27745

Merged
merged 1 commit into from Feb 25, 2021

Conversation

@cbiesinger
Copy link
Contributor

@cbiesinger cbiesinger commented Feb 23, 2021

Also test the computed style on a with a loaded image.

Addresses the review comments in
#27745

@wpt-pr-bot wpt-pr-bot requested review from annevk, domenic, foolip, jdm, jgraham and zqzhang Feb 23, 2021
@cbiesinger cbiesinger force-pushed the cbiesinger:source branch 2 times, most recently from a944d88 to 33ebbc4 Feb 23, 2021
Copy link
Member

@domenic domenic left a comment

Could you add getComputedStyle().width/height tests for both the img and the source elements, in all cases? I'm not even sure what the result should be, per https://github.com/whatwg/html/pull/5894/files#r582084336 , but probably you are...

@cbiesinger
Copy link
Contributor Author

@cbiesinger cbiesinger commented Feb 24, 2021

Nothing should get mapped to <source>'s style. I will add tests to ensure that. I can add some more tests for img's width/height mapping, but testing this is annoying since it only works on display:none.

BTW, I am planning to land the blink implementation patch tonight CET together with the test to ensure that it makes it into the branch, but I will certainly make sure to address any further comments afterwards.

@cbiesinger cbiesinger force-pushed the cbiesinger:source branch from 33ebbc4 to 6e43734 Feb 24, 2021
@cbiesinger
Copy link
Contributor Author

@cbiesinger cbiesinger commented Feb 24, 2021

@domenic I added tests for that now.

Copy link
Member

@domenic domenic left a comment

Tests LGTM. I would ideally like another set of eyes, such as @emilio or @annevk or @yoavweiss.

@cbiesinger
Copy link
Contributor Author

@cbiesinger cbiesinger commented Feb 24, 2021

Thank you!

I am submitting them through the Chrome CQ at https://crrev.com/c/2642939 to make merging simpler. I will continue to address any comments even after landing, of course.

Copy link
Member

@annevk annevk left a comment

I have two concerns:

  1. Many seemingly unrelated tests are grouped together in a single test. It seems that each time createPicture() is used a new test wrapper could also be used to keep the assertions somewhat isolated.
  2. The setup doesn't wait for the images to be loaded, so all the style tests are about img elements without images, as far as I can tell.
@cbiesinger
Copy link
Contributor Author

@cbiesinger cbiesinger commented Feb 25, 2021

I have two concerns:

  1. Many seemingly unrelated tests are grouped together in a single test. It seems that each time createPicture() is used a new test wrapper could also be used to keep the assertions somewhat isolated.

Thanks, good point, I will fix this.

  1. The setup doesn't wait for the images to be loaded, so all the style tests are about img elements without images, as far as I can tell.

Yes. I don't think this should matter for computed style, and the ones where I also assert_ratio will not work once the image is loaded because once an image is loaded its actual dimensions will override the aspect-ratio. However, I can add some computed style checks with a loaded image.

Also test the computed style on a <picture> with a loaded image.

Addresses the review comments in
#27745
@cbiesinger cbiesinger force-pushed the cbiesinger:source branch from 6e43734 to 8820cf1 Feb 25, 2021
@cbiesinger
Copy link
Contributor Author

@cbiesinger cbiesinger commented Feb 25, 2021

@annevk -- I've made the changes in this PR, please take a look

@annevk
annevk approved these changes Feb 25, 2021
Copy link
Member

@annevk annevk left a comment

Thanks!

@cbiesinger cbiesinger changed the title Add test for <source> width/height Split the big test() up into multiple small ones Feb 25, 2021
@cbiesinger cbiesinger merged commit 945659f into web-platform-tests:master Feb 25, 2021
21 checks passed
21 checks passed
update-pr-preview
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
Azure Pipelines Build #20210225.44 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
download-firefox-nightly Community-TC (pull_request)
Details
lint Community-TC (pull_request)
Details
sink-task Community-TC (pull_request)
Details
update-built Community-TC (pull_request)
Details
wpt-chrome-dev-results Community-TC (pull_request)
Details
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
wpt-chrome-dev-stability Community-TC (pull_request)
Details
wpt-decision-task Community-TC (pull_request)
Details
wpt-firefox-nightly-results Community-TC (pull_request)
Details
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
@cbiesinger cbiesinger deleted the cbiesinger:source branch Feb 25, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 15, 2021
…all ones, a=testonly

Automatic update from web-platform-tests
Split the big test() up into multiple small ones

Also test the computed style on a <picture> with a loaded image.

Addresses the review comments in
web-platform-tests/wpt#27745

--

wpt-commits: 945659ffe92b58c711d68e9c15aa047751dfb89e
wpt-pr: 27745
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Mar 15, 2021
…all ones, a=testonly

Automatic update from web-platform-tests
Split the big test() up into multiple small ones

Also test the computed style on a <picture> with a loaded image.

Addresses the review comments in
web-platform-tests/wpt#27745

--

wpt-commits: 945659ffe92b58c711d68e9c15aa047751dfb89e
wpt-pr: 27745

UltraBlame original commit: 19dce124c96becbadde1e2318d3ceae1ad435e1d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Mar 15, 2021
…all ones, a=testonly

Automatic update from web-platform-tests
Split the big test() up into multiple small ones

Also test the computed style on a <picture> with a loaded image.

Addresses the review comments in
web-platform-tests/wpt#27745

--

wpt-commits: 945659ffe92b58c711d68e9c15aa047751dfb89e
wpt-pr: 27745

UltraBlame original commit: 19dce124c96becbadde1e2318d3ceae1ad435e1d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Mar 15, 2021
…all ones, a=testonly

Automatic update from web-platform-tests
Split the big test() up into multiple small ones

Also test the computed style on a <picture> with a loaded image.

Addresses the review comments in
web-platform-tests/wpt#27745

--

wpt-commits: 945659ffe92b58c711d68e9c15aa047751dfb89e
wpt-pr: 27745

UltraBlame original commit: 19dce124c96becbadde1e2318d3ceae1ad435e1d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants