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 test for document wg color #13842

Closed
wants to merge 4 commits into from

Conversation

akansha2608
Copy link
Contributor

@akansha2608 akansha2608 commented Nov 1, 2018

Fixes #2391

Copy link
Contributor

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Couple of nits, but great start.

<link rel="help" href="https://html.spec.whatwg.org/multipage/obsolete.html">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<div id="log"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I don’t think you need this div. The test harness adds it automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, also, I think adding the div also creates a body automagically.

@akansha2608 akansha2608 changed the title [WIP] Add test for document wg color Add test for document wg color Nov 2, 2018
Copy link
Contributor

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Almost there! These are starting to look good.

<script>
test(function() {
assert_equals(document.fgColor, document.body.text);

Copy link
Contributor

Choose a reason for hiding this comment

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

I this test, let’s confirm that each of these is “red”.
assert_equal(document.fgColor, “red”);

And so on...


assert_equals(document.bgColor, "yellow");
}, "document's bgColor IDL attribute reflects the body's bgColor content attribute");
</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need one more set of tests, where we finally remove the attributes from body to make sure that they get reflected properly in document. So:

test(function(){
   document.body.removeAttribute("test");
   assert_equals(document.fgColor, "");
   // and so on...
}, "Removing the attributes causes them to be reflected as the empty string");

Copy link
Member

Choose a reason for hiding this comment

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

Aren't all these reflection tests already done in an automated fashion by https://github.com/web-platform-tests/wpt/blob/master/html/dom/elements-sections.js ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed... we don't need these tests :) @akansha2608 was learning how to read/process spec text.

So, I think we should abandon this and do the "TODO" in https://github.com/web-platform-tests/wpt/blob/master/html/dom/elements-sections.js

Copy link
Contributor

Choose a reason for hiding this comment

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

So yeah, looking at the above, we could basically run everything above in a frameset and on a new body.

@akansha2608, ping me on Slack on Monday and I’ll help you with it. In the meantime, read over the original bug to get a more clear sense of exactly what we need to test.

@akansha2608
Copy link
Contributor Author

Closing it as its already in https://github.com/web-platform-tests/wpt/blob/master/html/dom/elements-sections.js :)

Thanks for everyone to helping me 😄

@akansha2608 akansha2608 closed this Nov 7, 2018
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.

5 participants