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

Implement HTMLDetailsElement #9216

Closed
KiChjang opened this issue Jan 9, 2016 · 7 comments
Closed

Implement HTMLDetailsElement #9216

KiChjang opened this issue Jan 9, 2016 · 7 comments

Comments

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 9, 2016

This requires an implementation of the attribute_mutated function in VirtualMethods for HTMLDetailsElement, when attempting to implement the details notification task steps. It also requires the usage of the DOM manipulation task source.

Code: components/script/dom/htmldetailselement.rs (new file), components/script/dom/webidls/HTMLDetailsElement.webidl (new file)
Tests: tests/wpt/web-platform-tests/html/semantics/interactive-elements/the-details-element/details.html
Spec: https://html.spec.whatwg.org/multipage/#htmldetailselement

Requires #9217.

@MonsieurLanza
Copy link
Contributor

@MonsieurLanza MonsieurLanza commented Jan 12, 2016

This one is appealing me. May I give it a try ?

I understand #9217 is blocking it, but I expect it to be reviewed before I even get rid of

servo/components/script/dom/virtualmethods.rs:20:5: 20:48 error: unresolved import `dom::htmldetailselement::HTMLDetailsElement`. Could not find `htmldetailselement` in `dom` [E0432]

while trying to build.
;)

@jdm jdm added the C-assigned label Jan 12, 2016
@jdm
Copy link
Member

@jdm jdm commented Jan 12, 2016

I think it's possible to work on this without waiting for #9217. Either this work will need to be rebased after it merges, or #9217 will need to be updated to account for this. Either way, I would say go for it :)

@jdm
Copy link
Member

@jdm jdm commented Jan 12, 2016

As for that error, take a look at components/script/dom/mod.rs :)

@nox
Copy link
Member

@nox nox commented Jan 12, 2016

This module screams "have mercy and codegen the hell out of myself" to me.

@MonsieurLanza
Copy link
Contributor

@MonsieurLanza MonsieurLanza commented Jan 13, 2016

Thanks. :)

@MonsieurLanza
Copy link
Contributor

@MonsieurLanza MonsieurLanza commented Jan 18, 2016

The details element should hide non "first-child

" children.

I was wondering how this is supposed to be implemented, and if it is in the scope of the issue ?
My understanding is that could be done in user-agent.css, as this would allow overriding by a user stylesheet. Not sure what to do about this.

Also, user agent should provide a way to toggle the details element, but this is a design decision and goes way beyond just implementing the HTMLDetailsElement interface. ATM, on my working copy, the only way is setting the open attribute via JS.

@jdm
Copy link
Member

@jdm jdm commented Jan 18, 2016

Yes, we should file a separate issue about exposing a UI for <details>. As for hiding the children, doing that via CSS makes sense to me.

MonsieurLanza added a commit to MonsieurLanza/servo that referenced this issue Jan 19, 2016
MonsieurLanza added a commit to MonsieurLanza/servo that referenced this issue Jan 19, 2016
MonsieurLanza added a commit to MonsieurLanza/servo that referenced this issue Jan 19, 2016
bors-servo added a commit that referenced this issue Jan 19, 2016
Implement HTMLDetailsElement. Fixes #9216

Implement the interface HTMLDetailsElement ( // https://html.spec.whatwg.org/multipage/#htmldetailselement )

All tests pass in
tests/wpt/web-platform-tests/html/semantics/interactive-elements/the-details-element/details.html
&
tests/wpt/web-platform-tests/html/semantics/interactive-elements/the-details-element/toggleEvent.html

Anyway, no change is made on layout and attribute open currently has no effect, it just fires a toggle event.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9381)
<!-- Reviewable:end -->
MonsieurLanza added a commit to MonsieurLanza/servo that referenced this issue Jan 20, 2016
bors-servo added a commit that referenced this issue Jan 20, 2016
Implement HTMLDetailsElement. Fixes #9216

Implement the interface HTMLDetailsElement ( // https://html.spec.whatwg.org/multipage/#htmldetailselement )

All tests pass in
tests/wpt/web-platform-tests/html/semantics/interactive-elements/the-details-element/details.html
&
tests/wpt/web-platform-tests/html/semantics/interactive-elements/the-details-element/toggleEvent.html

Anyway, no change is made on layout and attribute open currently has no effect, it just fires a toggle event.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9381)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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