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.


        display input caret for textarea. fixes #7758

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...


        Auto merge of #7761 - fiji-flo:input_caret, r=pcwalton

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 label Sep 27, 2015

nox added the S-fails-tidy label Sep 27, 2015

nox removed the S-fails-tidy label Sep 27, 2015

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

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 29, 2015

mbrubeck assigned pcwalton Sep 30, 2015

fiji-flo force-pushed the fiji-flo:input_caret branch from 159e2f8 to fa3856b Sep 30, 2015

fiji-flo force-pushed the fiji-flo:input_caret branch from e3e15aa to f554b2e Oct 10, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 21, 2015

jdm removed the S-awaiting-review label Oct 21, 2015

display input caret for textarea. fixes #7758

Loading status checks…

80e8a67

fiji-flo force-pushed the fiji-flo:input_caret branch from f7dde87 to 80e8a67 Oct 21, 2015

highfive added the S-awaiting-review label Oct 21, 2015

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Oct 21, 2015

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

fiji-flo deleted the fiji-flo:input_caret branch Oct 21, 2015

servo / servo

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

servo / servo

Join GitHub today

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