HTML input value #9514

g-k · 2016-02-03T04:06:00Z

Ready for review.

KiChjang · 2016-02-03T04:24:02Z

components/script/dom/htmlinputelement.rs

+    fn default() -> InputValueMode { InputValueMode::Default }
+}
+
+impl InputValueMode {


nit: Move these impl up to where the enum is.

frewsxcv · 2016-02-03T05:00:10Z

./components/script/dom/htmlinputelement.rs:396: link to WHATWG may break in the future, use this format instead: https://html.spec.whatwg.org/multipage/#input-type-attr-summary
./components/script/dom/htmlinputelement.rs:411: missing space before {

g-k · 2016-02-03T05:10:00Z

Thanks for the feedback!

I broke a bunch of tests in: tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/ so I'm going to work on getting those passing next.

g-k · 2016-02-05T01:14:11Z

OK, I'm stuck.

9a4b26f passes the radionodelist test and the tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/ tests except for the one below.

I tried to get it test passing by setting the value to the textinput value when the type attribute changes (in VirtualMethods attribute_mutated for type AttributeMutation::Set), but that crashes.

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  │ FAIL [expected PASS] change state from hidden to checkbox
  │   → assert_equals: input.value should be '  foo\rbar  ' after change of state expected "  foo\rbar  " but got "on"
  │ FAIL [expected PASS] change state from hidden to radio
  │   → assert_equals: input.value should be '  foo\rbar  ' after change of state expected "  foo\rbar  " but got "on"
...
  │ FAIL [expected PASS] change state from image to reset
  │   → assert_equals: input.value should be '  foo\rbar  ' after change of state expected "  foo\rbar  " but got ""
  │ FAIL [expected PASS] change state from image to button
  │   → assert_equals: input.value should be '  foo\rbar  ' after change of state expected "  foo\rbar  " but got ""
  │ 
  │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:42:15
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:28:1

KiChjang · 2016-02-05T03:25:46Z

components/script/dom/htmlinputelement.rs

+        } else {
+            input.upcast::<Element>()
+                .get_attribute(&ns!(), &atom!("value"))
+                .map_or_else(|| default,


This looks like a .map_or() is enough, no need for .map_or_else()

KiChjang · 2016-02-05T03:27:20Z

Could you also provide step annotations for the functions that you've wrote? It's hard to follow which part of the spec you're implementing.

KiChjang · 2016-02-05T04:25:04Z

components/script/dom/htmlinputelement.rs

@@ -292,14 +352,12 @@ impl HTMLInputElementMethods for HTMLInputElement {

    // https://html.spec.whatwg.org/multipage/#dom-input-value
    fn Value(&self) -> DOMString {
-        self.textinput.borrow().get_content()
+        InputValueMode::from_input_element(&self).get_value(self)


I'm not sure how I missed this, but this style is kind of weird. I'd expect InputValueMode to be a field in HTMLInputElement, and this method would just be a call to self.get_value(). Here you're constructing a new InputValueMode every time you get or set the value of an HTMLInputElement, and then calling a method on the InputValueMode, supplying it with an instance of HTMLInputElement.

KiChjang · 2016-02-05T04:46:45Z

components/script/dom/htmlinputelement.rs

+    }
+
+    fn set_value(&self, input: &HTMLInputElement, value: DOMString) {
+        let element = input.upcast::<Element>();


I see no value in putting the let-binding here, since element is only used in the else-body.

KiChjang · 2016-02-05T06:38:36Z

I suspect that the test failures are because of the changes between InputModeValues - some of them make changes to the attributes on the upcasted Element struct, and some don't. Filed whatwg/html#628 for some clairifications.

g-k · 2016-02-08T05:25:53Z

Ah! I thought only content attributes were called attributes.

I made the suggested changes and the input and raidonodelist tests pass, but the implementation is wrong since SetValue for default/on updates the content attribute.

I need to find out if the simple list iterator does a tree order traversal (pre-order, depth-first traversal of DOM nodes) and whether remove attribute on an Element should call attribute_mutated when it's an HTMLInputElement.

KiChjang · 2016-02-12T21:14:42Z

.../web-platform-tests/html/infrastructure/common-dom-interfaces/collections/radionodelist.html

+	// assert_equals(document.getElementById("r1").value, -39); // but got string "1"
+
+	rdoList.value = "on";
+	// expected "1" but got "2"


Revert changes to this file, please.

I will. This is a WIP branch.

I just thought it was interesting, because it seems like IDL attribute has the right value before it is set to "on".

KiChjang · 2016-02-16T19:14:39Z

components/script/dom/htmlinputelement.rs

+
+            // On setting, if the new value is the empty string, it
+            // must empty the list of selected files; otherwise, it
+            // must throw an InvalidStateError exception.


All of these annotations are unnecessary; we only need step numbers.

I might be misreading the spec or looking in the wrong place, but I don't see any step numbers on: https://html.spec.whatwg.org/multipage/forms.html#dom-input-value

KiChjang · 2016-02-16T19:15:37Z

components/script/dom/htmlinputelement.rs

+            // must return that attribute's value; otherwise, it must
+            // return the string "on" (for default/on mode)
+            ValueMode::DefaultOn => {
+                let content = self.textinput.borrow().get_content();


This should instead upcast itself to an Element and grab the content attribute value from it.

Same for ValueMode::Default.

My bad, I didn't realize the red text in the spec are links and I thought they were talking about the IDL attribute.

Confusingly the bold red text is talking about an IDL attribute.

KiChjang · 2016-02-17T07:32:21Z

components/script/dom/htmlinputelement.rs

+                             |a| DOMString::from(a.summarize().value)),
+            ValueMode::DefaultOn =>
+                self.upcast::<Element>()
+                .get_attribute(&ns!(), &atom!("value"))


nit: Indentation

KiChjang · 2016-02-17T07:32:32Z

components/script/dom/htmlinputelement.rs

+            ValueMode::Filename | ValueMode::Value => self.textinput.borrow().get_content(),
+            ValueMode::Default =>
+                self.upcast::<Element>()
+                .get_attribute(&ns!(), &atom!("value"))


nit: Indentation

KiChjang · 2016-02-17T07:41:46Z

components/script/dom/htmlinputelement.rs

@@ -292,12 +320,32 @@ impl HTMLInputElementMethods for HTMLInputElement {

    // https://html.spec.whatwg.org/multipage/#dom-input-value
    fn Value(&self) -> DOMString {
-        self.textinput.borrow().get_content()
+        match self.get_value_mode() {
+            ValueMode::Filename | ValueMode::Value => self.textinput.borrow().get_content(),


This is actually much more involved than just returning the textinput.The spec says the following for value:

On getting, it must return the current value of the element. On setting, it must set the element's value to the new value, set the element's dirty value flag to true, invoke the value sanitization algorithm, if the element's type attribute's current state defines one, and then, if the element has a text entry cursor position, should move the text entry cursor position to the end of the text field, unselecting any selected text and resetting the selection direction to none.

And the following for filename:

On getting, it must return the string "C:\fakepath" followed by the name of the first file in the list of selected files, if any, or the empty string if the list is empty. On setting, if the new value is the empty string, it must empty the list of selected files; otherwise, it must throw an InvalidStateError exception.

KiChjang · 2016-02-17T07:45:06Z

components/script/dom/htmlinputelement.rs

+    }
+
+    // https://html.spec.whatwg.org/multipage/#input-type-attr-summary
+    fn get_value_mode_for_input_type(input_type: InputType) -> ValueMode {


Why is this written in a functional style? The HTMLInputElement has all the necessary information about its own input type.

I was using it in the type mutation handler.

KiChjang · 2016-02-17T07:46:27Z

components/script/dom/htmlinputelement.rs

+                            self.SetValue(DOMString::from(""));
+                        }
+
+                        if new_type == InputType::InputRadio {


This is step 5.

KiChjang · 2016-02-17T07:46:49Z

components/script/dom/htmlinputelement.rs

+                        } else if old_value_mode != ValueMode::Value && new_value_mode == ValueMode::Value {
+                            // Step 2
+                            self.SetValue(self.upcast::<Element>()
+                                          .get_attribute(&ns!(), &atom!("value"))


nit: Indentation

bors-servo · 2016-02-26T15:04:42Z

⌛ Testing commit e6c31e3 with merge b9698bf...


        Auto merge of #9514 - g-k:html-input-value, r=KiChjang

HTML input value Ready for review. Fixes #9455.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9514)

bors-servo · 2016-02-26T15:58:03Z

💔 Test failed - linux-rel

nox · 2016-02-26T15:58:55Z

@bors-servo retry #9148

bors-servo · 2016-02-26T18:03:01Z

⚡ Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

bors-servo · 2016-02-26T19:14:24Z

💔 Test failed - linux-rel

KiChjang · 2016-02-26T19:18:36Z

@bors-servo retry

#8120

bors-servo · 2016-02-26T20:07:45Z

⌛ Testing commit e6c31e3 with merge f30fb64...


        Auto merge of #9514 - g-k:html-input-value, r=KiChjang

HTML input value Ready for review. Fixes #9455.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9514)

bors-servo · 2016-02-26T21:01:06Z

💔 Test failed - linux-rel

KiChjang · 2016-02-26T21:09:08Z

@bors-servo retry

#9033

bors-servo · 2016-02-26T23:06:47Z

⌛ Testing commit e6c31e3 with merge 0d7a2ee...


        Auto merge of #9514 - g-k:html-input-value, r=KiChjang

HTML input value Ready for review. Fixes #9455.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9514)

bors-servo · 2016-02-27T00:01:04Z

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


        Changing input type to file clears value IDL attr

According to step 3 of https://html.spec.whatwg.org/multipage/forms.html#input-type-change a value mode transition from non-filename to filename should update the input internal value state to the empty string. According to https://html.spec.whatwg.org/multipage/forms.html#dom-input-value a subsequent read of the value IDL attribute should return the empty string (assuming no selected files). Refs: servo/servo#9514

highfive added the S-awaiting-review label Feb 3, 2016

KiChjang reviewed Feb 3, 2016
View changes

components/script/dom/htmlinputelement.rs Outdated

fn default() -> InputValueMode { InputValueMode::Default }

}

impl InputValueMode {

This comment has been minimized.

Sign in to view

Copy link

KiChjang Feb 3, 2016
Member

nit: Move these impl up to where the enum is.

jdm assigned KiChjang Feb 3, 2016

g-k force-pushed the g-k:html-input-value branch from 7f13744 to 9a4b26f Feb 4, 2016

KiChjang reviewed Feb 5, 2016
View changes

g-k force-pushed the g-k:html-input-value branch from 9a4b26f to a9994e1 Feb 8, 2016

KiChjang reviewed Feb 12, 2016
View changes

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Feb 14, 2016

KiChjang reviewed Feb 16, 2016
View changes

g-k force-pushed the g-k:html-input-value branch from a9994e1 to a2efef6 Feb 17, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 17, 2016

g-k force-pushed the g-k:html-input-value branch 2 times, most recently from 8fc5c64 to 7b21d8e Feb 17, 2016

KiChjang reviewed Feb 17, 2016
View changes

components/script/dom/htmlinputelement.rs Outdated

|a| DOMString::from(a.summarize().value)),

ValueMode::DefaultOn =>

self.upcast::<Element>()

.get_attribute(&ns!(), &atom!("value"))

This comment has been minimized.

Sign in to view

Copy link

KiChjang Feb 17, 2016
Member

nit: Indentation

KiChjang reviewed Feb 17, 2016
View changes

components/script/dom/htmlinputelement.rs Outdated

self.SetValue(DOMString::from(""));

}

if new_type == InputType::InputRadio {

This comment has been minimized.

Sign in to view

Copy link

KiChjang Feb 17, 2016
Member

This is step 5.

KiChjang reviewed Feb 17, 2016
View changes

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 26, 2016

nox added S-awaiting-merge and removed S-tests-failed labels Feb 26, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 26, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 26, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 26, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 26, 2016

bors-servo merged commit e6c31e3 into servo:master Feb 27, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors-servo mentioned this pull request Feb 27, 2016

add file support to form data #9198

Closed

g-k deleted the g-k:html-input-value branch Feb 27, 2016

servo / servo

HTML input value #9514

HTML input value #9514

g-k commented Feb 3, 2016

This comment has been minimized.

frewsxcv commented Feb 3, 2016

g-k commented Feb 3, 2016

g-k commented Feb 5, 2016

This comment has been minimized.

KiChjang commented Feb 5, 2016

This comment has been minimized.

This comment has been minimized.

KiChjang commented Feb 5, 2016

g-k commented Feb 8, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Feb 26, 2016

bors-servo commented Feb 26, 2016

nox commented Feb 26, 2016

bors-servo commented Feb 26, 2016

bors-servo commented Feb 26, 2016

KiChjang commented Feb 26, 2016

bors-servo commented Feb 26, 2016

bors-servo commented Feb 26, 2016

KiChjang commented Feb 26, 2016

bors-servo commented Feb 26, 2016

bors-servo commented Feb 27, 2016

servo / servo

Join GitHub today

HTML input value #9514

HTML input value #9514

Conversation

g-k commented Feb 3, 2016

This comment has been minimized.

frewsxcv commented Feb 3, 2016

g-k commented Feb 3, 2016

g-k commented Feb 5, 2016

This comment has been minimized.

KiChjang commented Feb 5, 2016

This comment has been minimized.

This comment has been minimized.

KiChjang commented Feb 5, 2016

g-k commented Feb 8, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Feb 26, 2016

bors-servo commented Feb 26, 2016

nox commented Feb 26, 2016

bors-servo commented Feb 26, 2016

bors-servo commented Feb 26, 2016

KiChjang commented Feb 26, 2016

bors-servo commented Feb 26, 2016

bors-servo commented Feb 26, 2016

KiChjang commented Feb 26, 2016

bors-servo commented Feb 26, 2016

bors-servo commented Feb 27, 2016