Omega Feng [:Omega] [:馮於懋] Reporter
	Description • 10 years ago

[Steps To Reproduce]
1. Launch Contacts app
2. Tap "+"
3. Tap "Name" field

[Actual Result]
It shows cursor and caret.

[Expected Result]
According to the spec (p.15) in bug 921965, it should show cursor only.

Device: Flame
OS Version: 2.1.0.0-prerelease
Platform Version: 34.0a1
Build Identifier: 20140825160203
Git Commit Info: 2014-08-26 03:53:24 (24799285)

	howie [:howie]
	Comment 1 • 10 years ago

Hi Peter, could you take a look first? This is something we need to adjust after, thanks.

	Peter Chang[:pchang]
	Comment 2 • 10 years ago

TY, please help to check it.

	Peter Chang[:pchang]
	Comment 3 • 10 years ago

Another idea is to combine the condition of 'focus' and 'input element is touched' and then display touchcaret if above condition meets.

	Peter Chang[:pchang]
	Comment 4 • 10 years ago

(In reply to Omega Feng [:Omega] [:馮於懋] from comment #0)
> [Steps To Reproduce]
> 1. Launch Contacts app
> 2. Tap "+"
> 3. Tap "Name" field
> 
Omega, there is no focus before step 3. Therefore, it is reasonable to show touchcaret after step 3 because it has focus.
> [Actual Result]
> It shows cursor and caret.
> 
> [Expected Result]
> According to the spec (p.15) in bug 921965, it should show cursor only.
> 
> Device: Flame
> OS Version: 2.1.0.0-prerelease
> Platform Version: 34.0a1
> Build Identifier: 20140825160203
> Git Commit Info: 2014-08-26 03:53:24 (24799285)

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 5 • 10 years ago

After offline discussion, we still think it's important to show only cursor when focusing an unfocused input. Please help evaluate the effort and if there are any other use cases have to be defined, thanks!

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 6 • 10 years ago

Hi Omega,

Would you please provide pros and cons for showing only cursor (caret) when focusing an unfocused input? 

Currently, we show cursor (caret) and touch caret simultaneously when focusing an unfocused input. The pros I could think of are:
1) When the input already contains text, the user could quickly move the cursor to the desired position by moving touch caret without a second click. Since the screen of mobile device is small, it's easy that user fail to click the the desired position the first time.
2) The implementation is simpler. We do not need to track whether each input fields has been focused or not. We could sync with the visibility of the cursor (caret) when focusing.

Cons might be:
1) The text below the input field might be covered for three seconds or until the user start typing.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 7 • 10 years ago

Hi Ting-Yu,
I think you've already listed the same things (or the opposite ones) as what you want me to do, so I'll skip this part and explain more about the reasons.
We think cursor movement is an important but secondary feature since typing is the primary one. User doesn't need touch caret every first time focusing an unfocused input, but covering other content happens every time. That's why we decide not to show the touch caret at that moment.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 9 • 10 years ago

Ehsan, 

Before hacking this bug, it will be great to hear your opinions about showing no touch caret when focusing an unfocused input. Technically, this behavior makes the display of caret and touch caret inconsistent and some existing test cases might need to be fixed.

	(no longer active)
	Comment 10 • 10 years ago

The UX spec is wrong here, I think, and we need to fix that.  There is no "typing mode" or "cursor mode" on the Web.  Either the editable element has the focus or it doesn't, there is simply no third state.  Also, the second paragraph of comment 7 is misguided.  Typing and moving the caret are part of the same operation.  Let me bring an example:

+--------------------+
| Content in textbox | scrolls past the bounds of the control
+--------------------+

Consider for example that I want to fix the contents of this textbox by adding a "that" after "textbox".  The current UX spec makes doing that extremely difficult unless my fat fingers happen to click right after the second "x" in "textbox" by accident.  Tapping anywhere else means that I will have a very difficult time typing what I want.

In the future, I would really urge the UX folks to consider whether the concepts in their design can be mapped to the features of the Web platform.  That should help with similar cases where our UX spec is based on a mode that doesn't exist on the Web.

WONTFIXing the bug based on the above.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 11 • 10 years ago

For the case in comment 10, I don't think it's extremely difficult to tap at the right position. If you meant to do it, the possibility is 50% or more. For example, you have text "AB" and want to insert something between A and B, you have 50% chance to tap at the right position as the followings:
   |       |
  A|A   BBB|BB
 A | A  B  |  B
AAA|AAA BBB|BB
A  |  A B  |  B
A  |  A BBB|BB
   |       |
 X |   O   | X

Since it's a 50-50 situation, we decided not to bother users at the first time. By the way, the current design is same as Fennec's behavior. I agreed there are no "typing mode" or "cursor mode" on the Web, but considering it's a touch environment, "typing mode" or "cursor mode" should be separated in the system level.

	(no longer active)
	Comment 12 • 10 years ago

(In reply to Omega Feng [:Omega] [:馮於懋] from comment #11)
> For the case in comment 10, I don't think it's extremely difficult to tap at
> the right position. If you meant to do it, the possibility is 50% or more.
> For example, you have text "AB" and want to insert something between A and
> B, you have 50% chance to tap at the right position as the followings:
>    |       |
>   A|A   BBB|BB
>  A | A  B  |  B
> AAA|AAA BBB|BB
> A  |  A B  |  B
> A  |  A BBB|BB
>    |       |
>  X |   O   | X

The issue is that for most text sizes rendered on the device, one's fingers is _way_ bigger than these letters, so it's not a matter of either tapping on the "X" or "O" sections above, but tapping the "O" section versus a much larger area surrounding it, so I don't think this is a 50-50% situation.  And just testing this casually on a device, I can't get anywhere close to a 50% correct tap.

> Since it's a 50-50 situation, we decided not to bother users at the first
> time. By the way, the current design is same as Fennec's behavior.

The Fennec behavior here is just buggy.  The native Android behavior is to always display the touch caret when tapping a filed that is not empty, which I think makes a lot of sense.

(In general, please note that the code for the selection/touch caret code on Fennec is super buggy, so I wouldn't necessarily take anything that Fennec does there at face value. :-)

> I agreed
> there are no "typing mode" or "cursor mode" on the Web, but considering it's
> a touch environment, "typing mode" or "cursor mode" should be separated in
> the system level.

I disagree.  As I said before, I don't think that the user just wants to type something in without caring about where the caret goes, as the result of the typing would be different than what you would expect if the caret is at the wrong location.  Thinking more about this, the only case where you actually don't care where the caret goes is if the text field is empty (and I think that only makes sense for input/textarea, not for contenteditable), and this seems to be the Android's default behavior as well.  I'd be fine with doing that, because it's very clear what an empty text control means.

But if you insist on adding a third mode, we need to precisely define it in a way that makes sure it interacts well with other parts of the platform, such as the focus model, focusing/blurring a field programmatically and/or through manual interaction, and also specify how this is remembered (for example what happens if you remove an element that is in "typing mode" and reinsert it elsewhere in the document, or what happens when you switch back to a tab which had an element in "typing mode"), how to handle readonly text fields where you may want to allow moving the caret for selection purposes but do not want to permit typing, etc.  Without all of these details hashed out, we cannot implement this idea in Gecko.  Note that because these notions do not map to anything sensible on the Web, there would be a lot of details to figure out and they will most likely interact very poorly with everything else, so I really don't want to go there, but if you disagree, I would like to see a full proposal with all of these details.  We definitely need to make sure that such a proposal works well with all of the Web content that has not been written specifically for Firefox OS.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 13 • 10 years ago

I've synced with Fennec UX designer Yuan and she seemed to agree the attached proposal here. Basically the changes are focusing on an empty input doesn't show the touch caret, but focusing on an input with text shows the touch caret.
@Yuan, could you confirm this, thanks!

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 14 • 10 years ago

Ehsan, how do you think about our new proposal in comment 13?

	(no longer active)
	Comment 15 • 10 years ago

(In reply to Omega Feng [:Omega] [:馮於懋] (please ni?) from comment #14)
> Ehsan, how do you think about our new proposal in comment 13?

That's basically what I suggested in comment 12 for the most part, so I agree with the general approach.  :-)

There is one case that is not clearly reflected in the spec though, and that is about programmatic changes to the selection/focus.  If those are treated the same as user initiated actions (for example, if calling .focus() on a non-focused input box is considered the same as tapping it) then this looks good to me.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 16 • 10 years ago

This is a WIP patch, and should have a reftest.

Try result (some test cases failed):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2e9295c937a2

	(no longer active)
	Comment 17 • 10 years ago

Comment on attachment 8510295 [details] [diff] [review]
Do not show touch caret if content is empty. (v1)

Review of attachment 8510295 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/TouchCaret.cpp
@@ +426,5 @@
>      return false;
>    }
>  
> +  dom::Element* editingHost = focusFrame->GetContent()->GetEditingHost();
> +  if (editingHost && !nsContentUtils::HasNonEmptyTextContent(editingHost)) {

This will only work correctly for designMode and contenteditable, not for input and textarea.  You probably need to add a special case to handle those elements.  To do that, you can QI the element to nsITextControlElement, use IsSingleLineTextControl and IsTextArea to determine what type of control you're dealing with, and then use GetTextEditorValue to get the value of the control, considering the edited value, etc.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 18 • 10 years ago

I've done some experiment, and found that using GetEditingHost() works for <input> and <textarea>.

Consider a <input> with value "abc". The frame tree contains:

nsTextControlFrame@13eed0c30 next=13eed1c98 {0,720,7740,1290} vis-overflow=-120,-120,7980,1530 scr-overflow=0,0,7740,1290 [state=0000004000004230] [content=129c82ca0] [sc=13eed03f8]<
  HTMLScroll(div)(-1)@13eed1358 {180,180,7380,930} [state=0000000000084038] [content=12c748420] [sc=13eed0f68]<
    Block(div)(-1)@13eed1950 {0,0,7380,930} [state=0000100000d04230] [content=12c748420] [sc=13eed1678:-moz-scrolled-content]<
      line 13eed1c48: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {60,60,1117,810} vis-overflow=30,60,1177,810 scr-overflow=60,60,1117,810 <
        Text(0)"abc"@13eed1bd8 {60,60,1117,810} vis-overflow=-30,0,1177,810 scr-overflow=0,0,1117,810 [state=80000040a0604010] [content=1294c9670] [sc=13eed1a90:-moz-non-element,parent=13eed0f68] [run=1191ed620][0,3,T] 
      >
    >
  >
>

editingHost is content=12c748420, and nsContentUtils::HasNonEmptyTextContent() will iterate over the editingHost's children, and found the Text node content=1294c9670 is non-empty. It does work for <input> here.

	(no longer active)
	Comment 19 • 10 years ago

Oh, OK then!  Please add tests covering all of these cases. :-)

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 20 • 10 years ago

We should sync touch caret's visibility with caret every time since
touch caret might be hidden due to timeout while caret is enabled. Also
remove dead code.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 21 • 10 years ago

This will be used in part 3.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 22 • 10 years ago

We want HasNonEmptyTextContent() to descend recursively into editingHost
since <div contenteditable="true"><span>123</span></div> should be
considered as non-empty.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 24 • 10 years ago

These tests all involving focusing on an empty element. Touch caret will
not show under the new touch caret UI spec. Therefore, I fix them by
disabling touch caret when running those tests.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 25 • 10 years ago

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=98cf3eaed20d

	(no longer active)
	Comment 26 • 10 years ago

Comment on attachment 8512635 [details] [diff] [review]
Part 2 - Make HasNonEmptyTextContent() descend recursively.

Review of attachment 8512635 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentUtils.h
@@ +1248,2 @@
>     */
> +  static bool HasNonEmptyTextContent(nsINode* aNode, bool aRecursive = false);

Nit: please add an enum like:

enum TextContentDiscoverMode { eRecurseIntoChildren, eDontRecurseIntoChildren };

And use that enum instead of the boolean argument, to make it clearer at the call site what the thing you're passing into the function means.

	(no longer active)
	Comment 27 • 10 years ago

Comment on attachment 8512636 [details] [diff] [review]
Part 3 - Do not show touch caret when content is empty.

Review of attachment 8512636 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/TouchCaret.cpp
@@ +428,5 @@
>    }
>  
> +  dom::Element* editingHost = focusFrame->GetContent()->IsEditable()
> +                                ? focusFrame->GetContent()->GetEditingHost()
> +                                : nullptr;

Just call GetEditingHost unconditionally.  It will return nullptr if the node is not editable.

	(no longer active)
	Comment 28 • 10 years ago

Comment on attachment 8512637 [details] [diff] [review]
Part 4 - Add test_touchcaret_visibility.html.

Review of attachment 8512637 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 29 • 10 years ago

Addressed comment 26 by adding enum TextContentDiscoverMode.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 30 • 10 years ago

Addressed comment 27 and update calling of HasNonEmptyTextContent due to change
in part 2.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 31 • 10 years ago

All platform build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0efe1cd3421e
Tests for B2G: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=98cf3eaed20d

	Carsten Book [:Tomcat]
	Comment 32 • 10 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/34b56f3f0cbc
https://hg.mozilla.org/integration/b2g-inbound/rev/0d436b2b5651
https://hg.mozilla.org/integration/b2g-inbound/rev/cb5055d28392
https://hg.mozilla.org/integration/b2g-inbound/rev/8f7d181a5e2c
https://hg.mozilla.org/integration/b2g-inbound/rev/442f9ab1d578

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/34b56f3f0cbc
https://hg.mozilla.org/mozilla-central/rev/0d436b2b5651
https://hg.mozilla.org/mozilla-central/rev/cb5055d28392
https://hg.mozilla.org/mozilla-central/rev/8f7d181a5e2c
https://hg.mozilla.org/mozilla-central/rev/442f9ab1d578

	Peter Chang[:pchang]
	Comment 34 • 10 years ago

Omega, with this path landed, it impacts the shortcut of touchcaret because there is no touch caret if no input. It is a kind of UX conflict, any comment?

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 35 • 10 years ago

Correct, it doesn't need paste shortcut of touch caret. However, user should be able to trigger touch caret and paste bubble by long pressing.

	Peter Chang[:pchang]
	Comment 36 • 10 years ago

(In reply to Omega Feng [:Omega] [:馮於懋] (please ni?) from comment #35)
> Correct, it doesn't need paste shortcut of touch caret. However, user should
> be able to trigger touch caret and paste bubble by long pressing.

Is this in UX spec? I didn't see we could trigger touch caret by long pressing.

	Omega Feng [:Omega] [:馮於懋] Reporter
	Comment 37 • 10 years ago

Yes, it's defined in p.11 of "FxOS 2.1 UX Spec_Text Selection and Clipboard_v1.2.pdf". (It's about password input, but still can be applied to other inputs.)