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

update wpt #2951

Merged
merged 2 commits into from Jul 4, 2020
Merged

update wpt #2951

merged 2 commits into from Jul 4, 2020

Conversation

zjffun
Copy link
Contributor

@zjffun zjffun commented Apr 20, 2020

  • remove assert_throws
  • fix connect ECONNREFUSED
  • update to-run.yaml
  • fix or triage TempUnknown in to-run.yaml

@zjffun zjffun force-pushed the rm-assert-throw branch 3 times, most recently from f906e93 to b645484 Compare April 25, 2020 12:30
@zjffun zjffun changed the title [WIP] update wpt update wpt Apr 25, 2020
@zjffun
Copy link
Contributor Author

zjffun commented Apr 25, 2020

I write a script to auto update to-run.yaml, but not sure if this project need. Array of yaml can fold and unfold, this's very convenient, and sort tests can be done automatically.

This update bring about one hundred fail, I only found out a few reasons of them.

@domenic
Copy link
Member

domenic commented Apr 25, 2020

Please revert the dramatic formatting changes to to-run.yml! The file is intended to be a flat list.

@domenic
Copy link
Member

domenic commented Apr 25, 2020

I pushed a commit which undoes the dramatic reformatting of to-run.yaml and goes through and updates it for a few cases. It still needs more updates, but hopefully you, or another project contributor, can do that without destroying the readability and editability of the file.

@zjffun
Copy link
Contributor Author

zjffun commented Apr 27, 2020

@TimothyGu Sorry, I push this file by accident thank you for reminding.

@domenic
Copy link
Member

domenic commented May 1, 2020

Thanks very much for all your hard work here @zjffun. It looks like you're almost done. Let us know if we can help.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could mark this as fixing #2935, that would be super helpful.

Copy link
Contributor Author

@zjffun zjffun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood before, not all elements has activation behavior are interactive content. like HTMLAreaElement

@zjffun
Copy link
Contributor Author

zjffun commented May 2, 2020

My pleasure. There is a question need help :

I write a small script, it's not good and only work in current to-run-yaml construction. I wender if this script needed, or change to using yaml AST to generate to-run-yaml (current used js-yaml can't support this, may need import an other package).
https://github.com/zjffun/jsdom/blob/rm-assert-throw/test/web-platform-tests/tools/update-to-run.js

lib/jsdom/living/nodes/HTMLImageElement.webidl Outdated Show resolved Hide resolved
lib/jsdom/living/helpers/cors-settings.js Outdated Show resolved Hide resolved
lib/jsdom/living/nodes/HTMLInputElement-impl.js Outdated Show resolved Hide resolved
lib/jsdom/living/nodes/HTMLAnchorElement-impl.js Outdated Show resolved Hide resolved
lib/jsdom/living/nodes/HTMLAreaElement-impl.js Outdated Show resolved Hide resolved
lib/jsdom/living/nodes/HTMLButtonElement-impl.js Outdated Show resolved Hide resolved
lib/jsdom/living/nodes/HTMLElement-impl.js Outdated Show resolved Hide resolved
@@ -37,10 +39,11 @@ class HTMLElementImpl extends ElementImpl {
// https://html.spec.whatwg.org/multipage/dom.html#the-translate-attribute
get translate() {
const translateAttr = this.getAttributeNS(null, "translate");
const translateAttrString = asciiLowercase(translateAttr || "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about an early return here if (translateAttr === null). This way we can avoid the awkward check below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems want to do the things in the table below. I can't think a better implementation. @domenic

translateAttr null '' 'yes' 'no' 'gibberish'
asciiLowercase '' '' 'yes' 'no' 'gibberish'
return the logic behind true true false the logic behind

lib/jsdom/living/nodes/HTMLLabelElement-impl.js Outdated Show resolved Hide resolved
@zjffun
Copy link
Contributor Author

zjffun commented May 4, 2020

It's weird that tabindex-focus-flag.html was taged fail in to-run.yaml https://github.com/zjffun/jsdom/blob/rm-assert-throw/test/web-platform-tests/to-run.yaml#L488, but failing in tarvis at window.add_completion_callback (test/web-platform-tests/run-single-wpt.js:197:22). And I can't reporduct it on my computer.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for all your work. I'd be happy to take this forward and fix the rest of the issues, which are primarily stylistic.

}

set _hasActivationBehavior(v) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the setter necessary?

Copy link
Contributor Author

@zjffun zjffun May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove those two setters will cause error, becauce the parent class HTMLElement need set _hasActivationBehavior and code within the class body's syntactic boundary is always executed in strict mode.
https://github.com/zjffun/jsdom/blob/rm-assert-throw/lib/jsdom/living/nodes/HTMLElement-impl.js#L23

}

set _hasActivationBehavior(v) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the setter necessary?


function resolveWpt(...paths) {
return path.resolve(__dirname, "../", ...paths);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks somewhat useful, but it doesn't look like the result is being used in this PR. Is my understanding correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dealing with all the TempUnknown and delete a few lines of this script's result is the current to-run.yaml.
Emm.. This script can get all fail tests that not in to-run.yaml, and add them in the right place of to-run.yaml with TempUnknown as reason.

lib/jsdom/living/helpers/cors-settings.js Outdated Show resolved Hide resolved
scripts/webidl/convert.js Outdated Show resolved Hide resolved
@TimothyGu
Copy link
Member

It's weird that tabindex-focus-flag.html was taged fail in to-run.yaml https://github.com/zjffun/jsdom/blob/rm-assert-throw/test/web-platform-tests/to-run.yaml#L488, but failing in tarvis at window.add_completion_callback (test/web-platform-tests/run-single-wpt.js:197:22). And I can't reporduct it on my computer.

The tabindex-focus-flag.html test was fixed in be0059f. Could you try rebasing your branch onto master?

@zjffun zjffun force-pushed the rm-assert-throw branch 4 times, most recently from 72e688b to 1eaf549 Compare May 8, 2020 11:07
@@ -476,12 +485,14 @@ sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-negative.h
sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-order.html: [fail, scrollIntoView() not implemented]
sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-positive.html: [fail, scrollIntoView() not implemented]
sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-zero.html: [fail, scrollIntoView() not implemented]
tabindex-focus-flag.html: [fail, https://github.com/jsdom/jsdom/pull/2965]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zjffun zjffun force-pushed the rm-assert-throw branch 2 times, most recently from 8e26a1a to 19d4b23 Compare June 21, 2020 16:20
@zjffun
Copy link
Contributor Author

zjffun commented Jun 21, 2020

Thanks everyone for the review. I reorganized commits of this pr, if there is anything else I need to do please @ me.

@zjffun
Copy link
Contributor Author

zjffun commented Jul 4, 2020

I self reviewed this PR and found that it was messy and had some errors, so I split them into separate branches. This PR only keeps the changes of update wpt.

In this PR the merged test cases are deleted, and the test under the html/canvas folder is set to require canvas.

Some failed tests cannot be expressed using wildcards, because some of them will pass even if they are not implemented. Like elementFromPoint-parameters.html and image-loading-eager.html.

Some fixes for the new test cases:

@domenic
Copy link
Member

domenic commented Jul 4, 2020

This is amazing; thank you very much. I appreciate the splitting; that definitely will make things easier. I'll get this merged now and then we can proceed with the other PRs and branches.

@domenic
Copy link
Member

domenic commented Jul 4, 2020

I noticed a few of the deleted files are not landed in web platform tests upstream. I will un-delete them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants