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 'labels' attribute on 'labelable elements' #7965

Merged

Conversation

frewsxcv
Copy link
Contributor

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 11, 2015
@frewsxcv frewsxcv force-pushed the labelable-elements-label-attribute branch 2 times, most recently from 1028cc2 to a8e512e Compare October 11, 2015 02:17
@jdm
Copy link
Member

jdm commented Oct 13, 2015

@nox could you review this?

@nox nox self-assigned this Oct 13, 2015
@nox
Copy link
Contributor

nox commented Oct 13, 2015

-S-awaiting-review +S-awaiting-answer +S-needs-code-changes


Reviewed 20 of 20 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


components/script/dom/element.rs, line 1862 [r1] (raw file):
Elements' IDs are stored as AttrValue::Atom values, we could use it if we use an atom for the label attribute on HTMLLabelElement. Either do it in that PR or file a follow-up issue.


components/script/dom/element.rs, line 1875 [r1] (raw file):
You didn't include that rule:

If the for attribute is not specified, but the label element has a labelable element descendant, then the first such descendant in tree order is the label element's labeled control.


components/script/dom/element.rs, line 1855 [r2] (raw file):
I'm not convinced by the LabelableElement trait, wouldn't it be simpler to define labels() on Element like how ParentNode's methods are defined on Node as helpers and called from the actual interfaces that implement them through an upcast

If traits are the way to go, we should file a follow-up to rewrite ParentNode and friends as traits.

Cc. @jdm @Ms2ger


Comments from the review on Reviewable.io

@nox nox added S-awaiting-answer Someone asked a question that requires an answer. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 13, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 13, 2015

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


components/script/dom/element.rs, line 1855 [r2] (raw file):
I don't see a need for the trait either.


Comments from the review on Reviewable.io

@nox nox added A-content/dom Interacting with the DOM from web content and removed S-awaiting-answer Someone asked a question that requires an answer. labels Oct 14, 2015
@bors-servo
Copy link
Contributor

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 14, 2015
@frewsxcv
Copy link
Contributor Author

components/script/dom/element.rs, line 1855 [r2] (raw file):
Aww I thought I was being clever with it :P

I'll definitely change it, but is there any reason not to? Extra unnecessary complexity? Seems like an easy way to isolate methods to certain Element types


Comments from the review on Reviewable.io

@frewsxcv frewsxcv force-pushed the labelable-elements-label-attribute branch from a8e512e to dbfa850 Compare October 24, 2015 02:03
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 24, 2015
@frewsxcv frewsxcv force-pushed the labelable-elements-label-attribute branch 2 times, most recently from fe77572 to 4b45f97 Compare October 24, 2015 02:40
@highfive highfive removed the S-needs-rebase There are merge conflict errors. label Oct 24, 2015
@frewsxcv
Copy link
Contributor Author

Addressed all the comments. Let me know how this looks.

@frewsxcv
Copy link
Contributor Author

I just noticed that some of this code will overlap with a couple of the methods on <label> elements. I'll plan on implementing those after this merges and making sure it doesn't get repeated.

https://html.spec.whatwg.org/multipage/forms.html#htmllabelelement

@nox
Copy link
Contributor

nox commented Oct 24, 2015

-S-awaiting-review +S-needs-code-changes +S-awaiting-answer


Reviewed 12 of 12 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


components/script/dom/htmlelement.rs, line 321 [r3] (raw file):
Please match against the type ID as usual.


components/script/dom/htmlelement.rs, line 347 [r3] (raw file):
Can you explain to me this part of the code like I am 5? I can't make sense out of it. :/


components/script/dom/htmlinputelement.rs, line 344 [r3] (raw file):
There is no need to upcast when calling window_from_node.


Comments from the review on Reviewable.io

@nox nox added S-awaiting-answer Someone asked a question that requires an answer. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 24, 2015
@frewsxcv
Copy link
Contributor Author

components/script/dom/htmlelement.rs, line 347 [r3] (raw file):
I'm going to pause on this PR briefly and implement <label> control attribute because it might clear this up


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

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

@frewsxcv
Copy link
Contributor Author

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 0e8fd65 with merge e59c184...

bors-servo pushed a commit that referenced this pull request Oct 31, 2015
…<try>

Implement 'labels' attribute on 'labelable elements'



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7965)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 31, 2015
@bors-servo
Copy link
Contributor

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 31, 2015
@frewsxcv frewsxcv force-pushed the labelable-elements-label-attribute branch from 0e8fd65 to 4de1169 Compare October 31, 2015 14:25
@frewsxcv
Copy link
Contributor Author

Rebased

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Oct 31, 2015
@frewsxcv frewsxcv removed the S-needs-rebase There are merge conflict errors. label Oct 31, 2015
@nox nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 1, 2015
@nox
Copy link
Contributor

nox commented Nov 1, 2015

Just one nit and we are done.

-S-awaiting-review +S-needs-code-changes


Reviewed 8 of 8 files at r4, 8 of 8 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


components/script/dom/htmlinputelement.rs, line 344 [r3] (raw file):
Please change this.


Comments from the review on Reviewable.io

@frewsxcv frewsxcv force-pushed the labelable-elements-label-attribute branch from 4de1169 to 9df3751 Compare November 1, 2015 14:50
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 1, 2015
@frewsxcv
Copy link
Contributor Author

frewsxcv commented Nov 1, 2015

Comment has been addressed

@nox
Copy link
Contributor

nox commented Nov 1, 2015

@bors-servo r+


Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

📌 Commit 9df3751 has been approved by nox

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 1, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 9df3751 with merge 0e70a1f...

bors-servo pushed a commit that referenced this pull request Nov 1, 2015
Implement 'labels' attribute on 'labelable elements'



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7965)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 9df3751 into servo:master Nov 1, 2015
@frewsxcv frewsxcv deleted the labelable-elements-label-attribute branch November 1, 2015 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/dom Interacting with the DOM from web content S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants