EditableText underlines the word being edited

EditableText underlines the word being edited

Also restored the blue line that appears below
focused Input text.

R=ianh@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/d86a57c8303c328e7c2a8d8ffdf61c04c27033d1

[hansmuller, 2015-06-19 22:20:25]

hansmuller@chromium.org changed reviewers:
+ abarth@chromium.org

[hansmuller, 2015-06-19 22:22:27]

hansmuller@chromium.org changed reviewers:
+ hansmuller@chromium.org

[hansmuller, 2015-06-19 22:22:28]

PTAL

[Hixie, 2015-06-19 22:39:43]

ianh@google.com changed reviewers:
+ ianh@google.com

[Hixie, 2015-06-19 22:39:44]

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/edi...
File sky/sdk/lib/editing/editable_text.dart (right):

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/edi...
sky/sdk/lib/editing/editable_text.dart:23: static const TextStyle _plainStyle =
const TextStyle();
Can't this be null?

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
File sky/sdk/lib/editing/input.dart (left):

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:32: //   white-space: pre;
did white-space:pre get carried over?

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
File sky/sdk/lib/editing/input.dart (right):

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:59: List<Widget> textChildren = [];
<Widget>[].

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:61: textChildren.add(new Container(
What is the Container() for?

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:66: new EditableText(value: _editableValue,
focused: focused));
If you put a line break after a (, but a line break before its corresponding ).

[abarth-chromium, 2015-06-19 22:41:37]

abarth@chromium.org changed reviewers:
- ianh@google.com

[abarth-chromium, 2015-06-19 22:41:38]

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/edi...
File sky/sdk/lib/editing/editable_text.dart (right):

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/edi...
sky/sdk/lib/editing/editable_text.dart:24: static const TextStyle
_composingStyle = const TextStyle(decoration: underline);
I would just inline these in the one place they're each used.  The |const| will
mean we only actually allocate them at compile time.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
File sky/sdk/lib/editing/input.dart (right):

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:71: constraints: new BoxConstraints(minWidth:
double.INFINITY)
You might as well just write:

width: double.INFINITY

But rather than do this, see below.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:78: decoration: new
BoxDecoration(backgroundColor: focusColor)
Why not use a bottom border to draw this line?  That's much cheaper than
creating an entire entry in the render tree.

[hansmuller, 2015-06-19 23:32:12]

Please give this another look.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/edi...
File sky/sdk/lib/editing/editable_text.dart (right):

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/edi...
sky/sdk/lib/editing/editable_text.dart:23: static const TextStyle _plainStyle =
const TextStyle();
On 2015/06/19 22:39:44, Hixie wrote:
> Can't this be null?

Not at the moment (StyledText doesn't support that). Eventually I think this
TextStyle will come from the theme.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/edi...
sky/sdk/lib/editing/editable_text.dart:24: static const TextStyle
_composingStyle = const TextStyle(decoration: underline);
On 2015/06/19 22:41:38, abarth wrote:
> I would just inline these in the one place they're each used.  The |const|
will
> mean we only actually allocate them at compile time.

Done.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
File sky/sdk/lib/editing/input.dart (left):

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:32: //   white-space: pre;
On 2015/06/19 22:39:44, Hixie wrote:
> did white-space:pre get carried over?

Good point. The height and white-space haven't been carried forward.
I'll add a comment.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
File sky/sdk/lib/editing/input.dart (right):

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:59: List<Widget> textChildren = [];
On 2015/06/19 22:39:44, Hixie wrote:
> <Widget>[].

Done.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:61: textChildren.add(new Container(
On 2015/06/19 22:39:44, Hixie wrote:
> What is the Container() for?

Oops, thanks for pointing that out.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:66: new EditableText(value: _editableValue,
focused: focused));
On 2015/06/19 22:39:44, Hixie wrote:
> If you put a line break after a (, but a line break before its corresponding
).

Done.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:71: constraints: new BoxConstraints(minWidth:
double.INFINITY)
On 2015/06/19 22:41:38, abarth wrote:
> You might as well just write:
> 
> width: double.INFINITY
> 
> But rather than do this, see below.

Acknowledged.

https://codereview.chromium.org/1194113002/diff/20001/sky/sdk/lib/editing/inp...
sky/sdk/lib/editing/input.dart:78: decoration: new
BoxDecoration(backgroundColor: focusColor)
On 2015/06/19 22:41:38, abarth wrote:
> Why not use a bottom border to draw this line?  That's much cheaper than
> creating an entire entry in the render tree.

Done.

[Hixie, 2015-06-19 23:43:24]

ianh@google.com changed reviewers:
+ ianh@google.com

[Hixie, 2015-06-19 23:43:24]

lgtm

[abarth-chromium, 2015-06-19 23:48:52]

lgtm2

[hansmuller1, 2015-06-19 23:50:14]

Committed patchset #5 (id:80001) manually as
d86a57c8303c328e7c2a8d8ffdf61c04c27033d1 (presubmit successful).