Implement indexed access on select elements #13066

mskrzypkows · 2016-08-26T23:19:58Z

Added methods for indexed access on select: SetLength, Length, Item, IndexedGetter

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11763 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-08-26T23:20:01Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmlselectelement.rs, components/script/dom/webidls/HTMLSelectElement.webidl

jdm · 2016-08-26T23:41:49Z

r? @KiChjang

KiChjang · 2016-08-26T23:44:48Z

components/script/dom/htmlselectelement.rs

+                }
+            }
+        } else if value > length {  // add new blank option elements
+            let document = node.GetOwnerDocument();


document_from_node(self)

KiChjang · 2016-08-26T23:47:41Z

components/script/dom/htmlselectelement.rs

+                    let appended = node.AppendChild(element.upcast());
+                    match appended {
+                        Ok(_) => {},
+                        Err(e) => println!("error appending child of HTML Select: {:?}", e),


if let Err(e) = node.AppendChild(element.upcast()) { println!("error appending child of HTMLSelectElement: {:?}", e); }

KiChjang · 2016-08-26T23:50:31Z

components/script/dom/htmlselectelement.rs

+    fn Item(&self, index: u32) -> Option<Root<Element>> {
+        let node = self.upcast::<Node>();
+        let item = node.traverse_preorder().filter_map(Root::downcast::<HTMLOptionElement>).nth(index as usize);
+        if item.is_some() {


if let Some(item) = item {

Actually, we can even do this:

item.map(|i| Root::from_ref(i.upcast()))

Or, if you don't mind chaining up functions:

self.upcast::<Node>() .traverse_preorder() .filter_map(Root::downcast::<HTMLOptionElement>) .nth(index as usize) .map(|item| Root::from_ref(item.upcast()))

KiChjang · 2016-08-27T00:33:24Z

components/script/dom/htmlselectelement.rs

+        let node = self.upcast::<Node>();
+        if value < length {     // truncate the number of option elements
+            for _ in 0..(length - value) {
+                let lastChild = node.GetLastChild();


I think we can just use node.descending_last_children().take(length - value) here.

For some reason

for child in node.descending_last_children().take((length - value) as usize) { if let Err(e) = node.RemoveChild(&child) { println!("error removing child of HTML Select: {:?}", e); } }

doesn't work. I'll check it and upload next commit.

let mut iter = node.descending_last_children().take((length - value) as usize); while let Some(child) = iter.next() { if let Err(e) = node.RemoveChild(&child) { println!("Error removing child of HTMLSelectElement: {:?}", e); } }

Still doesn't work, removing children test fails.

Manishearth · 2016-09-16T10:06:14Z

Any updates? If you need help, just ask! 😄

mskrzypkows · 2016-09-16T10:10:34Z

I'll update today, sorry for the delay, I was busy recently.

Manishearth · 2016-09-16T10:14:08Z

No problem :)

KiChjang · 2016-09-16T17:33:36Z

components/script/dom/htmlselectelement.rs

+        let node = self.upcast::<Node>();
+        if value < length {     // truncate the number of option elements
+            for _ in 0..(length - value) {
+                if let Some(lastChild) = node.GetLastChild() {


Looks like you ignored my review comments for this part.

I didn't ignore your comment, check my reply to your previous comment. It doesn't work as you mentioned children nodes aren't deleted.

Sorry, descending_last_children is incorrect. Use rev_children instead.

OK, thanks for hints.

KiChjang · 2016-09-16T17:33:36Z

components/script/dom/htmlselectelement.rs

+
+    // https://html.spec.whatwg.org/multipage/#dom-select-length
+    fn Length(&self) -> u32 {
+        let node = self.upcast::<Node>();


This can be melded into the following line without a let-binding to node.

KiChjang · 2016-09-17T21:18:55Z

components/script/dom/htmlselectelement.rs

+        let node = self.upcast::<Node>();
+        if value < length {     // truncate the number of option elements
+            for _ in 0..(length - value) {
+                if let Some(lastChild) = node.GetLastChild() {


Sorry, descending_last_children is incorrect. Use rev_children instead.

KiChjang · 2016-09-17T21:18:55Z

...s/wpt/metadata/html/semantics/forms/the-select-element/common-HTMLOptionsCollection.html.ini

@@ -1,18 +1,2 @@
 [common-HTMLOptionsCollection.html]
  type: testharness


This file can be deleted.

KiChjang · 2016-09-21T07:54:39Z

Squash!

bors-servo · 2016-09-21T13:49:12Z

💔 Test failed - mac-rel-wpt

jdm · 2016-09-21T14:14:59Z

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLSelectElement interface: attribute length

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLSelectElement interface: operation item(unsigned long)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLSelectElement interface: document.createElement("select") must inherit property "length" with the proper type (10)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLSelectElement interface: document.createElement("select") must inherit property "item" with the proper type (11)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLSelectElement interface: calling item(unsigned long) on document.createElement("select") with too few arguments must throw TypeError


        Implement indexed access on select elements

refs #11763

mskrzypkows · 2016-09-22T12:07:29Z

Strange build error, it seems not related to my changes.

KiChjang · 2016-09-22T12:19:15Z

@bors-servo r+

bors-servo · 2016-09-22T12:19:16Z

📌 Commit 670693b has been approved by KiChjang

bors-servo · 2016-09-22T13:51:34Z

⌛ Testing commit 670693b with merge 96f6c1d...


        Auto merge of #13066 - mskrzypkows:htmlselect_index, r=KiChjang

Implement indexed access on select elements  Added methods for indexed access on select: SetLength, Length, Item, IndexedGetter ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11763 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13066)

bors-servo · 2016-09-22T15:01:28Z

💔 Test failed - mac-rel-wpt

KiChjang · 2016-09-22T17:30:06Z

@bors-servo retry

#12654, #13359

bors-servo · 2016-09-22T18:14:28Z

⌛ Testing commit 670693b with merge d74f6ad...


        Auto merge of #13066 - mskrzypkows:htmlselect_index, r=KiChjang

Implement indexed access on select elements  Added methods for indexed access on select: SetLength, Length, Item, IndexedGetter ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11763 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13066)

bors-servo · 2016-09-22T19:18:53Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned jdm Aug 26, 2016

highfive added the S-awaiting-review label Aug 26, 2016

highfive assigned KiChjang and unassigned jdm Aug 26, 2016

KiChjang reviewed Aug 26, 2016
View changes

components/script/dom/htmlselectelement.rs Outdated

}

}

} else if value > length { // add new blank option elements

let document = node.GetOwnerDocument();

This comment has been minimized.

Sign in to view

Copy link

KiChjang Aug 26, 2016
Member

document_from_node(self)

KiChjang reviewed Aug 26, 2016
View changes

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Aug 27, 2016

KiChjang reviewed Aug 27, 2016
View changes

mskrzypkows force-pushed the mskrzypkows:htmlselect_index branch from 10138ca to a3134fc Sep 16, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 16, 2016

KiChjang requested changes Sep 16, 2016

View changes

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Sep 16, 2016

KiChjang requested changes Sep 17, 2016

View changes

mskrzypkows force-pushed the mskrzypkows:htmlselect_index branch from a3134fc to e9d5615 Sep 19, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 19, 2016

KiChjang added S-needs-squash and removed S-awaiting-review labels Sep 21, 2016

mskrzypkows force-pushed the mskrzypkows:htmlselect_index branch from e9d5615 to 920f42b Sep 21, 2016

highfive added the S-awaiting-review label Sep 21, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 21, 2016

Implement indexed access on select elements

Loading status checks…

670693b

refs #11763

mskrzypkows force-pushed the mskrzypkows:htmlselect_index branch from 920f42b to 670693b Sep 22, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Sep 22, 2016

KiChjang approved these changes Sep 22, 2016

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 22, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 22, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 22, 2016

highfive removed the S-awaiting-merge label Sep 22, 2016

notriddle modified the milestone: Taiwan Sprint Sep 23, 2016

mskrzypkows deleted the mskrzypkows:htmlselect_index branch Sep 23, 2016

servo / servo

Implement indexed access on select elements #13066

Implement indexed access on select elements #13066

mskrzypkows commented Aug 26, 2016 •

edited by larsbergstrom

highfive commented Aug 26, 2016

jdm commented Aug 26, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Sep 16, 2016

mskrzypkows commented Sep 16, 2016

Manishearth commented Sep 16, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Sep 21, 2016

bors-servo commented Sep 21, 2016

jdm commented Sep 21, 2016

mskrzypkows commented Sep 22, 2016

KiChjang commented Sep 22, 2016

bors-servo commented Sep 22, 2016

bors-servo commented Sep 22, 2016

bors-servo commented Sep 22, 2016

KiChjang commented Sep 22, 2016

bors-servo commented Sep 22, 2016

bors-servo commented Sep 22, 2016

		@@ -1,18 +1,2 @@
		[common-HTMLOptionsCollection.html]
		type: testharness

servo / servo

Join GitHub today

Implement indexed access on select elements #13066

Implement indexed access on select elements #13066

Conversation

mskrzypkows commented Aug 26, 2016 • edited by larsbergstrom

highfive commented Aug 26, 2016

jdm commented Aug 26, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang Aug 26, 2016 • edited

This comment has been minimized.

KiChjang Aug 27, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang Sep 10, 2016 • edited

This comment has been minimized.

Manishearth commented Sep 16, 2016

mskrzypkows commented Sep 16, 2016

Manishearth commented Sep 16, 2016

This comment has been minimized.

KiChjang Sep 16, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Sep 21, 2016

bors-servo commented Sep 21, 2016

jdm commented Sep 21, 2016

mskrzypkows commented Sep 22, 2016

KiChjang commented Sep 22, 2016

bors-servo commented Sep 22, 2016

bors-servo commented Sep 22, 2016

bors-servo commented Sep 22, 2016

KiChjang commented Sep 22, 2016

bors-servo commented Sep 22, 2016

bors-servo commented Sep 22, 2016

mskrzypkows commented Aug 26, 2016 •

edited by larsbergstrom

KiChjang Aug 26, 2016 •

edited

KiChjang Aug 27, 2016 •

edited

KiChjang Sep 10, 2016 •

edited

KiChjang Sep 16, 2016 •

edited