display input caret for textarea. fixes #7758 #7761

fiji-flo · 2015-09-27T18:14:47Z

This adds the input caret for textareas. Although, it does not handle multiline textareas correctly. The caret gets displayed for each line.

I'll look into that but that will take more time. Some feedback on this small patch would be appreciated though.

highfive · 2015-09-27T18:14:50Z

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

highfive · 2015-09-27T18:14:51Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

nox · 2015-09-27T18:32:25Z

./components/layout/wrapper.rs:35: use statement is not in alphabetical order
    expected: data::{LayoutDataFlags, LayoutDataWrapper, PrivateLayoutData}
    found: std::str::CharIndices
./components/layout/wrapper.rs:36: use statement is not in alphabetical order
    expected: gfx::display_list::OpaqueNode
    found: data::{LayoutDataFlags, LayoutDataWrapper, PrivateLayoutData}
./components/layout/wrapper.rs:37: use statement is not in alphabetical order
    expected: gfx::text::glyph::CharIndex
    found: gfx::display_list::OpaqueNode
./components/layout/wrapper.rs:38: use statement is not in alphabetical order
    expected: incremental::RestyleDamage
    found: gfx::text::glyph::CharIndex
./components/layout/wrapper.rs:39: use statement is not in alphabetical order
    expected: ipc_channel::ipc::IpcSender
    found: incremental::RestyleDamage
./components/layout/wrapper.rs:40: use statement is not in alphabetical order
    expected: msg::constellation_msg::{PipelineId, SubpageId}
    found: ipc_channel::ipc::IpcSender
./components/layout/wrapper.rs:41: use statement is not in alphabetical order
    expected: opaque_node::OpaqueNodeMethods
    found: msg::constellation_msg::{PipelineId, SubpageId}
./components/layout/wrapper.rs:42: use statement is not in alphabetical order
    expected: script::dom::attr::AttrValue
    found: opaque_node::OpaqueNodeMethods
./components/layout/wrapper.rs:43: use statement is not in alphabetical order
    expected: script::dom::bindings::codegen::InheritTypes::{CharacterDataCast, ElementCast}
    found: script::dom::attr::AttrValue
./components/layout/wrapper.rs:44: use statement is not in alphabetical order
    expected: script::dom::bindings::codegen::InheritTypes::{HTMLCanvasElementCast, HTMLIFrameElementCast}
    found: script::dom::bindings::codegen::InheritTypes::{CharacterDataCast, ElementCast}
./components/layout/wrapper.rs:45: use statement is not in alphabetical order
    expected: script::dom::bindings::codegen::InheritTypes::{HTMLImageElementCast, HTMLInputElementCast}
    found: script::dom::bindings::codegen::InheritTypes::{HTMLCanvasElementCast, HTMLIFrameElementCast}
./components/layout/wrapper.rs:46: use statement is not in alphabetical order
    expected: script::dom::bindings::codegen::InheritTypes::{HTMLTextAreaElementCast, NodeCast, TextCast}
    found: script::dom::bindings::codegen::InheritTypes::{HTMLImageElementCast, HTMLInputElementCast}
./components/layout/wrapper.rs:47: use statement is not in alphabetical order
    expected: script::dom::bindings::js::LayoutJS
    found: script::dom::bindings::codegen::InheritTypes::{HTMLTextAreaElementCast, NodeCast, TextCast}
./components/layout/wrapper.rs:48: use statement is not in alphabetical order
    expected: script::dom::characterdata::{CharacterDataTypeId, LayoutCharacterDataHelpers}
    found: script::dom::bindings::js::LayoutJS
./components/layout/wrapper.rs:49: use statement is not in alphabetical order
    expected: script::dom::element::{Element, ElementTypeId}
    found: script::dom::characterdata::{CharacterDataTypeId, LayoutCharacterDataHelpers}
./components/layout/wrapper.rs:50: use statement is not in alphabetical order
    expected: script::dom::element::{LayoutElementHelpers, RawLayoutElementHelpers}
    found: script::dom::element::{Element, ElementTypeId}
./components/layout/wrapper.rs:51: use statement is not in alphabetical order
    expected: script::dom::htmlcanvaselement::LayoutHTMLCanvasElementHelpers
    found: script::dom::element::{LayoutElementHelpers, RawLayoutElementHelpers}
./components/layout/wrapper.rs:52: use statement is not in alphabetical order
    expected: script::dom::htmlelement::HTMLElementTypeId
    found: script::dom::htmlcanvaselement::LayoutHTMLCanvasElementHelpers
./components/layout/wrapper.rs:53: use statement is not in alphabetical order
    expected: script::dom::htmlimageelement::LayoutHTMLImageElementHelpers
    found: script::dom::htmlelement::HTMLElementTypeId
./components/layout/wrapper.rs:54: use statement is not in alphabetical order
    expected: script::dom::htmlinputelement::{HTMLInputElement, LayoutHTMLInputElementHelpers}
    found: script::dom::htmlimageelement::LayoutHTMLImageElementHelpers
./components/layout/wrapper.rs:55: use statement is not in alphabetical order
    expected: script::dom::htmltextareaelement::LayoutHTMLTextAreaElementHelpers
    found: script::dom::htmlinputelement::{HTMLInputElement, LayoutHTMLInputElementHelpers}
./components/layout/wrapper.rs:56: use statement is not in alphabetical order
    expected: script::dom::node::{HAS_CHANGED, HAS_DIRTY_DESCENDANTS, HAS_DIRTY_SIBLINGS, IS_DIRTY}
    found: script::dom::htmltextareaelement::LayoutHTMLTextAreaElementHelpers
./components/layout/wrapper.rs:57: use statement is not in alphabetical order
    expected: script::dom::node::{LayoutNodeHelpers, SharedLayoutData}
    found: script::dom::node::{HAS_CHANGED, HAS_DIRTY_DESCENDANTS, HAS_DIRTY_SIBLINGS, IS_DIRTY}
./components/layout/wrapper.rs:58: use statement is not in alphabetical order
    expected: script::dom::node::{Node, NodeTypeId}
    found: script::dom::node::{LayoutNodeHelpers, SharedLayoutData}
./components/layout/wrapper.rs:59: use statement is not in alphabetical order
    expected: script::dom::text::Text
    found: script::dom::node::{Node, NodeTypeId}
./components/layout/wrapper.rs:60: use statement is not in alphabetical order
    expected: selectors::matching::DeclarationBlock
    found: script::dom::text::Text
./components/layout/wrapper.rs:61: use statement is not in alphabetical order
    expected: selectors::parser::{AttrSelector, NamespaceConstraint}
    found: selectors::matching::DeclarationBlock
./components/layout/wrapper.rs:62: use statement is not in alphabetical order
    expected: smallvec::VecLike
    found: selectors::parser::{AttrSelector, NamespaceConstraint}
./components/layout/wrapper.rs:63: use statement is not in alphabetical order
    expected: std::borrow::ToOwned
    found: smallvec::VecLike
./components/layout/wrapper.rs:64: use statement is not in alphabetical order
    expected: std::cell::{Ref, RefMut}
    found: std::borrow::ToOwned
./components/layout/wrapper.rs:65: use statement is not in alphabetical order
    expected: std::marker::PhantomData
    found: std::cell::{Ref, RefMut}
./components/layout/wrapper.rs:66: use statement is not in alphabetical order
    expected: std::mem
    found: std::marker::PhantomData
./components/layout/wrapper.rs:67: use statement is not in alphabetical order
    expected: std::str::CharIndices
    found: std::mem

nox · 2015-09-27T18:33:03Z

You so fast.

fiji-flo · 2015-09-28T06:16:14Z

@pcwalton should I put that into a new file or is there a suitable place for a new trait like this? I didn't spot one. I would include get_value_for_layout in that trait, too.

I'll take a look where to put the search_insertion_point routine or if I can just use an existing one.

Thanks for the input.

pcwalton · 2015-09-28T17:13:32Z

@fiji-flo components/util/str.rs looks like a good place to me.

fiji-flo · 2015-09-28T19:34:03Z

@pcwalton I created a search_index function in components/util/str.rs and removed search_insertion_point

I'm having a hard time figuring out where to put a unified trait for text area and input element. I did not find something similar in components/script/dom. Basically all files represent a single dom element or part of a dom element. Should I just create a htmltextinputhelpers.rs and put it there?

Thanks again for your input here.

bors-servo · 2015-09-29T22:59:16Z

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

fiji-flo · 2015-09-30T06:25:55Z

With the latest changes from master get_insertion_point_for_layout from textarea and input element have diverged. The one from input element also checks if it is a text or password input and returns an Option. This would be unnecessary for textarea.

@pcwalton maybe they just don't belong into the same trait. Also, I noticed that the input caret is displayed in each line when you add newlines in the text area. Should I solve this in this PR or rather keep this small and solve it as a new issue?

jdm · 2015-09-30T13:26:07Z

Let's keep this PR small and solve the multiline issue in another one. I agree that I don't see much cause for sharing the code at this point.

pcwalton · 2015-09-30T21:57:26Z

Yeah, let's keep them separate and avoid the trait.

fiji-flo · 2015-09-30T22:21:29Z

@pcwalton I've updated and squashed down. Is there something missing? And as I said. I'll work on the followup issue.

fiji-flo · 2015-10-04T16:31:57Z

I'm hunting down the multiline problem I saw. This should be not so hard to solve so postpone the review as some more changes are coming.

fiji-flo · 2015-10-10T20:51:05Z

The last commits should take care of the multiline problem I was observing. @pcwalton can I have some feedback on this?

bors-servo · 2015-10-21T15:07:33Z

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

jdm · 2015-10-21T15:13:46Z

I'll poke @pcwalton about this today.

pcwalton · 2015-10-21T16:40:24Z

Looks good to me. Rebase and I'll r+. Sorry this took so long.

fiji-flo · 2015-10-21T20:41:16Z

@pcwalton rebased and squashed down

jdm · 2015-10-21T20:46:37Z

@bors-servo: r=pcwalton
Thanks!

bors-servo · 2015-10-21T20:46:37Z

📌 Commit 80e8a67 has been approved by pcwalton

bors-servo · 2015-10-21T21:23:53Z

⌛ Testing commit 80e8a67 with merge af6a64e...

display input caret for textarea. fixes #7758 This adds the input caret for textareas. Although, it does not handle multiline textareas correctly. The caret gets displayed for each line. I'll look into that but that will take more time. Some feedback on this small patch would be appreciated though.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7761)

bors-servo · 2015-10-21T22:02:19Z

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

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 27, 2015

nox added the S-fails-tidy `./mach test-tidy` reported errors. label Sep 27, 2015

nox removed the S-fails-tidy `./mach test-tidy` reported errors. label Sep 27, 2015

nox added A-layout/uncategorized A-forms labels Sep 28, 2015

highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 29, 2015

mbrubeck assigned pcwalton Sep 30, 2015

fiji-flo force-pushed the input_caret branch from 159e2f8 to fa3856b Compare September 30, 2015 22:14

fiji-flo force-pushed the input_caret branch from e3e15aa to f554b2e Compare October 10, 2015 20:00

highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 21, 2015

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 21, 2015

display input caret for textarea. fixes servo#7758

80e8a67

fiji-flo force-pushed the input_caret branch from f7dde87 to 80e8a67 Compare October 21, 2015 20:39

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 21, 2015

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. S-needs-rebase There are merge conflict errors. labels Oct 21, 2015

bors-servo merged commit 80e8a67 into servo:master Oct 21, 2015

fiji-flo deleted the input_caret branch October 21, 2015 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

display input caret for textarea. fixes #7758 #7761

display input caret for textarea. fixes #7758 #7761

fiji-flo commented Sep 27, 2015

highfive commented Sep 27, 2015

highfive commented Sep 27, 2015

nox commented Sep 27, 2015

nox commented Sep 27, 2015

fiji-flo commented Sep 28, 2015

pcwalton commented Sep 28, 2015

fiji-flo commented Sep 28, 2015

bors-servo commented Sep 29, 2015

fiji-flo commented Sep 30, 2015

jdm commented Sep 30, 2015

pcwalton commented Sep 30, 2015

fiji-flo commented Sep 30, 2015

fiji-flo commented Oct 4, 2015

fiji-flo commented Oct 10, 2015

bors-servo commented Oct 21, 2015

jdm commented Oct 21, 2015

pcwalton commented Oct 21, 2015

fiji-flo commented Oct 21, 2015

jdm commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

display input caret for textarea. fixes #7758 #7761

display input caret for textarea. fixes #7758 #7761

Conversation

fiji-flo commented Sep 27, 2015

highfive commented Sep 27, 2015

highfive commented Sep 27, 2015

nox commented Sep 27, 2015

nox commented Sep 27, 2015

fiji-flo commented Sep 28, 2015

pcwalton commented Sep 28, 2015

fiji-flo commented Sep 28, 2015

bors-servo commented Sep 29, 2015

fiji-flo commented Sep 30, 2015

jdm commented Sep 30, 2015

pcwalton commented Sep 30, 2015

fiji-flo commented Sep 30, 2015

fiji-flo commented Oct 4, 2015

fiji-flo commented Oct 10, 2015

bors-servo commented Oct 21, 2015

jdm commented Oct 21, 2015

pcwalton commented Oct 21, 2015

fiji-flo commented Oct 21, 2015

jdm commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015