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

el.toggleAttribute("style") after assigning to el.style returns true instead of false #25130

Closed
pshaughn opened this issue Dec 5, 2019 · 4 comments

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 5, 2019

WPT dom/nodes/attributes.html tests toggleAttribute in many situations, most of which pass, but "Toggling element with inline style should make inline style disappear" fails. It's actually just testing what toggleAttribute returns and not strictly testing whether the style has disappeared or not, but the return value of toggleAttribute should definitely be false here. Special things I can see here to distinguish this case from passing ones are that this test is using .style= instead of setAttribute, and that style might be the only [PutForward] attribute in the entire test.

@jdm jdm added the A-content/dom label Dec 5, 2019
@jdm jdm added this to To do in web-platform-test failures via automation Dec 5, 2019
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 12, 2019

When we .style= in
https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/dom/nodes/attributes.html#L142
we see None returned by GetAttribute in
https://github.com/servo/servo/blob/master/components/script/dom/element.rs#L1906

If we setAttribute("style") instead of .style=, then the test passes as written, but the type returned by GetAttribute is DOMSTring, not CSSStyleDeclaration; it's like the content attribute isn't connected to either the IDL type or to the Javascript object attribute.

Setting el.style as it is in the test, and then reading back el.style from Javascript, gets a CSSStyleDeclaration, so the IDL attribute is there on the Javascript DOM element, but seemingly entirely separately from get/set/toggleAttribute.

@jdm
Copy link
Member

@jdm jdm commented Dec 13, 2019

Oh!

[SameObject/*, PutForwards=cssText*/] readonly attribute CSSStyleDeclaration style;
has the PutForwards commented out and the attribute is readonly, so there is no code that handles setting the style IDL property right now. Setting the content attribute works because of this code.

@jdm
Copy link
Member

@jdm jdm commented Dec 13, 2019

I am astounded that we apparently haven't had a working style setter in 7 years.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 27, 2020

Fixed by #25450

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.