Position insertion point in input field with mouse #14291

fiji-flo · 2016-11-20T10:15:39Z

Implements cursor positioning in input elements (text/password) via mouse. The related issue is #10083 but is only covered partly.
This PR does not cover:

positioning in textarea elements via mouse
text selection in input/textarea elements via mouse

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix input / textarea - mouse interaction doesn't work #10083 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because I can't think of a way to test mouse interaction in the current test pipeline.

This change is

highfive · 2016-11-20T10:15:43Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmlinputelement.rs, components/script/dom/window.rs, components/script_layout_interface/message.rs, components/script_layout_interface/rpc.rs
@fitzgen: components/script/dom/htmlinputelement.rs, components/script/dom/window.rs, components/script_layout_interface/message.rs, components/script_layout_interface/rpc.rs
@emilio: components/layout/query.rs

highfive · 2016-11-20T10:15:44Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify gfx, layout, and script code, but no tests are modified. Please consider adding a test!

bors-servo · 2016-12-06T23:44:05Z

☔ The latest upstream changes (presumably #14367) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2016-12-06T23:51:40Z

@nox review ping.

fiji-flo · 2016-12-07T07:56:54Z

I rebased my branch to solve some merge conflicts.

jdm · 2016-12-15T19:25:43Z

@nox Review ping.

nox · 2016-12-16T09:14:46Z

I know nothing about gfx and layout. r? @pcwalton

emilio

Only minor nits. The approach in general makes sense to me (I'd need to review way more carefully though). Seems like @pcwalton can review this with less effort than I.

emilio · 2016-12-16T10:46:10Z

components/script/dom/window.rs

+        if !self.reflow(ReflowGoal::ForScriptQuery,
+                        ReflowQueryType::TextIndexQuery(node, mouse_x, mouse_y),
+                        ReflowReason::Query) {
+            println!("failed in reflow");


emilio · 2016-12-16T10:46:38Z

components/script_layout_interface/rpc.rs

@@ -92,3 +94,6 @@ impl MarginStyleResponse {
        }
    }
 }
+
+#[derive(Clone)]
+pub struct TextIndexResponse(pub Option<(usize)>);


Why not just TextIndexResponse(pub Option<usize>)?

An early draft had tuple here. Thanks for seeing this.

emilio · 2016-12-16T10:50:16Z

components/gfx/text/text_run.rs

@@ -304,6 +304,24 @@ impl<'a> TextRun {
        })
    }

+    /// Returns the index in the range of the first glyph advancing over given advance
+    pub fn range_index_of_advance(&self, range: &Range<ByteIndex>, advance: Au) -> Option<usize> {
+        if range.is_empty() {


Is this check necessary? Doesn't natural_words_in_range handle empty ranges?

Also, this function seems to always return Some(..), so why not returning just an usize?

fiji-flo · 2016-12-17T07:47:00Z

@emilio Thanks for the first round of feedback. I'll go over it today.

fiji-flo · 2017-01-05T07:42:37Z

@jdm @pcwalton: I rebased and squashed the changes I did after emilio's comments. Could I get some feedback here?

bors-servo · 2017-01-10T16:02:11Z

☔ The latest upstream changes (presumably #14938) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-01-10T16:06:26Z

I'm really sorry about how this work has been received so far @fiji-flo ! I'll make sure it gets some traction today.

pcwalton · 2017-01-10T17:17:18Z

components/gfx/text/glyph.rs

+                    index += 1;
+                    true
+                }
+            }).collect::<Vec<_>>();


It looks like you're collecting this into a vector and then throwing it away.

I recommend just using a for loop:

for glyph in self.iter_glyphs_for_byte_range(range) { ... }

Indeed. My bad.

pcwalton · 2017-01-10T17:18:03Z

components/gfx/display_list/mod.rs

+                       translated_point: &mut Point2D<Au>,
+                       scroll_offsets: &ScrollOffsetMap) {
+        // Adjust the translated point to account for the scroll offset if
+        // necessary. This can only happen when WebRender is in use.


While you're here, could you remove the "This can only happen when WebRender is in use." bit? WebRender is always enabled now.

pcwalton · 2017-01-10T17:19:16Z

components/gfx/display_list/mod.rs

-        }
-        self.hit_test_contents(traversal, &translated_point, client_point, scroll_offsets, result);
-    }
-
    fn hit_test_stacking_context<'a>(&self,


Could you add a comment explaining that this function translates the point to account for the scroll offset?

I'm not sure if get which function you're referring to? While adopting to your comments if removed hit_test_stacking_context to make it more readable.

fiji-flo · 2017-01-10T22:03:12Z

@jdm thanks for pushing
@pcwalton thanks for the feedback

I adopted my changes to the feedback, rebased and squashed.

pcwalton

This is looking pretty good, just a couple more nits

pcwalton · 2017-01-11T00:15:30Z

components/gfx/text/glyph.rs

+            }
+            if current_advance > advance {
+                break;
+            } else {


nit: No need for else after break.

pcwalton · 2017-01-11T00:15:44Z

components/gfx/text/glyph.rs

@@ -547,6 +547,25 @@ impl<'a> GlyphStore {
    }

    #[inline]
+    pub fn range_index_of_advance(&self, range: &Range<ByteIndex>, advance: Au, extra_word_spacing: Au) -> (usize, Au) {


Please document what the return values of this function are in a comment.

While describing what this function returns I realized that it's more intuitive if it returns (index, current_advance) instead of (index, advance -current_advance). This just needed a slight modification in the part that invokes this.
I'll add some comments after I push the modifications.

pcwalton · 2017-01-11T00:16:21Z

components/gfx/text/text_run.rs

+                let (new_index, new_remaining) =
+                    slice.glyphs.range_index_of_advance(&slice.range, remaining, self.extra_word_spacing);
+                remaining = new_remaining;
+                new_index + index


Isn't there a sum method on iterators?

I guess you mean doing a map and sum which would indeed look nicer. That's just a habit from my early haskell days.

fiji-flo · 2017-01-11T08:05:16Z

Thanks for the feedback. Adopted and squashed.

fiji-flo · 2017-01-11T08:09:36Z

components/gfx/text/text_run.rs

+            .map(|slice| {
+                let (slice_index, slice_advance) =
+                    slice.glyphs.range_index_of_advance(&slice.range, remaining, self.extra_word_spacing);
+                remaining -= slice_advance;


@pcwalton glyphs.range_index_of_advance now returns (index, advanced) instead of (index, remaining) to make it more intuitive. Therefore we need to update remaining here.

pcwalton · 2017-01-11T21:40:54Z

@bors-servo: r+

bors-servo · 2017-01-11T21:40:54Z

📌 Commit b40db5b has been approved by pcwalton

bors-servo · 2017-01-11T22:19:12Z

⌛ Testing commit b40db5b with merge 68a8e1b...

Position insertion point in input field with mouse  Implements cursor positioning in input elements (text/password) via mouse. The related issue is #10083 but is only covered partly. This PR does **not** cover: * positioning in textarea elements via mouse * text selection in input/textarea elements via mouse ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #10083 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because I can't think of a way to test mouse interaction in the current test pipeline.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14291)

bors-servo · 2017-01-11T23:32:42Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

paulrouget · 2017-01-16T12:35:49Z

This caused a regression: #15015

highfive assigned nox Nov 20, 2016

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

highfive added the S-needs-rebase There are merge conflict errors. label Dec 6, 2016

fiji-flo force-pushed the text-input-select branch from 244bc44 to b130521 Compare December 7, 2016 07:55

KiChjang removed the S-needs-rebase There are merge conflict errors. label Dec 7, 2016

highfive assigned pcwalton and unassigned nox Dec 16, 2016

emilio reviewed Dec 16, 2016

View reviewed changes

fiji-flo force-pushed the text-input-select branch from b130521 to 7c08848 Compare December 17, 2016 15:59

fiji-flo force-pushed the text-input-select branch from 7c08848 to 6f913e7 Compare January 5, 2017 07:03

highfive added the S-needs-rebase There are merge conflict errors. label Jan 10, 2017

pcwalton reviewed Jan 10, 2017

View reviewed changes

jdm 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 Jan 10, 2017

fiji-flo force-pushed the text-input-select branch from 6f913e7 to fee9d1f Compare January 10, 2017 22:01

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 Jan 10, 2017

jdm removed the S-needs-rebase There are merge conflict errors. label Jan 11, 2017

pcwalton reviewed Jan 11, 2017

View reviewed changes

Position insertion point in input field with mouse

b40db5b

fiji-flo force-pushed the text-input-select branch from fee9d1f to b40db5b Compare January 11, 2017 08:04

fiji-flo commented Jan 11, 2017

View reviewed changes

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 Jan 11, 2017

bors-servo merged commit b40db5b into servo:master Jan 11, 2017

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

This was referenced Jan 11, 2017

Remove network requests from image cache thread #14962

Merged

Subsequent step for implementing HTML form validation #14566

Closed

paulrouget mentioned this pull request Jan 16, 2017

Cannot click on anything #15015

Closed

This was referenced Jan 16, 2017

Don't translate point for independent stacking contexts #15049

Closed

Fix input regressione emilio/servo#2

Merged

Position insertion point in input field with mouse #14291

Position insertion point in input field with mouse #14291

Conversation

fiji-flo commented Nov 20, 2016 • edited by larsbergstrom

highfive commented Nov 20, 2016

highfive commented Nov 20, 2016

bors-servo commented Dec 6, 2016

jdm commented Dec 6, 2016

fiji-flo commented Dec 7, 2016

jdm commented Dec 15, 2016

nox commented Dec 16, 2016

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiji-flo commented Dec 17, 2016

fiji-flo commented Jan 5, 2017

bors-servo commented Jan 10, 2017

jdm commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiji-flo commented Jan 10, 2017

pcwalton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiji-flo commented Jan 11, 2017

Choose a reason for hiding this comment

pcwalton commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

paulrouget commented Jan 16, 2017

fiji-flo commented Nov 20, 2016 •

edited by larsbergstrom