Change Offset_to_text_point function and add unit tests #19946

paavininanda · 2018-02-04T14:54:16Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix assertion failed: self.edit_point.index <= self.lines[self.edit_point.line].len() #18613.

Added unit tests and web platform tests

This change is

highfive · 2018-02-04T14:54:22Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

highfive · 2018-02-04T14:54:22Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/textinput.rs
@fitzgen: components/script/textinput.rs
@KiChjang: components/script/textinput.rs

highfive · 2018-02-04T14:54:23Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2018-02-04T15:23:55Z

If you change the testcase in #18613 to use any other value than 1, does the assertion still fail?

emilio

I can try to take a closer look, but also maybe @mbrubeck and @jonleighton can take a look.

emilio · 2018-02-04T15:34:13Z

components/script/textinput.rs

@@ -276,7 +276,7 @@ impl<T: ClipboardProvider> TextInput<T> {
        }

        debug_assert!(self.edit_point.line < self.lines.len());
-        debug_assert!(self.edit_point.index <= self.lines[self.edit_point.line].len());
+        debug_assert!(self.edit_point.index <= max(1, self.lines[self.edit_point.line].len()));


I need to take a look at what's going on on the test-case exactly, but I suspect this is wrong.

In particular, this should be self.lines[self.edit_point.line].len() + 1 instead, but in any case, I still suspect it's the wrong solution, and the edit_point should end up being 1, 0 instead of 0, 1 in this case, since we have two lines in the test-case.

emilio · 2018-02-04T15:34:14Z

components/script/textinput.rs

@@ -276,7 +276,7 @@ impl<T: ClipboardProvider> TextInput<T> {
        }

        debug_assert!(self.edit_point.line < self.lines.len());
-        debug_assert!(self.edit_point.index <= self.lines[self.edit_point.line].len());
+        debug_assert!(self.edit_point.index <= max(1, self.lines[self.edit_point.line].len()));


I need to take a look at what's going on on the test-case exactly, but I suspect this is wrong.

In particular, this should be self.lines[self.edit_point.line].len() + 1 instead, but in any case, I still suspect it's the wrong solution, and the edit_point should end up being 1, 0 instead of 0, 1 in this case, since we have two lines in the test-case.

emilio · 2018-02-04T15:49:08Z

So the issue is that with two empty lines (["", ""]), offset_to_text_point(1) is returning (0, 1) instead of (1, 0).

paavininanda · 2018-02-04T15:49:43Z

@jdm Yes it fails then also.

paavininanda · 2018-02-04T15:52:33Z

@emilio So should we alter the offset_to_text_point function instead?

emilio · 2018-02-04T15:56:37Z

Yes, I think so...

emilio · 2018-02-04T16:03:24Z

In particular I think that what offset_to_text_point wants to do is something like this:

        for line_content in self.lines.iter() {
            // If the index fits on the line, we're done.
            if index <= line_content.len() {
                break;
            }

            // else advance a line.
            index -= line_content.len();
            line += 1;
            if index > 0 {
                index -= 1; // Account for the \n
            }
        }

But I'm not sure that's 100% right, it may need to go through tests and all that :)

jonleighton · 2018-02-04T17:19:35Z

It would be great to add additional unit test(s) to cover this case I think. (The relevant file is tests/unit/script/textinput.rs.)

@emilio: I'm happy to try to review this if you like.

jdm · 2018-02-04T17:30:06Z

Note: unit tests can be run with ./mach test-unit -p script

emilio · 2018-02-04T20:41:44Z

@bors-servo try

bors-servo · 2018-02-04T20:41:49Z

⌛ Trying commit 5a6e63e with merge 13e2000...

Change Offset_to_text_point function and add unit tests  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #(#18613).  - [x] These changes do not require tests because they solve an issue related to a already existing code    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19946)

bors-servo · 2018-02-04T21:29:15Z

☀️ 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
State: approved= try=True

paavininanda · 2018-02-05T03:35:49Z

r? @emilio

emilio

Looks good, thanks!

emilio · 2018-02-05T11:52:37Z

components/script/textinput.rs

-                if abs_point > new_acc && index > line_end {
+                let line_end = val.len();
+                let new_acc = acc + line_end + 1;
+                if abs_point >= new_acc && index > line_end {
                    index -= line_end + 1;


I still think this function is way harder to understand than what it should, but I'll file a followup after this lands to do that.

Yeah! Actually changing the function altogether was creating some new issues. So i preferred changing it this way only.

emilio · 2018-02-05T11:54:40Z

tests/unit/script/textinput.rs

@@ -600,6 +600,13 @@ fn test_textinput_set_selection_with_direction() {
    assert!(textinput.selection_origin.is_some());
    assert_eq!(textinput.selection_origin.unwrap().line, 0);
    assert_eq!(textinput.selection_origin.unwrap().index, 6);
+
+    textinput = text_input(Lines::Multiple, "\n\n");


Let's test also with only \n, which is the case we're concerned about. Also, let's add a crashtest for this based on @mateon1's test-case, into tests/wpt/web-platform-tests/selection/. See https://github.com/servo/servo/blob/master/tests/wpt/README.md#writing-new-tests.

The test should call setSelectionRange, but also that the selectionStart and selectionEnd values agree with other browsers, that is, something like:

<!doctype html> <textarea> </textarea> <script> test(function() { let textarea = document.querySelector('textarea'); assert_equals(textarea.selectionStart, 0); assert_equals(textarea.selectionEnd, 0); textarea.setSelectionRange(0, 1); assert_equals(textarea.selectionStart, 0); assert_equals(textarea.selectionEnd, 1); }, "setSelectionRange on line boundaries"); </script>

servo-wpt-sync · 2018-02-05T16:57:37Z

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#23.

emilio

So the unit test you wrote seems to be failing on travis:

	thread 'textinput::test_textinput_set_selection_with_direction' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`', tests/unit/script/textinput.rs:616:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:68
             at libstd/sys_common/backtrace.rs:57
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:380
   3: std::panicking::default_hook
             at libstd/panicking.rs:390
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:576
   5: std::panicking::begin_panic
             at libstd/panicking.rs:537
   6: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:521
   7: script_tests::textinput::test_textinput_set_selection_with_direction
             at tests/unit/script/textinput.rs:616
   8: <F as alloc::boxed::FnBox<A>>::call_box
             at libtest/lib.rs:1441
             at /checkout/src/libcore/ops/function.rs:223
             at /checkout/src/liballoc/boxed.rs:788
   9: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102

Additionally, you need to make sure the manifest is up-to-date running ./mach update-manifest.

emilio · 2018-02-05T17:49:37Z

tests/unit/script/textinput.rs

+
+    textinput = text_input(Lines::Multiple, "\n");
+    textinput.set_selection_range(0, 1, SelectionDirection::Forward);
+    assert_eq!(textinput.edit_point.line, 0);


Hmm, I suspect this should be 1...

servo-wpt-sync · 2018-02-05T18:04:28Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#23.

servo-wpt-sync · 2018-02-05T18:24:26Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#23.

servo-wpt-sync · 2018-02-06T19:51:39Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#23.

jdm · 2018-02-06T20:11:51Z

@bors-servo r+

bors-servo · 2018-02-06T20:11:52Z

📌 Commit 7b58fb5 has been approved by jdm

bors-servo · 2018-02-06T20:11:58Z

⌛ Testing commit 7b58fb5 with merge 36eb711...

Change Offset_to_text_point function and add unit tests  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18613.  - [x] Added unit tests and web platform tests    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19946)

bors-servo · 2018-02-06T21:12:28Z

☀️ 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: jdm
Pushing 36eb711 to master...

Upstreamed from servo/servo#19946 [ci skip]

highfive assigned emilio Feb 4, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 4, 2018

emilio reviewed Feb 4, 2018

View reviewed changes

paavininanda changed the title ~~Changed an assertion for the case when line has value "\n"~~ Change Offset_to_text_point function and add unit tests Feb 4, 2018

emilio approved these changes Feb 5, 2018

View reviewed changes

emilio reviewed Feb 5, 2018

View reviewed changes

Changed offset_to_text_point function and added unit tests for the same

7b58fb5

paavininanda force-pushed the Issue18613 branch from 0c2b774 to 7b58fb5 Compare February 6, 2018 19:51

highfive assigned jdm and unassigned emilio Feb 6, 2018

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 Feb 6, 2018

bors-servo merged commit 7b58fb5 into servo:master Feb 6, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 6, 2018

jdm pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 7, 2018

Changed offset_to_text_point function and added unit tests for the same

eb127ab

Upstreamed from servo/servo#19946 [ci skip]

servo-wpt-sync mentioned this pull request Feb 7, 2018

Changed offset_to_text_point function and added unit tests for the same web-platform-tests/wpt#9425

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Offset_to_text_point function and add unit tests #19946

Change Offset_to_text_point function and add unit tests #19946

paavininanda commented Feb 4, 2018 •

edited

highfive commented Feb 4, 2018

highfive commented Feb 4, 2018

highfive commented Feb 4, 2018

jdm commented Feb 4, 2018

emilio left a comment

emilio Feb 4, 2018

emilio Feb 4, 2018

emilio commented Feb 4, 2018

paavininanda commented Feb 4, 2018

paavininanda commented Feb 4, 2018

emilio commented Feb 4, 2018

emilio commented Feb 4, 2018 •

edited

jonleighton commented Feb 4, 2018

jdm commented Feb 4, 2018 •

edited

emilio commented Feb 4, 2018

bors-servo commented Feb 4, 2018

bors-servo commented Feb 4, 2018

paavininanda commented Feb 5, 2018

emilio left a comment

emilio Feb 5, 2018

paavininanda Feb 5, 2018

emilio Feb 5, 2018

servo-wpt-sync commented Feb 5, 2018

emilio left a comment

emilio Feb 5, 2018

servo-wpt-sync commented Feb 5, 2018

servo-wpt-sync commented Feb 5, 2018

servo-wpt-sync commented Feb 6, 2018

jdm commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

Change Offset_to_text_point function and add unit tests #19946

Change Offset_to_text_point function and add unit tests #19946

Conversation

paavininanda commented Feb 4, 2018 • edited

highfive commented Feb 4, 2018

highfive commented Feb 4, 2018

highfive commented Feb 4, 2018

jdm commented Feb 4, 2018

emilio left a comment

Choose a reason for hiding this comment

emilio Feb 4, 2018

Choose a reason for hiding this comment

emilio Feb 4, 2018

Choose a reason for hiding this comment

emilio commented Feb 4, 2018

paavininanda commented Feb 4, 2018

paavininanda commented Feb 4, 2018

emilio commented Feb 4, 2018

emilio commented Feb 4, 2018 • edited

jonleighton commented Feb 4, 2018

jdm commented Feb 4, 2018 • edited

emilio commented Feb 4, 2018

bors-servo commented Feb 4, 2018

bors-servo commented Feb 4, 2018

paavininanda commented Feb 5, 2018

emilio left a comment

Choose a reason for hiding this comment

emilio Feb 5, 2018

Choose a reason for hiding this comment

paavininanda Feb 5, 2018

Choose a reason for hiding this comment

emilio Feb 5, 2018

Choose a reason for hiding this comment

servo-wpt-sync commented Feb 5, 2018

emilio left a comment

Choose a reason for hiding this comment

emilio Feb 5, 2018

Choose a reason for hiding this comment

servo-wpt-sync commented Feb 5, 2018

servo-wpt-sync commented Feb 5, 2018

servo-wpt-sync commented Feb 6, 2018

jdm commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

paavininanda commented Feb 4, 2018 •

edited

emilio commented Feb 4, 2018 •

edited

jdm commented Feb 4, 2018 •

edited