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 HTMLTextArea.setSelectionRange (continuation of #10007) #10612

Merged
merged 1 commit into from Apr 17, 2016

Conversation

@autrilla
Copy link
Contributor

autrilla commented Apr 14, 2016

Tests on tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html all pass and the other tests don't panic due to double borrows anymore.

cc: @KiChjang

Fixes #9994.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Apr 14, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmltextareaelement.rs, components/script/dom/webidls/HTMLTextAreaElement.webidl, components/script/dom/htmlinputelement.rs, components/script/textinput.rs
@jdm jdm assigned KiChjang and unassigned asajeffrey Apr 14, 2016
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Apr 14, 2016

Looks like ./mach test-unit -p script currently fails:

C:\projects\servo\tests\unit\script\textinput.rs:127:25: 132:6 error: this function takes 5 parameters but 4 parameters were supplied [E0061]
C:\projects\servo\tests\unit\script\textinput.rs:127     let mut textinput = TextInput::new(
C:\projects\servo\tests\unit\script\textinput.rs:128         Lines::Single,
C:\projects\servo\tests\unit\script\textinput.rs:129         DOMString::from(""),
C:\projects\servo\tests\unit\script\textinput.rs:130         DummyClipboardContext::new(""),
C:\projects\servo\tests\unit\script\textinput.rs:131         Some(2)
C:\projects\servo\tests\unit\script\textinput.rs:132     );
C:\projects\servo\tests\unit\script\textinput.rs:127:25: 132:6 help: run `rustc --explain E0061` to see a detailed explanation
C:\projects\servo\tests\unit\script\textinput.rs:144:25: 149:6 error: this function takes 5 parameters but 4 parameters were supplied [E0061]
C:\projects\servo\tests\unit\script\textinput.rs:144     let mut textinput = TextInput::new(
C:\projects\servo\tests\unit\script\textinput.rs:145         Lines::Single,
C:\projects\servo\tests\unit\script\textinput.rs:146         DOMString::from(""),
C:\projects\servo\tests\unit\script\textinput.rs:147         DummyClipboardContext::new(""),
C:\projects\servo\tests\unit\script\textinput.rs:148         Some(3)
C:\projects\servo\tests\unit\script\textinput.rs:149     );
C:\projects\servo\tests\unit\script\textinput.rs:144:25: 149:6 help: run `rustc --explain E0061` to see a detailed explanation
C:\projects\servo\tests\unit\script\textinput.rs:163:25: 168:6 error: this function takes 5 parameters but 4 parameters were supplied [E0061]
C:\projects\servo\tests\unit\script\textinput.rs:163     let mut textinput = TextInput::new(
C:\projects\servo\tests\unit\script\textinput.rs:164         Lines::Single,
C:\projects\servo\tests\unit\script\textinput.rs:165         DOMString::from("\u{10437}"),
C:\projects\servo\tests\unit\script\textinput.rs:166         DummyClipboardContext::new(""),
C:\projects\servo\tests\unit\script\textinput.rs:167         Some(1)
C:\projects\servo\tests\unit\script\textinput.rs:168     );
C:\projects\servo\tests\unit\script\textinput.rs:163:25: 168:6 help: run `rustc --explain E0061` to see a detailed explanation
error: aborting due to 3 previous errors 
Build failed, waiting for other jobs to finish...
error: Could not compile `script_tests`.

To learn more, run the command again with --verbose.
Command exited with code 101
@autrilla

This comment has been minimized.

Copy link
Contributor Author

autrilla commented Apr 14, 2016

@jdm yes, I noticed that. Do you remember it failing on the previous PR? I don't see any record of it, but I didn't change TextInput::new at all. Anyway, I'll be sure to fix that.

@autrilla autrilla force-pushed the autrilla:textdir branch from 675e094 to 693d764 Apr 14, 2016
@emilio

This comment has been minimized.

Copy link
Member

emilio commented Apr 16, 2016

The changes look good to me, thanks for doing this! :)

I left some nits. There were also some tests crashing due to double borrows... Not totally sure they're fixed, so I'll make a try run before r+'ing.

The panicking tests were html/semantics/forms/the-input-element/selection.html, html/semantics/forms/textfieldselection/selection-not-application-textarea.html, html/semantics/forms/textfieldselection/selection-not-application.htmland html/semantics/forms/textfieldselection/textfieldselection-setRangeText.html.

@bors-servo: try

There were also a few other passing tests IIRC, so more expectations might need to get updated... Anyways, we'll find out soon! :)

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


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


components/script/dom/htmlinputelement.rs, line 545 [r3] (raw file):
nit: The match below could be written more concisely as:

let direction = direction.map_or(SelectionDirection::None, |d| SelectionDirection::from(d));
self.textinput.borrow_mut().selection_direction = direction;

components/script/dom/htmltextareaelement.rs, line 257 [r3] (raw file):
ditto


Comments from Reviewable

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Apr 16, 2016

⌛️ Trying commit 693d764 with merge a795b35...

bors-servo added a commit that referenced this pull request Apr 16, 2016
Implement HTMLTextArea.setSelectionRange (continuation of #10007)

Tests on `tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html` all pass and the other tests don't panic due to double borrows anymore.

cc: @KiChjang

Fixes #9994.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10612)
<!-- Reviewable:end -->
@emilio emilio assigned emilio and unassigned KiChjang Apr 16, 2016
@autrilla autrilla force-pushed the autrilla:textdir branch from 693d764 to c493020 Apr 16, 2016
@autrilla

This comment has been minimized.

Copy link
Contributor Author

autrilla commented Apr 17, 2016

@emilio any clue why the appveyor tests fail?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Apr 17, 2016

That's an unrelated problem that was fixed in a PR that merged after that commit was pushed.

@autrilla autrilla force-pushed the autrilla:textdir branch from c493020 to b77528b Apr 17, 2016
@autrilla

This comment has been minimized.

Copy link
Contributor Author

autrilla commented Apr 17, 2016

@jdm thanks. I rebased my commit on top of the last one on master.

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Apr 17, 2016

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Apr 17, 2016

⌛️ Trying commit b77528b with merge 943da37...

bors-servo added a commit that referenced this pull request Apr 17, 2016
Implement HTMLTextArea.setSelectionRange (continuation of #10007)

Tests on `tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html` all pass and the other tests don't panic due to double borrows anymore.

cc: @KiChjang

Fixes #9994.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10612)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Apr 17, 2016

💔 Test failed - linux-rel

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Apr 17, 2016

Cool, you have to update the interfaces.html test:

Tests with unexpected results:
  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTextAreaElement interface: attribute selectionStart

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

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

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTextAreaElement interface: operation setSelectionRange(unsigned long,unsigned long,DOMString)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTextAreaElement interface: document.createElement("textarea") must inherit property "selectionStart" with the proper type (27)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTextAreaElement interface: document.createElement("textarea") must inherit property "selectionEnd" with the proper type (28)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTextAreaElement interface: document.createElement("textarea") must inherit property "selectionDirection" with the proper type (29)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTextAreaElement interface: document.createElement("textarea") must inherit property "setSelectionRange" with the proper type (32)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTextAreaElement interface: calling setSelectionRange(unsigned long,unsigned long,DOMString) on document.createElement("textarea") with too few arguments must throw TypeError
@emilio

This comment has been minimized.

Copy link
Member

emilio commented Apr 17, 2016

@bors-servo: r+

Thanks for taking this @autrilla!

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Apr 17, 2016

📌 Commit 5e863f2 has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Apr 17, 2016

⌛️ Testing commit 5e863f2 with merge b00c274...

bors-servo added a commit that referenced this pull request Apr 17, 2016
Implement HTMLTextArea.setSelectionRange (continuation of #10007)

Tests on `tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html` all pass and the other tests don't panic due to double borrows anymore.

cc: @KiChjang

Fixes #9994.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10612)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Apr 17, 2016

@bors-servo bors-servo merged commit 5e863f2 into servo:master Apr 17, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@autrilla autrilla deleted the autrilla:textdir branch Apr 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.