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 the table section IDL attributes for HTML tables #8745

Closed
wants to merge 1 commit into from

Conversation

@dzbarsky
Copy link
Member

dzbarsky commented Nov 30, 2015

Rebasing this old PR. Still need to write tests but at least it compiles.

Review on Reviewable

@eefriedman eefriedman self-assigned this Nov 30, 2015
@eefriedman
Copy link
Contributor

eefriedman commented Nov 30, 2015

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.


components/script/dom/htmltableelement.rs, line 102 [r1] (raw file):
This can legitimately fail: if the table is nested inside the thead, this will throw a HierarchyRequestError.

Not sure if we're supposed to do that check before or after removing the existing thead.


components/script/dom/htmltableelement.rs, line 116 [r1] (raw file):
I'm not sure what the recommended way to do Atom->DOMString conversion is at the moment; it's been fluctuating a bit. @asajeffrey ?


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jan 12, 2016

The latest upstream changes (presumably #9056) made this pull request unmergeable. Please resolve the merge conflicts.

@KiChjang
Copy link
Member

KiChjang commented Jan 22, 2016

What's the status of this?

@dzbarsky
Copy link
Member Author

dzbarsky commented Jan 22, 2016

The status is that it needs to address review comments and add tests but I haven't had time to work on it =/ If someone feels a desire to pick this up, I would appreciate it.


Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Feb 26, 2016

Closed due to lack of updates, and filed #9769 to keep track of this. Thanks @dzbarsky!

@jdm jdm closed this Feb 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.