Closed Bug 1016184 Opened 6 years ago Closed 6 years ago

[Touch Caret] Enable touch caret on B2G and fix the existing test case failures

Categories

(Core :: DOM: Selection, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
b2g-v2.1 --- disabled

People

(Reporter: slee, Assigned: TYLin)

References

(Depends on 1 open bug)

Details

(Whiteboard: [ft:system-platform])

Attachments

(17 files, 15 obsolete files)

12.07 KB, text/plain
Details
49.90 KB, text/plain
Details
2.96 KB, patch
ehsan
: feedback-
Details | Diff | Splinter Review
279 bytes, text/html
Details
9.54 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
3.24 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
255.86 KB, image/png
Details
3.35 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
12.46 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
1.77 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
5.43 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
4.09 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
2.91 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.85 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
908 bytes, patch
TYLin
: review+
Details | Diff | Splinter Review
1.05 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.85 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
Bug 924692 implemented touch caret, but it turns off by default. Some test cases may fail when turning it on. This bug is to turn on Touch Caret and fixing the failure of test cases.
Depends on: 960897
Assignee: nobody → tlin
Test fails are mainly caused by 

1. Ref-testing one page after sending series of synthesize key with a page focusing the editable element. According to UX spec, touch caret will be set invisible when keys are pressed, while touch caret will be shown when we set focus (tap) the editable area, causing reftest failure.

http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/test_bug644768.html

http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/test_reftests_with_caret.html
  [ 'bug240933-1.html' , 'bug240933-1-ref.html' ] ,
  [ 'bug240933-2.html' , 'bug240933-1-ref.html' ] ,

  [ 'bug602141-1.html' , 'bug602141-1-ref.html' ] ,
  [ 'bug602141-2.html' , 'bug602141-2-ref.html' ] ,
  [ 'bug602141-3.html' , 'bug602141-3-ref.html' ] ,
  [ 'bug602141-4.html' , 'bug602141-4-ref.html' ] ,         			 

  [ 'bug646382-1.html' , 'bug646382-1-ref.html' ] ,
  [ 'bug646382-2.html' , 'bug646382-2-ref.html' ] ,
These 9 tests all failed with this problem.

2. Synthesize mouse event on touch caret. 
   This happens with this test case http://dxr.mozilla.org/mozilla-central/source/dom/events/test/test_focus_disabled.html#110. The test synthesizes a mouse event on the element which is just behind the touch caret, expecting it to change focus, but unfortunately mouse clicking the touch caret will make the focus remain on the input element.
1 test fails with this problem.
 
3. Must have something to with synthesize key but the main cause of failure is unsure. 
http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/test_reftests_with_caret.html
  [ 'bug585922.html'   , 'bug585922-ref.html'   ] ,
  [ 'bug664087-1.html' , 'bug664087-1-ref.html' ] ,
  [ 'bug664087-2.html' , 'bug664087-2-ref.html' ] ,
Try server result with b2g touchcaret pref enabled
https://tbpl.mozilla.org/?tree=Try&rev=0d20568ab217
(In reply to Phoebe Chang [:phoebe] from comment #1)
> 3. Must have something to with synthesize key but the main cause of failure
> is unsure. 
> http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/
> test_reftests_with_caret.html
>   [ 'bug585922.html'   , 'bug585922-ref.html'   ] ,
In [1], the test case collapse the selection and move caret to the end. But [2] receives the NotifySelectionChanged with no reason. And it decide not to show the caret. ehsan, could you give me some hints about it?

>   [ 'bug664087-1.html' , 'bug664087-1-ref.html' ] ,
In [3], when textarea.focus() is called, main thread calls to the "done()" function and executes the synthesizeKey("VK_LEFT", {}). It makes caret disappears. After the function ends, nsFocusManager::SetFocus callback again and set the visibility of touch caret to true. Please check the attached backtrace. :smuag, could you give me some feedback? Or could I just disable touch caret in this test case?

>   [ 'bug664087-2.html' , 'bug664087-2-ref.html' ] ,
It should be similar as bug664087-1-ref.html since it calls focus() then do somethings in the onfocus callback.

[1] http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/bug585922-ref.html#16
[2] http://dxr.mozilla.org/mozilla-central/source/layout/base/TouchCaret.cpp#343
[3] http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/bug664087-1-ref.html
Flags: needinfo?(ehsan)
Flags: needinfo?(bugs)
(In reply to StevenLee[:slee] from comment #3)
> Created attachment 8435590 [details]
> bug664087-1-ref-backtrace.txt
> 
> (In reply to Phoebe Chang [:phoebe] from comment #1)
> > 3. Must have something to with synthesize key but the main cause of failure
> > is unsure. 
> > http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/
> > test_reftests_with_caret.html
> >   [ 'bug585922.html'   , 'bug585922-ref.html'   ] ,
> In [1], the test case collapse the selection and move caret to the end. But
> [2] receives the NotifySelectionChanged with no reason. And it decide not to
> show the caret. ehsan, could you give me some hints about it?

Do you have a backtrace?

> >   [ 'bug664087-1.html' , 'bug664087-1-ref.html' ] ,
> In [3], when textarea.focus() is called, main thread calls to the "done()"
> function and executes the synthesizeKey("VK_LEFT", {}). It makes caret
> disappears.

Why?
Flags: needinfo?(ehsan)
Hi ehsan,

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #4)
> (In reply to StevenLee[:slee] from comment #3)
> > >   [ 'bug585922.html'   , 'bug585922-ref.html'   ] ,
> > In [1], the test case collapse the selection and move caret to the end. But
> > [2] receives the NotifySelectionChanged with no reason. And it decide not to
> > show the caret. ehsan, could you give me some hints about it?
> 
> Do you have a backtrace?
I debug the test today and found that the cause is not what I mentioned in my last comment. The focus() function in [1] does not make the visibility of caret to true(it seems only happens on "input". For "textarea", calling focus makes caret visible). Please see the attached backtrace. 
bug585922.html calls enableCaret in [2] and bug585922-ref.html only calls focus function(that does not make caret visible). So it fails

> 
> > >   [ 'bug664087-1.html' , 'bug664087-1-ref.html' ] ,
> > In [3], when textarea.focus() is called, main thread calls to the "done()"
> > function and executes the synthesizeKey("VK_LEFT", {}). It makes caret
> > disappears.
> 
> Why?
That's according to the UX spec, touch caret will be set invisible when keys are pressed.

  [ 'bug664087-1.html' , 'bug664087-1-ref.html' ] ,
  [ 'bug664087-2.html' , 'bug664087-2-ref.html' ] ,

For the failures of these 2 cases is resulting from the calling sequence. For the following code, the calling sequence is
text.focus -> foofocus -> TouchCaret::SetVisibility(true) ->timeout
So that for bug664087-1.html, it calls synthesizeKey in [3] in timeout function that set caret invisible. It's ref-page, bug664087-1-ref.html, which also calls synthesizeKey in [4] but it's in onfocus callback. After the onfocus callback TouchCaret::SetVisibility(true) is called, it sets caret visible. bug664087-2.html has the same problem.

function foo() {
  text.onfocus = function foofocus() {
    setTimeout(function timeout() {}, 0);
  }
  text.focus();
}

For these 2 cases, should we disable them?

[1] http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/bug585922.html#17
[2] http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/bug585922.html#29
[3] http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/bug664087-1.html#19
[4] http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/bug664087-1-ref.html#16
Flags: needinfo?(bugs) → needinfo?(ehsan)
Well, it's absolutely not OK to disable the tests!  If the UX spec says that the caret should not be updated properly when the selection changes, we need to fix the UX spec.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> Well, it's absolutely not OK to disable the tests!  If the UX spec says that
> the caret should not be updated properly when the selection changes, we need
> to fix the UX spec.
So do you mean when users press keys, the caret should be visible(I checked fennec. Its caret appears when I press keys, but chrome doesn't.)?
Flags: needinfo?(ehsan)
(In reply to StevenLee[:slee] from comment #7)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #6)
> > Well, it's absolutely not OK to disable the tests!  If the UX spec says that
> > the caret should not be updated properly when the selection changes, we need
> > to fix the UX spec.
> So do you mean when users press keys, the caret should be visible(I checked
> fennec. Its caret appears when I press keys, but chrome doesn't.)?

Yes, I think the caret should remain visible.
Flags: needinfo?(ehsan)
Caret should be remain visible, but touch caret(teardrop object beneath caret) doesn't have to.
The main reason we need caret is to know the position of key-in character; The main reason to have touch caret is to move "caret" to specific position easier. While key-in a character, IMHO, I don't think we need touch caret at that moment

Ethan/ Steven, I would like to double confirm that you are discussing on the visibility of "caret" or "touch caret"
Flags: needinfo?(slee)
Flags: needinfo?(ehsan)
(In reply to C.J. Ku[:cjku] from comment #9)
> Ethan/ Steven, I would like to double confirm that you are discussing on the
> visibility of "caret" or "touch caret"
In comment 5, I meant "touch caret", sorry for misleading.
Flags: needinfo?(slee)
Yeah I think (hope!) I'm getting the terminology right.  I think the touch caret and the caret need to stay in sync always.
Flags: needinfo?(ehsan)
fyi, on fennec we used to hide the cursor (blinking "caret") on keypress, but in bug 927882 we changed the logic so the caret ("touch caret") follows the "caret" to improve auto-complete / -correction situations ...
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #11)
> Yeah I think (hope!) I'm getting the terminology right.  I think the touch
> caret and the caret need to stay in sync always.
If the "blinking caret" and "touch caret" stay sync, we need change the UX spec. So that when user presses keys, both "blinking caret" and "touch caret" should be displayed. Is that right?
Flags: needinfo?(ehsan)
Hi Carrie,

On comment 11, ehsan said that we should sync the status of "blinking caret" and "touch caret". You are the author of UX spec, what do you think?
Flags: needinfo?(cawang)
Omega is taking over the KB UX. 

Hi Omega, 

Can you help on this one? Thanks!
Flags: needinfo?(cawang) → needinfo?(ofeng)
Blocks: 921964
From UX perspective, we don't want the touch caret visible after typing, to keep the text area clean and provide the better typing experience. I know that's a little different from Fennec, but we still like it this way.
Flags: needinfo?(ofeng)
(In reply to Omega Feng [:Omega] from comment #16)
> From UX perspective, we don't want the touch caret visible after typing, to
> keep the text area clean and provide the better typing experience. I know
> that's a little different from Fennec, but we still like it this way.

Two questions:

1. Where is the UX spec in question?  I need to verify what it exactly says.

2. When do we show the touch caret again with your proposal?  And how is the user supposed to manipulate the caret position when the touch caret is hidden?  For example, if instead of typing hello I type "helo|" (the pipe character is where the caret position is) how would I be able to position the caret between l and o in order to fix my spelling mistake?
Flags: needinfo?(ehsan) → needinfo?(ofeng)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #17)
> Two questions:
> 
> 1. Where is the UX spec in question?  I need to verify what it exactly says.
Here is the UX spec. https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8435794
Actually, it doesn't express the details about this behavior directly. Only some parts imply that:
p.6 Screen 3 and 5
p.7 Screen 3 and 5
p.8 Screen 5
So, we had some discussion, and then decided not to show the caret after typing. When we have conclusion in this bug, I can add more details about in the spec then.

> 2. When do we show the touch caret again with your proposal?  And how is the
> user supposed to manipulate the caret position when the touch caret is
> hidden?  For example, if instead of typing hello I type "helo|" (the pipe
> character is where the caret position is) how would I be able to position
> the caret between l and o in order to fix my spelling mistake?
User can tap the text input again to show to caret, and then drag it to where he wants.
It's one step more than your expectation for sure, but there are always pros and cons, and here we choose the clean typing experience rather than cursor moving convenience.
Flags: needinfo?(ofeng) → needinfo?(ehsan)
Summary: [Touch Caret]enable touch caret on B2G → [Touch Caret] Enable touch caret on B2G
(In reply to Omega Feng [:Omega] from comment #18)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #17)
> > Two questions:
> > 
> > 1. Where is the UX spec in question?  I need to verify what it exactly says.
> Here is the UX spec.
> https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8435794
> Actually, it doesn't express the details about this behavior directly. Only
> some parts imply that:
> p.6 Screen 3 and 5
> p.7 Screen 3 and 5
> p.8 Screen 5
> So, we had some discussion, and then decided not to show the caret after
> typing. When we have conclusion in this bug, I can add more details about in
> the spec then.
> 
> > 2. When do we show the touch caret again with your proposal?  And how is the
> > user supposed to manipulate the caret position when the touch caret is
> > hidden?  For example, if instead of typing hello I type "helo|" (the pipe
> > character is where the caret position is) how would I be able to position
> > the caret between l and o in order to fix my spelling mistake?
> User can tap the text input again to show to caret, and then drag it to
> where he wants.
> It's one step more than your expectation for sure, but there are always pros
> and cons, and here we choose the clean typing experience rather than cursor
> moving convenience.

OK, I'm afraid if we want to implement what the current spec says, we need to extend the existing touch caret to have two modes, one the normal mode (what we have implemented so far) and one the "typing" mode where we don't draw the handles but we still draw the vertical caret line.  In order for us to be able to implement that, we need to know all of the ways that the "typing" caret and "normal" caret can transition into each other.  So far it seems that you want us to draw the "typing" caret as soon as a key press event has been sent (what if the key press event is a directional key such as VK_LEFT for example?).  It's not quite clear to me what all of the ways that we can transition out of the typing mode are.

That all being said, I still think that doing this adds unnecessary complexity to the code and is confusing to the user because these transitions may not be quite clear.  It will also mean more work on the Gecko side, which we need to do in another bug once the transitions are precisely defined.  I'm also worried about possible edge cases that we may not handle properly which would result in the caret to be stuck in typing mode.  But the UX side of things is ultimately your call.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #19)
> OK, I'm afraid if we want to implement what the current spec says, we need
> to extend the existing touch caret to have two modes, one the normal mode
> (what we have implemented so far) and one the "typing" mode where we don't
> draw the handles but we still draw the vertical caret line.  In order for us
> to be able to implement that, we need to know all of the ways that the
> "typing" caret and "normal" caret can transition into each other.  So far it
> seems that you want us to draw the "typing" caret as soon as a key press
> event has been sent (what if the key press event is a directional key such
> as VK_LEFT for example?).  It's not quite clear to me what all of the ways
> that we can transition out of the typing mode are.
If it's a directional key like VK_LEFT pressed, we also hide the caret.

> That all being said, I still think that doing this adds unnecessary
> complexity to the code and is confusing to the user because these
> transitions may not be quite clear.  It will also mean more work on the
> Gecko side, which we need to do in another bug once the transitions are
> precisely defined.  I'm also worried about possible edge cases that we may
> not handle properly which would result in the caret to be stuck in typing
> mode.  But the UX side of things is ultimately your call.
Great! Let's do it this way. Thanks for all your efforts on it!
(In reply to Omega Feng [:Omega] from comment #20)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #19)
> > OK, I'm afraid if we want to implement what the current spec says, we need
> > to extend the existing touch caret to have two modes, one the normal mode
> > (what we have implemented so far) and one the "typing" mode where we don't
> > draw the handles but we still draw the vertical caret line.  In order for us
> > to be able to implement that, we need to know all of the ways that the
> > "typing" caret and "normal" caret can transition into each other.  So far it
> > seems that you want us to draw the "typing" caret as soon as a key press
> > event has been sent (what if the key press event is a directional key such
> > as VK_LEFT for example?).  It's not quite clear to me what all of the ways
> > that we can transition out of the typing mode are.
> If it's a directional key like VK_LEFT pressed, we also hide the caret.
Hi Omega,
Borrow Eshan's definition here, we may define more clear rules of mode transition
1. typing mode to normal mode
   a. timeout(default value is 3 seconds) - switch from typing mode to normal mode automatically after 3 seconds without touch event.
   b. touch on any place out of touch caret area
   c. key event sent -this is the condition we discussed in this bug.
2. normal mode to typing mode
   a. click on an input/editable element again.
Basically, I can't find clear definition of mode transition in spec so far.
Please see the updated UX spec:
https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8439660
@CJ, it's a little bit different from your comments, so please see if you have any feedback on that, thanks!
Flags: needinfo?(cku)
In p.15
Select mode -(Dismiss keyboard)-> Unfocused
  I don't know why we do this transition.
  a. select on non-editable element - keyboard doesn't raise up in this condition. Skip
  b. select on editable element
     i. user do that because he want to delete a work or a string, or replace it by new input word.
     ii. user want to copy and paste.
     In condition ii, user does want to dismiss keyboard, so that he can see more content. In this case, 

Except this one, everything looks good to me, thank Omega!
Flags: needinfo?(cku) → needinfo?(ofeng)
> In p.15
> Select mode -(Dismiss keyboard)-> Unfocused
>   I don't know why we do this transition.
>   a. select on non-editable element - keyboard doesn't raise up in this
> condition. Skip
>   b. select on editable element
>      i. user do that because he want to delete a work or a string, or
> replace it by new input word.
>      ii. user want to copy and paste.
>      In condition ii, user does want to dismiss keyboard, so that he can see
> more content. IMO, In this case, stay in select mode is better.
> 
> Except this one, everything looks good to me, thank Omega!
Hi CJ,
As discussion, we think keeping selection without keyboard may cause some problem like unable to delete/type. So, we will keep the design in the UX spec. Thanks for your understanding.
Flags: needinfo?(ofeng)
Attached patch test.patchSplinter Review
Attachment #8445074 - Flags: feedback?(ehsan)
(In reply to StevenLee[:slee] from comment #26)
> Created attachment 8445074 [details] [diff] [review]
> test.patch
Hi ehsan,

There are some test cases(the expected behaviors are different from the UX spec on B2G), such as [1], we need to either disable them or disable touch caret when testing. This patch is try to fix the display problem that we disable touch caret on the fly.
 
1. Although we check the latest preference of "touchcaret.enabled", TouchCaret::NotifySelectionChanged([2]) is called directly from nsSelection without checking the preference. To solve this, we need to include "nsPresShell.h" in TouchCaret.cpp and check the preference in the callback function.
2. [3] will be called when the element is focused. I check whether preference is enabled or not.

[1] http://dxr.mozilla.org/mozilla-central/source/layout/base/tests/test_bug644768.html
[2] http://dxr.mozilla.org/mozilla-central/source/layout/base/TouchCaret.cpp#315
[3] http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#2215
Assignee: tlin → slee
Summary: [Touch Caret] Enable touch caret on B2G → [Touch Caret] Enable touch caret on B2G and fix the existing test case failures
Comment on attachment 8445074 [details] [diff] [review]
test.patch

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

::: layout/base/TouchCaret.cpp
@@ +90,4 @@
>  void
>  TouchCaret::SetVisibility(bool aVisible)
>  {
> +  printf_stderr("TouchCaret::SetVisibility to %d \n", aVisible);

Please remove this.

@@ +324,4 @@
>    }
>  
>    nsRefPtr<nsCaret> caret = presShell->GetCaret();
> +  if (!caret || !PresShell::TouchCaretPrefEnabled()) {

If you do what I suggest in my comment on nsPresShell.cpp, then NotifySelectionChanged will not even be called here, and you won't need to touch this file.

::: layout/base/nsPresShell.cpp
@@ +878,4 @@
>    // before creating any frames.
>    SetPreferenceStyleRules(false);
>  
> +  printf_stderr("%p try to new TouchCaret \n", this);

Please remove this.

@@ +2231,4 @@
>      if (mCaret) {
>        mCaret->SetCaretVisible(mCaretEnabled);
>      }
> +    if (mTouchCaret && TouchCaretPrefEnabled()) {

The right way to prevent NotifySelectionChanged from being called is to avoid calling AddSelectionListener on the touch caret near the hunk above in PresShell::Init.

Note that doing this will mean that the pref will just be checked when the presshell is being initialized so changing the pref at runtime will only affect newly created presshells.  This might mean that you need to restructre test_bug644768.html to make it set the pref first, and then either load the actual test code in an iframe in order to get the presshell for the iframe to be initialized after the pref is set.  I think that is a better solution than spreading these pref checks everywhere, as it will be much more maintainable.  :-)

::: layout/base/tests/test_bug644768.html
@@ +52,4 @@
>    ok(equal, "newline before bidi text shouldn't change direction: expected " +
>       str1 + " but got " + str2);
>  
> +  SpecialPowers.setBoolPref('touchcaret.enabled', true);

You shouldn't need to do this, pushPrefEnv should restore the pref when the test is done.

::: testing/profiles/prefs_b2g_unittest.js
@@ +8,4 @@
>  user_pref("dom.mozBrowserFramesWhitelist","app://test-container.gaiamobile.org,http://mochi.test:8888");
>  user_pref("marionette.force-local", true);
>  user_pref("dom.testing.datastore_enabled_for_hosted_apps", true);
> +user_pref("touchcaret.enabled", true);

This just enables the pref on tests, not on devices.  I thought the objective of this bug is to enable this on devices?  If you do that (in b2g.js for example) then you won't need this line here.
Attachment #8445074 - Flags: feedback?(ehsan) → feedback-
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #28)
> > +  printf_stderr("TouchCaret::SetVisibility to %d \n", aVisible);
> Please remove this.
Sure, this is my debug code. XD

> @@ +2231,4 @@
> >      if (mCaret) {
> >        mCaret->SetCaretVisible(mCaretEnabled);
> >      }
> > +    if (mTouchCaret && TouchCaretPrefEnabled()) {
> 
> The right way to prevent NotifySelectionChanged from being called is to
> avoid calling AddSelectionListener on the touch caret near the hunk above in
> PresShell::Init.
> 
> Note that doing this will mean that the pref will just be checked when the
> presshell is being initialized so changing the pref at runtime will only
> affect newly created presshells.  This might mean that you need to
> restructre test_bug644768.html to make it set the pref first, and then
> either load the actual test code in an iframe in order to get the presshell
> for the iframe to be initialized after the pref is set.  I think that is a
> better solution than spreading these pref checks everywhere, as it will be
> much more maintainable.  :-)
O
> 
> > +  SpecialPowers.setBoolPref('touchcaret.enabled', true);
> You shouldn't need to do this, pushPrefEnv should restore the pref when the
> test is done.
Got it, thanks.

> ::: testing/profiles/prefs_b2g_unittest.js
> @@ +8,4 @@
> >  user_pref("dom.mozBrowserFramesWhitelist","app://test-container.gaiamobile.org,http://mochi.test:8888");
> >  user_pref("marionette.force-local", true);
> >  user_pref("dom.testing.datastore_enabled_for_hosted_apps", true);
> > +user_pref("touchcaret.enabled", true);
> 
> This just enables the pref on tests, not on devices.  I thought the
> objective of this bug is to enable this on devices?  If you do that (in
> b2g.js for example) then you won't need this line here.
Sorry, I should update my new plan. copy & paste is decided to be preference-off in FFOS 2.0. But we want to make sure the incoming code does not break it. So we are going to enabled touch caret in the test cases but not on B2G. When we are on 2.1, we can turn the preference on with less effort. I will file another bug that enable touch caret on b2g and remove the preference on prefs_b2g_unittest.js.
Summary: [Touch Caret] Enable touch caret on B2G and fix the existing test case failures → [Touch Caret] Enable touch caret on B2G test cases and fix the existing test case failures
(In reply to StevenLee[:slee] from comment #29)
> > @@ +2231,4 @@
> > >      if (mCaret) {
> > >        mCaret->SetCaretVisible(mCaretEnabled);
> > >      }
> > > +    if (mTouchCaret && TouchCaretPrefEnabled()) {
> > 
> > The right way to prevent NotifySelectionChanged from being called is to
> > avoid calling AddSelectionListener on the touch caret near the hunk above in
> > PresShell::Init.
> > 
> > Note that doing this will mean that the pref will just be checked when the
> > presshell is being initialized so changing the pref at runtime will only
> > affect newly created presshells.  This might mean that you need to
> > restructre test_bug644768.html to make it set the pref first, and then
> > either load the actual test code in an iframe in order to get the presshell
> > for the iframe to be initialized after the pref is set.  I think that is a
> > better solution than spreading these pref checks everywhere, as it will be
> > much more maintainable.  :-)
Sorry, I cut out my comments accidentally.
I think the original code is doing what you told. I can try to restructure the test case and make it can get the latest pref value. Thanks for your suggestion. :)
Depends on: 1029943
Blocks: 1029943
No longer depends on: 1029943
Hi Steven, anytime we can have the patch soon?
Flags: needinfo?(slee)
I am stuck on the timeout issue on try server. When all test cases are done, the testing script fails to get the output data and time out, [1]. I can reproduce it on my PC and still trying to figure why it happens when we turn on touch caret. Here is the results on try server, [2]. You can check test suite, 14.

[1] http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#761
[2] https://tbpl.mozilla.org/?tree=Try&rev=b8799a9df67b
Flags: needinfo?(slee)
See the assertions toward the end of that log.  It seems that we're shutting down some stuff too late.  It would be good to start investigating the shutdown sequence here.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #33)
> See the assertions toward the end of that log.  It seems that we're shutting
> down some stuff too late.  It would be good to start investigating the
> shutdown sequence here.
I found the problem. There are 2 problems.
1. someone tries to send IPC message after we call MessageLoop::Quit. It causes the assertion log.
2. When #1 happens, the crash reporter of b2g process is trying to suspend the process and get the backtrace. But it's blocked in [1]. After 450 seconds, testing framework timeout, b2g process is killed. 

The problem can be reproduced easily on emulator debug version. You can just run "./mach mochitest-remote layout/forms/test". I will file other bugs to trace these 2 issues. 

[1] http://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_ptrace_dumper.cc#58
Depends on: 1034523
Depends on: 1034527
(In reply to StevenLee[:slee] from comment #34)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #33)
> > See the assertions toward the end of that log.  It seems that we're shutting
> > down some stuff too late.  It would be good to start investigating the
> > shutdown sequence here.
> I found the problem. There are 2 problems.
> 1. someone tries to send IPC message after we call MessageLoop::Quit. It
> causes the assertion log.
> 2. When #1 happens, the crash reporter of b2g process is trying to suspend
> the process and get the backtrace. But it's blocked in [1]. After 450
> seconds, testing framework timeout, b2g process is killed. 

Can you clarify #1 here? What exactly is the b2g process blocked on?
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #35)
> Can you clarify #1 here? What exactly is the b2g process blocked on?
You can check bug 1034523 and bug bug 1034527. 
Bug 1034523 is fixed by bug 774388 and I marked bug 1034523 as duplicate. It's because the layer module tries to send IPC message after message loop quits. 

When someone hits assertion and throw signaling, parent process tries to wake up minidump to get the call stacks of the content process. b2g process will be blocked in SuspendThread function. I have no idea about what happens. I check the state of content process from ps and it shows 'T'(stopped). I also filed bug 1034527.
Reftest failures when touch caret is enabled.

https://tbpl.mozilla.org/?tree=Try&rev=5810c0995f91
Attached file Test focus on input
Attachment #8455117 - Flags: review?(ehsan)
This change fixed all the existing test failures. The idea is not to
sync the touch caret enabled status with the caret in PresShell.
Otherwise touch caret is either shown at the wrong position when
javascript moves a dom element around (e.g. unneeded_scroll.html), or
being flashy when javascript changes foucs or attributes (e.g.
568441.html).

Let touch caret show only when selection changed externally by the user,
rather than caret being enabled internally.
Attachment #8455119 - Flags: review?(ehsan)
Attachment #8455120 - Flags: review?(ehsan)
More on patch set part 3: I've created attachment 8455106 [details] (Test focus on input) to test if we should show touch caret when focusing on a dom element by javascript. Neither Fennec nor Chrome on Android show touch caret in this test case. I feel it's reasonable not to show touch caret as well.
Uh, I got mochitest-14 failure because of the assertion in message_loop.cc.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=8bb4d7b1cca9
Comment on attachment 8455117 [details] [diff] [review]
part 1 - Correct documents in nsCaret.h

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

Nit: on the commit message: "documentation"
Attachment #8455117 - Flags: review?(ehsan) → review+
Attachment #8455118 - Flags: review?(ehsan) → review+
Comment on attachment 8455119 [details] [diff] [review]
part 3 - Revise touch caret visibility logic

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

This patch is not really fixing anything, it's just disabling the code that tracks the location of the touch caret to wallpaper over the test failures.  This is wrong, and it will break real pages when they change the structure of the DOM while a touch caret is being displayed.  We really need to track down the root causes of test failures and fix them instead of wallpapering over them this way.
Attachment #8455119 - Flags: review?(ehsan) → review-
Comment on attachment 8455120 [details] [diff] [review]
part 4 - Enable touch caret by default on b2g

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

r- until the tests are fixed.
Attachment #8455120 - Flags: review?(ehsan) → review-
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #43)
> More on patch set part 3: I've created attachment 8455106 [details] (Test
> focus on input) to test if we should show touch caret when focusing on a dom
> element by javascript. Neither Fennec nor Chrome on Android show touch caret
> in this test case. I feel it's reasonable not to show touch caret as well.

I think part 3 is way more extensive than what this test case is testing.
Assignee: slee → tlin
Correct commit message: s/documents/documentation
Attachment #8455117 - Attachment is obsolete: true
Attachment #8455939 - Flags: review+
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #48)
> (In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #43)
> > More on patch set part 3: I've created attachment 8455106 [details] (Test
> > focus on input) to test if we should show touch caret when focusing on a dom
> > element by javascript. Neither Fennec nor Chrome on Android show touch caret
> > in this test case. I feel it's reasonable not to show touch caret as well.
> 
> I think part 3 is way more extensive than what this test case is testing.

I guess I have to keep the status of caret and touch caret in sync in PresShell::SetCaretEnabled(), and dig deeper into the display issues in the test cases.
I would like to check in part 1 and part 2 to ease the debug process. Thanks.
Status: NEW → ASSIGNED
This patch just improve the code of touch caret, and it does not modify
touch caret's behavior.

* Use nsIPresShell* instead of nsWeakPtr to hold the pointer to
  PresShell. Since touch caret is an member of PresShell, PresShell is
  guaranteed to live longer than touch caret. Selection carets did this,
  too.
* Group the private section together, and remove the protect section
  since touch caret is a MOZ_FINAL class.
* Remove GetCanvasFrame().
* Remove unneeded #includes, and sort them.
* Fix typos at various locations.
Attachment #8459567 - Flags: review?(ehsan)
* Refactor touch caret's update visibility and position functions.
* Update touch caret in PresShell::DidDoReflow() and
  PresShell::UnsuppressAndInvalidate().
Attachment #8455119 - Attachment is obsolete: true
Attachment #8459568 - Flags: review?(ehsan)
Move the witness down so that touch caret won't cover other elements
when witness is focused().
Attachment #8459569 - Flags: review?(ehsan)
Rename test_bug644768.html to bug644768.html, and change the original
test_bug644768.html to an iframe container.

Thank Steven Lee <slee@mozilla.com> for the original WIP patch.
Attachment #8459570 - Flags: review?(ehsan)
Turn off the touch caret preference to fix test cases that ought to be
failed by definition of the touch caret behavior.

Thank Steven Lee <slee@mozilla.com> for the original WIP patch.
Attachment #8459571 - Flags: review?(ehsan)
To avoid the appearance of the touch caret interfering with the
synthesized mouse event position, add spaces between <input> fields, and
make the event at the center of the pasteArea.
Attachment #8459572 - Flags: review?(ehsan)
Rebase
Attachment #8455120 - Attachment is obsolete: true
Comment on attachment 8459574 [details] [diff] [review]
part 4 - Enable touch caret by default on b2g

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

I hope they are all green this time.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=e0580cfe06f0
Attachment #8459574 - Flags: review?(ehsan)
Comment on attachment 8459567 [details] [diff] [review]
part 3a - Use raw pointer to PresShell in touch caret

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

::: layout/base/TouchCaret.h
@@ +65,5 @@
>      return mVisible;
>    }
>  
> +  // The auto scroll timer's interval in milliseconds.
> +  static const int32_t sAutoScrollTimerDelay = 30;

Why did you make this public?

@@ +222,5 @@
>      return sTouchCaretExpirationTime;
>    }
>  
> +  // PresShell that owns us
> +  nsIPresShell* mPresShell;

TouchCaret is a refcounted object, so its lifetime is not bound to the owning presshell object since that object just owns one reference to this.  So this patch will introduce cases where we can access mPresShell after it has been freed.
Attachment #8459567 - Flags: review?(ehsan) → review-
Comment on attachment 8459570 [details] [diff] [review]
part 3d - Fix test_bug644768.html

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

Why is it OK to turn the touch caret off for this test?
Comment on attachment 8459573 [details] [diff] [review]
part 3g - Enlarge expiration time in mochitest and reftest

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

::: layout/tools/reftest/reftest-preferences.js
@@ +47,5 @@
>      // failures.
>      branch.setBoolPref("layout.interruptible-reflow.enabled", false);
> +    // Set a large expiration time (in milliseconds) for touch caret to prevent
> +    // it disappear too quickly to avoid potential intermittent issues.
> +    branch.setIntPref("touchcaret.expiration.time", 60000);

Why not set it to 0 to disable the caret hiding completely?
Attachment #8459573 - Flags: review?(ehsan) → review-
Ting-Yu, can you please describe the rest of the patches in this series with more details?  I took a brief look at all of them and in most cases I'm not sure what problems each patch is trying to solve, which makes reviewing the patches difficult.

Thanks!
Flags: needinfo?(tlin)
Comment on attachment 8459567 [details] [diff] [review]
part 3a - Use raw pointer to PresShell in touch caret

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

::: layout/base/TouchCaret.h
@@ +65,5 @@
>      return mVisible;
>    }
>  
> +  // The auto scroll timer's interval in milliseconds.
> +  static const int32_t sAutoScrollTimerDelay = 30;

SelectionCarets is accessing sAutoScrollTimerDelay and I removed the "friend class SelectionCarets", so I made it public. Do you suggest we keep the friend class?

@@ +222,5 @@
>      return sTouchCaretExpirationTime;
>    }
>  
> +  // PresShell that owns us
> +  nsIPresShell* mPresShell;

Touch caret might be a refcounted object in syntax. However, touch caret works like it is owned by mPresShell to me. It creates touch caret in PresShell::Init(), and PresShell::Destory() calls     mTouchCaret->Terminate() to make our mPresShell be a nullptr [1].

In fact, touch caret cannot work without PresShell. Before part 3a, touch caret used to check mPresShell's validity and return early in various public functions. After refactoring, I added MOZ_ASSERT(mPresShell) in part 3b. 

And I saw roc's comment [2] in selection carets to make mPresShell a raw pointer. That's why I did this . Do you suggest we keep mPresShell as a weak pointer?

[1] http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?offset=57#1149
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=987718#c35
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #64)
> Comment on attachment 8459570 [details] [diff] [review]
> part 3d - Fix test_bug644768.html
> 
> Review of attachment 8459570 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why is it OK to turn the touch caret off for this test?

Please see the explanation in comments in the following test code.

  textInput.focus();
  s1 = snapshotWindow(window); // focus() makes touch caret show. We have touch caret in this snapshot.

  synthesizeKey("VK_UP", { }); // key events make touch caret hide.
  synthesizeKey("VK_UP", { });
  synthesizeKey("VK_UP", { });
  synthesizeKey("VK_DELETE", { });
  synthesizeKey("VK_RETURN", { });
  // Bug 1016184: Touch caret will hide due to key event.
  s2 = snapshotWindow(window); // We do not have touch caret in this snapshot
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #65)
> Comment on attachment 8459573 [details] [diff] [review]
> part 3g - Enlarge expiration time in mochitest and reftest
> 
> Review of attachment 8459573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/tools/reftest/reftest-preferences.js
> @@ +47,5 @@
> >      // failures.
> >      branch.setBoolPref("layout.interruptible-reflow.enabled", false);
> > +    // Set a large expiration time (in milliseconds) for touch caret to prevent
> > +    // it disappear too quickly to avoid potential intermittent issues.
> > +    branch.setIntPref("touchcaret.expiration.time", 60000);
> 
> Why not set it to 0 to disable the caret hiding completely?

You're right. It's better to set it to 0. Will do in next patch.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #66)
> Ting-Yu, can you please describe the rest of the patches in this series with
> more details?  I took a brief look at all of them and in most cases I'm not
> sure what problems each patch is trying to solve, which makes reviewing the
> patches difficult.
> 
> Thanks!

Part 3a: 
I would like to simplify the use of mPresShell, and make the touch caret code look better.

Part 3b:
This change revised the condition to show touch caret, and fix the reftest failed R6, R7, R20 in [1]. Basically, all the test failed after turn on the pref on try server are reftest failures. Those mochitests failed because they have difference snapshots.

* R6 failed in tests/layout/reftests/bugs/542317-1.html: Touch caret show at the wrong place in REFERENCE. It should show in the correct place under the caret.
* R7 failed in layout/reftests/bugs/568441.html: Touch caret should be shown.
* R20 failed in reftests/unneeded_scroll.html: notice that touch caret did show in TEST, but the caret is *not* show. Touch caret should not be show in this test case because caret is hidden.

To fix above issue, I first refactored the UpdateTouchCaret() into void UpdateVisibilityAndPosition(bool aSyncCaretVisibility). I notice caret updates its status in PresShell::UnsuppressAndInvalidate() and PresShell::DidDoReflow(). So I did the same for touch caret to to fix R6, R7, and R20. 

Why the aSyncCaretVisibility argument?  We call UpdateVisibilityAndPosition() in two circumstances.
1) We would like to sync the visibility with caret, i.e. show touch caret when the caret is shown, and hide touch caret when the caret is hidden. For example, in PresShell::SetCaretEnabled() or in TouchCaret::NotifySelectionChanged(). We set aSyncCaretVisibility to true.
2) We would like to update our position and visibility only. When caret is hidden, touch caret should be hide for sure. However when caret is shown, touch caret might not be shown because it received some key event before. In this case, we set aSyncCaretVisibility to false when calling in PresShell::UnsuppressAndInvalidate() and PresShell::DidDoReflow(). The two functions are just post-update functions, not actually turn on/off the visibility of caret and touch caret.

[1] https://tbpl.mozilla.org/?tree=Try&rev=645d5e5b56c6
Part 3c: In these test cases, they synthesize and fire mouse events at some element. When touch caret shows, it covers the element where the mouse event should go, so the mouse events did not go to the desired place. I move those element around in these patches so that touch caret won't cover them.

Part 3d: Explained in comment 68.

Part 3e: Most of the test cases are all involving with one sending synthesizeKey() (which hide the touch caret), the other without. They ought to be failed when touch caret preference on. We could turn the touch preference off to solve them. 

Part 3f: Same reason as 3c.
Flags: needinfo?(tlin)
Attachment #8459573 - Attachment is obsolete: true
Attachment #8460040 - Flags: review?(ehsan)
Comment on attachment 8459567 [details] [diff] [review]
part 3a - Use raw pointer to PresShell in touch caret

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

::: layout/base/TouchCaret.h
@@ +65,5 @@
>      return mVisible;
>    }
>  
> +  // The auto scroll timer's interval in milliseconds.
> +  static const int32_t sAutoScrollTimerDelay = 30;

I would prefer not to public sAutoScrollTimerDelay since only SelectionCaret uses it.
Comment on attachment 8459568 [details] [diff] [review]
part 3b - Revise touch caret visibility logic

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

::: layout/base/TouchCaret.cpp
@@ +335,1 @@
>      SetVisibility(false);

I think we don't need to call SetVisibility here because SetVisibility() always returns when touchCaretElement is null.

@@ +358,5 @@
> +    return;
> +  }
> +
> +  // When caret is hidden, we should always be hid. When caret is shown, we look
> +  // for aSyncCaretVisibility to decide if we should to be shown.

Here caret is always visible, therefore, above comment should be "Set the visibility of touch caret as true when aSyncCaretVisible is true, otherwise keep original visibility state"

@@ +368,5 @@
> +void
> +TouchCaret::UpdatePosition()
> +{
> +  if (!mVisible) {
> +    return;

Move the mVisible checking to UpdateVisibilityAndPosition()

@@ +412,5 @@
> +
> +void
> +TouchCaret::UpdateVisibilityAndPosition(bool aSyncCaretVisibility)
> +{
> +  UpdateVisibility(aSyncCaretVisibility);

Only mVisiblity is true, then we need to update position.

::: layout/base/nsPresShell.cpp
@@ +3920,5 @@
>      }
> +
> +    if (mTouchCaret) {
> +      mTouchCaret->UpdateVisibilityAndPosition(false);
> +    }

Please add comment why no need to sync CaretVisibility here

@@ +8649,5 @@
>    }
>  
> +  if (mTouchCaret) {
> +    mTouchCaret->UpdateVisibilityAndPosition(false);
> +  }

Please add comment why no need to sync CaretVisibility here
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #67)
> Comment on attachment 8459567 [details] [diff] [review]
> part 3a - Use raw pointer to PresShell in touch caret
> 
> Review of attachment 8459567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/TouchCaret.h
> @@ +65,5 @@
> >      return mVisible;
> >    }
> >  
> > +  // The auto scroll timer's interval in milliseconds.
> > +  static const int32_t sAutoScrollTimerDelay = 30;
> 
> SelectionCarets is accessing sAutoScrollTimerDelay and I removed the "friend
> class SelectionCarets", so I made it public. Do you suggest we keep the
> friend class?

I'm suggesting to not modify anything unless there is a good reason to.  I don't think making this member public is a useful change.

> @@ +222,5 @@
> >      return sTouchCaretExpirationTime;
> >    }
> >  
> > +  // PresShell that owns us
> > +  nsIPresShell* mPresShell;
> 
> Touch caret might be a refcounted object in syntax. However, touch caret
> works like it is owned by mPresShell to me. It creates touch caret in
> PresShell::Init(), and PresShell::Destory() calls    
> mTouchCaret->Terminate() to make our mPresShell be a nullptr [1].

But the point is that there is *nothing* that enforces this, so your assumption is not correct.  Refcounted objects are deleted when the last outstanding reference to them gets released, and in this case this may well be after the presshell is dead.  The presshell doesn't own these objects in any way, it just has a strong reference to them.

> In fact, touch caret cannot work without PresShell. Before part 3a, touch
> caret used to check mPresShell's validity and return early in various public
> functions. After refactoring, I added MOZ_ASSERT(mPresShell) in part 3b. 

That change is also incorrect.  Since you cannot guarantee that this object always has a presshell available, you need to check for its existence at runtime as opposed to just assert it.

> And I saw roc's comment [2] in selection carets to make mPresShell a raw
> pointer. That's why I did this . Do you suggest we keep mPresShell as a weak
> pointer?

I don't know why roc said that but that change will only be correct if _something_ enforces that this object will be destroyed before the presshell dies, and I can't see what enforces that.

Also, please let's try to keep stuff that is not relevant to enabling touch carets on b2g out of this bug, as it is getting long and confusing.  If you can construct a proof why TouchCaret cannot outlive the presshell, please file another bug and ask me for review on the patch there with the proof in the commit message.  Thanks!
Comment on attachment 8459570 [details] [diff] [review]
part 3d - Fix test_bug644768.html

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

OK, thanks for the explanation!
Attachment #8459570 - Flags: review?(ehsan) → review+
Attachment #8460040 - Flags: review?(ehsan) → review+
Comment on attachment 8459568 [details] [diff] [review]
part 3b - Revise touch caret visibility logic

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

::: layout/base/TouchCaret.cpp
@@ +287,5 @@
>  TouchCaret::NotifySelectionChanged(nsIDOMDocument* aDoc, nsISelection* aSel,
>                                     int16_t aReason)
>  {
> +  TOUCHCARET_LOG("Reason=%d", aReason);
> +  MOZ_ASSERT(mPresShell, "mPresShell is nullptr!");

As I noted previously, you need to do a null check here.

@@ +368,5 @@
> +void
> +TouchCaret::UpdatePosition()
> +{
> +  if (!mVisible) {
> +    return;

As Peter suggested, I think it's better to do this in the caller.

That being said, it's a good idea to MOZ_ASSERT(mVisible) here to make sure that nobody will forget to do this check in the caller in the future.

::: layout/base/TouchCaret.h
@@ +79,5 @@
> +
> +
> +  /**
> +   * Update touch caret's position.
> +   */

Not a super useful comment.  ;-)  Please remove it.
Attachment #8459568 - Flags: review?(ehsan) → review+
Attachment #8459569 - Flags: review?(ehsan) → review+
Attachment #8459571 - Flags: review?(ehsan) → review+
Comment on attachment 8459572 [details] [diff] [review]
part 3f - Fix test_paste_selection.html

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

::: dom/tests/mochitest/general/test_paste_selection.html
@@ +16,4 @@
>  <input id="paste-selection-area" value="" onpaste="pastedSelection(event)">
> +<br>
> +<br>
> +<br>

This is *so* hacky...  Why is the touch caret in the first input not hidden when we .focus() the second one?  Or am I missing something?
Attachment #8459572 - Flags: review?(ehsan) → review-
Comment on attachment 8459574 [details] [diff] [review]
part 4 - Enable touch caret by default on b2g

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

Yay!
Attachment #8459574 - Flags: review?(ehsan) → review+
Peter, Ehsan, thank you for your prompted review! I'll address your comments, and upload new patches as soon as possible.
Comparing with "part 3a - Use raw pointer to PresShell in touch caret", this change only fixes typos and style. No change is made to mPresShell.
Attachment #8459567 - Attachment is obsolete: true
Attachment #8461543 - Flags: review?(ehsan)
After reading Comment 74 by Peter, I realized that UpdateVisibilityAndPosition(bool aSyncCaretVisibility); might not be clear. I refactored a bit. Hoping that reads better.
Attachment #8459568 - Attachment is obsolete: true
Attachment #8461557 - Flags: review?(ehsan)
Attachment #8461557 - Flags: feedback?(pchang)
Comment on attachment 8459572 [details] [diff] [review]
part 3f - Fix test_paste_selection.html

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

::: dom/tests/mochitest/general/test_paste_selection.html
@@ +16,4 @@
>  <input id="paste-selection-area" value="" onpaste="pastedSelection(event)">
> +<br>
> +<br>
> +<br>

I am not sure why... It is passed on my local machine only with the change to synthesizeMouseAtCenter() below. However, it's orange on B2G Desktop Linux on try server. I add those <br>s by gut feeling, and it's green. I need to add more log to debug.
Attachment #8461543 - Flags: review?(ehsan) → review+
Comment on attachment 8461557 [details] [diff] [review]
part 3b - Revise touch caret visibility logic (v2)

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

Looks *much* better now!

::: layout/base/TouchCaret.cpp
@@ +391,5 @@
> +  }
> +}
> +
> +bool
> +TouchCaret::isDisplayable()

Nit: IsDisplayable()
Attachment #8461557 - Flags: review?(ehsan) → review+
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #83)
> Comment on attachment 8459572 [details] [diff] [review]
> part 3f - Fix test_paste_selection.html
> 
> Review of attachment 8459572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/tests/mochitest/general/test_paste_selection.html
> @@ +16,4 @@
> >  <input id="paste-selection-area" value="" onpaste="pastedSelection(event)">
> > +<br>
> > +<br>
> > +<br>
> 
> I am not sure why... It is passed on my local machine only with the change
> to synthesizeMouseAtCenter() below. However, it's orange on B2G Desktop
> Linux on try server. I add those <br>s by gut feeling, and it's green. I
> need to add more log to debug.

The underlying issue is that synthesizeMouseAtCenter (and friends) only work if the element is on the screen, and with the test runner iframe being a different size on different platforms, it sometimes means that a test that passes in one platform fails on another one.

Can you try using .scrollIntoView() on these elements before calling synthesizeMouseAtCenter on them to make sure that they are always in the viewport?
Or use the variant which dispatches events to window (whatever ends up using sendMouseEventToWindow, not sendMouseEvent).
Comment on attachment 8461557 [details] [diff] [review]
part 3b - Revise touch caret visibility logic (v2)

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

LGTM
Attachment #8461557 - Flags: feedback?(pchang) → feedback+
(In reply to comment #86)
> Or use the variant which dispatches events to window (whatever ends up using
> sendMouseEventToWindow, not sendMouseEvent).

Yes, this is actually a better idea.  Thanks, Olli!
While debugging test_paste_selection.html, more reftest test failures happen after I rebased patches.
The cause is the horizontal scroll bar shows. Not sure why for now :(

https://tbpl.mozilla.org/?tree=Try&rev=208388bff0f9
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #89)
> While debugging test_paste_selection.html, more reftest test failures happen
> after I rebased patches.
> The cause is the horizontal scroll bar shows. Not sure why for now :(

Hmm, that is really weird...  There is for example this test failure <https://tbpl.mozilla.org/php/getParsedLog.php?id=44596973&tree=Try&full=1> but the page doesn't have anything on it which would trigger a touch caret.  It would be helpful to pinpoint the test failure to one of the patches in your queue to see what's going on here.
After removing "visibility: hidden;" in ua.css, I saw the touch caret is at position (0, 0). That's why the horizontal scroll bar show. I'm wondering why this didn't catch by try server before.

I try to using "display: none" to hide the touch caret, but I got a lot of warning as [1]. I think I will just add "left: 15px" to compensate "margin-left: -15px". The "left" property will be updated when touch caret shows by TouchCaret::SetTouchFramePos() anyway.

[1] WARNING: Someone passed native anonymous content directly into frame construction.  Stop doing that!: file /Users/aethanyc/Projects/gecko-dev/layout/base/nsCSSFrameConstructor.cpp, line 6282
Rebase and add r=ehsan.
Attachment #8461543 - Attachment is obsolete: true
Attachment #8463069 - Flags: review+
Rebase, and add f=pchang, r=ehsan. Fix nit: s/isDisplayable/IsDisplayable
Attachment #8461557 - Attachment is obsolete: true
Attachment #8463070 - Flags: review+
Rebase and add r=ehsan.
Attachment #8459569 - Attachment is obsolete: true
Attachment #8463071 - Flags: review+
Rebase and add r=ehsan.
Attachment #8459570 - Attachment is obsolete: true
Attachment #8463072 - Flags: review+
Rebase and add r=ehsan.
Attachment #8459571 - Attachment is obsolete: true
Attachment #8463073 - Flags: review+
Thank Olli Pettay <bugs@pettay.fi> for suggesting
sendMouseEventToWindow(). This is a tremendous help!
Attachment #8459572 - Attachment is obsolete: true
Attachment #8463074 - Flags: review?(ehsan)
Rebase and add r=ehsan.
Attachment #8460040 - Attachment is obsolete: true
Attachment #8463075 - Flags: review+
The touch caret frame is initialized at (0, 0). When <html> or <body>
has dir="rtl", "margin-left: -15px" (in ua.css) makes the horizontal
scroll bar appear even if the touch caret is invisible. To compensate
this, add "left: 15px" to make the touch caret frame initialized within
the viewport so that horizontal scroll bar won't show.
Attachment #8463076 - Flags: review?(ehsan)
Rebase and add r=ehsan.
Attachment #8459574 - Attachment is obsolete: true
Attachment #8463077 - Flags: review+
Attachment #8463074 - Flags: review?(ehsan) → review+
Comment on attachment 8463076 [details] [diff] [review]
part 3h - Fix horizontal scroll bar appears. r=ehsan

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

I'm going to forward this one to roc, I'm not 100% sure if this is the right thing to do.
Attachment #8463076 - Flags: review?(ehsan) → review?(roc)
Comment on attachment 8463076 [details] [diff] [review]
part 3h - Fix horizontal scroll bar appears. r=ehsan

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

I think it would be better to set the margin to zero if the caret is invisible. When it's invisible I suggest we also set its width and height to zero.
Attachment #8463076 - Flags: review?(roc) → review-
The touch caret frame is initialized at (0, 0). When <html> or <body>
has dir="rtl", "margin-left: -15px" (in ua.css) makes the horizontal
scroll bar appear even if the touch caret is invisible.

To compensate this, make width, height, and margin be 0px when the touch
caret is invisible.
Attachment #8463076 - Attachment is obsolete: true
Attachment #8463801 - Flags: review?(roc)
Please help check in part 3a ~ 3h, and part 4. Thanks.
Summary: [Touch Caret] Enable touch caret on B2G test cases and fix the existing test case failures → [Touch Caret] Enable touch caret on B2G and fix the existing test case failures
Great job Ting-Yu! I suppose we can close this bug and moving on to text selection cases :)
Whiteboard: [ft:system-platform]
Target Milestone: --- → 2.1 S1 (1aug)
Ryan, sorry for causing the trouble. I'm investigating the root cause.
Hi Sheriff, 

I would like to checkin part 3a ~ 3h to avoid these patches bitrotton. Please do NOT checkin part 4, which causes Gip fail on B2G Desktop OS X Opt for now. Thank you.

Try result (part 3a ~ 3h):
https://tbpl.mozilla.org/?tree=Try&rev=ecd834898560

Try result for other B2G tests (Please ignore Gip fail on B2G Desktop OS X Opt):
https://tbpl.mozilla.org/?tree=Try&rev=fbc6a3ec7809
Depends on: 1049104
Can this be closed now?
Flags: needinfo?(tlin)
Ehsan, I hope so, but part 4 does not landed yet. It's causes bug 1049104. I've requested a slave to reproduce.
Flags: needinfo?(tlin)
Oh, OK.  It will be hard later on to figure out which parts landed when, but I guess it's too late now.  Please try to split these types of large bugs into multiple smaller ones in the future.  Thanks!
After struggling for a week, I finally found that commit 194155:f4b101378a07 which change touch caret to use svg image should be the cause that makes Gip timeout-prone. The root cause is unknown to me. Since that change is a temporary solution to fix caret being blurry when zooming in, I think it makes no harm to backout the change.

The test fail in mulet-5 (layout/base/tests/test_reftests_with_caret.html) will be investigated later.
https://tbpl.mozilla.org/?tree=Try&rev=a65a354724c2
bug512295-1.html and bug512295-2.html are sending key events which hides
touch caret. Turn off the touch caret preference to fix test cases that
ought to be failed by definition of the touch caret behavior.

Ehsan, excuse me for putting another patch into this bug. I think we can almost
see the finish line.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=f1fb8db4fd74
Attachment #8474380 - Flags: review?(ehsan)
Attachment #8474380 - Flags: review?(ehsan) → review+
Please help checkin part3i and part 4. Thanks.
https://hg.mozilla.org/mozilla-central/rev/c0d776bda527
https://hg.mozilla.org/mozilla-central/rev/a08d841635ca
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
\o/
Set "disabled" for B2G v2.1 as suggested in comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=1059061#c8
You need to log in before you can comment on or make changes to this bug.