Add a sanitize_value implementation for the color input #19330

Eijebong · 2017-11-22T00:18:22Z

I had to change the test a little bit to avoid some failures due to
color and text both having a sanitizedValue which was making the test
use the first assertion instead of the second one in some cases.

The sanitize_value implementation is pretty simple, we iterate over the
content and checks that the content is 7 characters long, that the first
character is a # and then that all the following characters are
hexadecimal. If all those requirements are met, we lowercase the
content, otherwise we put #000000 in it.

This change is

highfive · 2017-11-22T00:18:26Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmlinputelement.rs
@KiChjang: components/script/dom/htmlinputelement.rs

KiChjang

This is a great start! I just have a couple of nits below. r=me after the change.

KiChjang · 2017-11-22T00:55:13Z

components/script/dom/htmlinputelement.rs

+                let is_valid = {
+                    let mut chars = content.chars();
+                    if content.len() == 7 && chars.next() == Some('#') {
+                        chars.fold(true, |v, c| v && c.is_digit(16))


Instead of using fold here, we can use all.

Nice, didn't know about that :)

KiChjang · 2017-11-22T00:55:45Z

components/script/dom/htmlinputelement.rs

+                let mut textinput = self.textinput.borrow_mut();
+                let content = textinput.get_content();
+                let is_valid = {
+                    let mut chars = content.chars();


Why does this need to be mutable?

The iterator needs to be mutable because we're calling .next() on it to discard the # before checking if every other character in it is a valid hex digit.

KiChjang · 2017-11-22T01:00:00Z

components/script/dom/htmlinputelement.rs

@@ -875,6 +875,24 @@ impl HTMLInputElement {
                content.strip_newlines();
                content.strip_leading_and_trailing_ascii_whitespace();
            }
+            atom!("color") => {
+                let mut textinput = self.textinput.borrow_mut();
+                let content = textinput.get_content();


get_content() clones the value, so let's use single_line_content() here.

Eijebong · 2017-11-22T11:18:25Z

About the test failure, should I change the hash myself ?

KiChjang · 2017-11-22T18:32:40Z

@bors-servo r+

Thanks!

bors-servo · 2017-11-22T18:32:41Z

📌 Commit daf66aa has been approved by KiChjang

bors-servo · 2017-11-22T18:54:37Z

⌛ Testing commit daf66aa with merge 199aa27...

Add a sanitize_value implementation for the color input I had to change the test a little bit to avoid some failures due to color and text both having a sanitizedValue which was making the test use the first assertion instead of the second one in some cases. The sanitize_value implementation is pretty simple, we iterate over the content and checks that the content is 7 characters long, that the first character is a `#` and then that all the following characters are hexadecimal. If all those requirements are met, we lowercase the content, otherwise we put `#000000` in it.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19330)

bors-servo · 2017-11-22T19:32:35Z

💔 Test failed - linux-rel-wpt

KiChjang · 2017-11-22T19:41:02Z

Looks like we just hit the jackpot for passing tests!

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/valueMode.html:
  └ PASS [expected FAIL] value IDL attribute of input type color without value attribute

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/valueMode.html:
  └ PASS [expected FAIL] value IDL attribute of input type color with value attribute

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Empty value should return #000000

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Missing value should return #000000

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Valid simple color (containing LATIN CAPITAL LETTERS): should return #ffffff (converted to ASCII lowercase)

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Zero-padding

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid simple color: not 7 characters long

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid simple color: no starting # sign

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid simple color: non ASCII hex digits

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid simple color: foobar

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: trailing Null (U+0000)

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: trailing ;

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: leading space

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: trailing space

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: leading+trailing spaces

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: keyword crimson

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: keyword bisque

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: keyword currentColor

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: keyword transparent

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: keyword ActiveBorder

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: keyword inherit

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: rgb(1,1,1)

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: rgb(1,1,1,1)

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/color.html:
  └ PASS [expected FAIL] Invalid color: PILE OF POO (U+1F4A9)

Eijebong · 2017-11-22T21:08:26Z

\o/ Didn't think about running other tests, I'll change the expectations :)

I had to change the test a little bit to avoid some failures due to color and text both having a sanitizedValue which was making the test use the first assertion instead of the second one in some cases. The sanitize_value implementation is pretty simple, we iterate over the content and checks that the content is 7 characters long, that the first character is a `#` and then that all the following characters are hexadecimal. If all those requirements are met, we lowercase the content, otherwise we put `#000000` in it.

KiChjang · 2017-11-23T00:07:11Z

@bors-servo r+

bors-servo · 2017-11-23T00:07:11Z

📌 Commit 05c4e08 has been approved by KiChjang

bors-servo · 2017-11-23T01:42:59Z

⌛ Testing commit 05c4e08 with merge 72e7f60...

Add a sanitize_value implementation for the color input I had to change the test a little bit to avoid some failures due to color and text both having a sanitizedValue which was making the test use the first assertion instead of the second one in some cases. The sanitize_value implementation is pretty simple, we iterate over the content and checks that the content is 7 characters long, that the first character is a `#` and then that all the following characters are hexadecimal. If all those requirements are met, we lowercase the content, otherwise we put `#000000` in it.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19330)

bors-servo · 2017-11-23T03:05:03Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: KiChjang
Pushing 72e7f60 to master...

I had to change the test a little bit to avoid some failures due to color and text both having a sanitizedValue which was making the test use the first assertion instead of the second one in some cases. The sanitize_value implementation is pretty simple, we iterate over the content and checks that the content is 7 characters long, that the first character is a `#` and then that all the following characters are hexadecimal. If all those requirements are met, we lowercase the content, otherwise we put `#000000` in it. Upstreamed from servo/servo#19330 [ci skip]

highfive assigned pcwalton Nov 22, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 22, 2017

Eijebong force-pushed the sanitize_color branch from 5c63f86 to f05d857 Compare November 22, 2017 00:44

KiChjang suggested changes Nov 22, 2017

View reviewed changes

KiChjang assigned KiChjang and unassigned pcwalton Nov 22, 2017

KiChjang 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 22, 2017

Eijebong force-pushed the sanitize_color branch from f05d857 to daf66aa Compare November 22, 2017 12:31

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 22, 2017

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 22, 2017

KiChjang approved these changes Nov 22, 2017

View reviewed changes

KiChjang mentioned this pull request Nov 22, 2017

[meta] Implement value sanitizations for different input modes #19172

Closed

8 tasks

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 Nov 22, 2017

Eijebong force-pushed the sanitize_color branch from daf66aa to 05c4e08 Compare November 22, 2017 21:36

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 22, 2017

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 23, 2017

bors-servo merged commit 05c4e08 into servo:master Nov 23, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 23, 2017

tigercosmos mentioned this pull request Nov 29, 2017

implement "Date type inputs", "Month type inputs" #19385

Merged

3 tasks

servo-wpt-sync mentioned this pull request Jan 4, 2018

Add a sanitize_value implementation for the color input web-platform-tests/wpt#8906

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a sanitize_value implementation for the color input #19330

Add a sanitize_value implementation for the color input #19330

Eijebong commented Nov 22, 2017 •

edited by SimonSapin

Loading

highfive commented Nov 22, 2017

KiChjang left a comment

KiChjang Nov 22, 2017

Eijebong Nov 22, 2017

KiChjang Nov 22, 2017

Eijebong Nov 22, 2017

KiChjang Nov 22, 2017

Eijebong commented Nov 22, 2017

KiChjang commented Nov 22, 2017

bors-servo commented Nov 22, 2017

bors-servo commented Nov 22, 2017

bors-servo commented Nov 22, 2017

KiChjang commented Nov 22, 2017 •

edited by jdm

Loading

Eijebong commented Nov 22, 2017

KiChjang commented Nov 23, 2017

bors-servo commented Nov 23, 2017

bors-servo commented Nov 23, 2017

bors-servo commented Nov 23, 2017

Add a sanitize_value implementation for the color input #19330

Add a sanitize_value implementation for the color input #19330

Conversation

Eijebong commented Nov 22, 2017 • edited by SimonSapin Loading

highfive commented Nov 22, 2017

KiChjang left a comment

Choose a reason for hiding this comment

KiChjang Nov 22, 2017

Choose a reason for hiding this comment

Eijebong Nov 22, 2017

Choose a reason for hiding this comment

KiChjang Nov 22, 2017

Choose a reason for hiding this comment

Eijebong Nov 22, 2017

Choose a reason for hiding this comment

KiChjang Nov 22, 2017

Choose a reason for hiding this comment

Eijebong commented Nov 22, 2017

KiChjang commented Nov 22, 2017

bors-servo commented Nov 22, 2017

bors-servo commented Nov 22, 2017

bors-servo commented Nov 22, 2017

KiChjang commented Nov 22, 2017 • edited by jdm Loading

Eijebong commented Nov 22, 2017

KiChjang commented Nov 23, 2017

bors-servo commented Nov 23, 2017

bors-servo commented Nov 23, 2017

bors-servo commented Nov 23, 2017

Eijebong commented Nov 22, 2017 •

edited by SimonSapin

Loading

KiChjang commented Nov 22, 2017 •

edited by jdm

Loading