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

Add sectioning tests. #1538

Closed
wants to merge 12 commits into from
Closed

Add sectioning tests. #1538

wants to merge 12 commits into from

Conversation

Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jan 15, 2015

No description provided.

@tobie tobie added the html label Jan 15, 2015
@Ms2ger Ms2ger mentioned this pull request Jan 15, 2015
@ayg ayg closed this Aug 28, 2016
@ayg ayg deleted the sectioning branch August 28, 2016 13:58
@gsnedders gsnedders restored the sectioning branch August 28, 2016 20:20
@gsnedders gsnedders reopened this Aug 28, 2016
@ayg ayg closed this Oct 26, 2016
@ayg ayg deleted the sectioning branch October 26, 2016 17:37
@Ms2ger Ms2ger restored the sectioning branch October 27, 2016 09:20
@Ms2ger Ms2ger reopened this Oct 27, 2016
Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

@Ms2ger, if this is rebased, is it still worth reviewing, or have things changed so much since this PR was submitted that it's better to abandon?

onerror = "foobar = 4;"
onafterprint = "foobar = 5;"
onbeforeprint = "foobar = 6;"
onbeforeunload = "foobar = 7;""
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's a syntax error here.

@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 1, 2018

I suspect they're all still valid and as useful as they were when first submitted.

@foolip
Copy link
Member

foolip commented Oct 1, 2018

@Ms2ger can you rebase to retrigger Travis+Taskcluster. I can review.

@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 2, 2018

CI passes now.

assert_regexp_match(strCheck, regEx, "document.body." + strAttr + ":");

} else {
assert_equals(document.body[strAttr], funcExpected, "document.body." + strAttr + ":");
Copy link
Member

Choose a reason for hiding this comment

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

Redundant with tests in html/webappapis/scripting/events.
<meta charset="utf-8">
<title>Body Element</title>
<link rel="author" title="dzenana" href="mailto:dzenana.trenutak@gmail.com">
<link rel="help" href="http://www.w3.org/html/wg/drafts/html/CR/single-page.html#the-body-element">
Copy link
Member

Choose a reason for hiding this comment

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

The spec for this and part2 is the places that reference https://html.spec.whatwg.org/#determining-the-target-of-an-event-handler. The model is that setting/getting the content and IDL attribute redirects to the window object, as opposed to the event target order being tweaked during event dispatch.

part1 and part2 all pass on Chrome, Edge, Firefox and Safari, except for part1 on Edge. I'm pretty sure that's covered by the Edge failures in https://wpt.fyi/results/html/webappapis/scripting/events/event-handler-attributes-body-window.html?label=stable&aligned=true.

With this covered in html/webappapis/scripting/events/, I'll delete these too.

@foolip
Copy link
Member

foolip commented Oct 2, 2018

Alright, so a pretty clear pattern is emerging here. The tests are all about event handlers on the body element, quite verbose and slow to run. For the cases I've looked closely at I've concluded that the exists tests are better.

I've tried running most of the tests in all browsers and while I've spotted some differences, those too seem to be covered by existing tests. So, I don't think there's value in anyone studying in detail if there is some delta of test coverage provided by these tests but not existing ones. I'll just close the PR, but if anyone feels differently please reopen and review away :)

@foolip foolip closed this Oct 2, 2018
@foolip foolip deleted the sectioning branch October 2, 2018 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants