This bug is similar to bug 1120358, but it happens on AccssibleCaret when focusing on an empty content. Steps to reproduce: 1. Open Messages app. 2. Click the pencil icon on the upper-right corner to create a new message. 3. Type some text into the "To:" field, and copy them. The focus should remain on "To:" field. 4. Long tap onto the "empty" message body to show the paste shortcut. Actual results: The paste shortcut overlaps the message body. Expected results: The paste shortcut updates its position, and it does not overlap the message body.

After bug 571294, a modification to the range associated with current selection will fire a selectionchange event with reason = NO_REASON. In scrollMessageContent(), it modifies the range associated with the current selection in [1]. This causes AccessibleCaretManager hides the caret by [2] in the STR step 4 in comment #0. Therefore, the text selection dialog will disappear completely. [1] https://github.com/mozilla-b2g/gaia/blob/899860aa2f58cb2597405cd3f8572a938e7ca384/apps/sms/views/conversation/js/compose.js#L473-L479 [2] https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/layout/base/AccessibleCaretManager.cpp?offset=1700#65-68

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #1) > After bug 571294, a modification to the range associated with current > selection will fire a selectionchange event with reason = NO_REASON. > > In scrollMessageContent(), it modifies the range associated with the current > selection in [1]. This causes AccessibleCaretManager hides the caret by [2] > in the STR step 4 in comment #0. Therefore, the text selection dialog will > disappear completely. Do you think it's an expected behavior though ?

Comment on attachment 8663568 [details] [gaia] aethanyc:bug1196176 > mozilla-b2g:master r=me But I have 2 questions: * is it possible to do an integration test to make sure this doesn't regress ? Please land if it's too complicated though, but file a separate bug anyway. * I feel this is a workaround; is there a possibility to fix it from the AccessibleCaret implementation ?

Re comment #3 and comment #4: Thank for the review. Answers below. > Do you think it's an expected behavior though ? Yes. I think it's reasonable to have selectionchange if the range associated with current selection object is modified by script. > * is it possible to do an integration test to make sure this doesn't regress > ? Please land if it's too complicated though, but file a separate bug anyway. It's nearly impossible to test this since involves the interaction between AccessibleCaret, text selection dialog, and message app. However, I can write a gtest to ensure the CaretStateChanged events is fired correctly by AccessibleCaret for the STR in comment #0. > * I feel this is a workaround; is there a possibility to fix it from the > AccessibleCaret implementation ? I'm afraid it's not possible. In current design, AccessibleCaret will hide whenever the selectionchange reason is NO_REASON, which is the case when range or selection is modified by script. If we update caret for this reason, carets might show when script runs or too manay CaretStateChanged events will be fired which might have performance impact as in bug 1066515. From comment in [1], it's a workaround on top of an workaround for getting the bounding client rect on collapsed range though... [1] https://github.com/mozilla-b2g/gaia/blob/899860aa2f58cb2597405cd3f8572a938e7ca384/apps/sms/views/conversation/js/compose.js#L473-L479

> It's nearly impossible to test this since involves the interaction between > AccessibleCaret, text selection dialog, and message app. If it's possible to access the text selection dialog from Marionette, then I feel it should be possible. But I understand this would need to learn a lot of new things with Marionette and that it could be too long for now. Can you file a separate bug for this though ?

Bug 1196176 - No need to test mLastUpdateCaretMode. r=mtseng I now think that mLastUpdateCaretMode is an implementation detail. Test it will make test and implementation coupled. It's better to call EXPECT_EQ directly in test functions so that the line number in test result will be accurate when things go wrong.

Bug 1196176 - Hide carets for mouse down reason. r=mtseng SelectionCaret had been hiding carets upon receiving mouse down reason. In this way, text selection dialog won't show, hide, and show again.

Bug 1196176 - Fix CaretStateChanged not dispatch on empty content. r=mtseng We should dispatch CaretStateChanged event in OnReflow() in cursor mode when the first caret's appearance is NormalNotShown. Otherwise the text selection dialog won't update its position.

Bug 1196176 - Do not fire extra CaretStateChanged event when typing. r=mtseng After HideCaret() is called via keyboard event, OnScrollPositionChanged() still fire another CaretStateChanged event even if the caret is hidden. We follow OnReflow() to update carets only when carets are logically visible. A test case: 1. Type a string on the rocketbar until the text is long enough to scroll. 2. Copy arbitrary string. 3. Tap on rocketbar to show caret, and move it to the end (within 15 seconds timeout) 4. Type a character. The text selection dialog should not show.

Comment on attachment 8664133 [details] MozReview Request: Bug 1196176 - No need to test mLastUpdateCaretMode. r=mtseng https://reviewboard.mozilla.org/r/19923/#review18019

Comment on attachment 8664134 [details] MozReview Request: Bug 1196176 - Hide carets for mouse down reason. r=mtseng https://reviewboard.mozilla.org/r/19925/#review18023

Comment on attachment 8664135 [details] MozReview Request: Bug 1196176 - Fix CaretStateChanged not dispatch on empty content. r=mtseng https://reviewboard.mozilla.org/r/19927/#review18025

Comment on attachment 8664136 [details] MozReview Request: Bug 1196176 - Do not fire extra CaretStateChanged event when typing. r=mtseng https://reviewboard.mozilla.org/r/19929/#review18027

Morris, thank you for the review. Please help checkin both the gecko and gaia patch. Thanks.

gaia patch : https://github.com/mozilla-b2g/gaia/commit/4904b9ec4fab1178f9c8f3fb7290ee4166a81672

https://hg.mozilla.org/integration/b2g-inbound/rev/cd3d36c8fdae https://hg.mozilla.org/integration/b2g-inbound/rev/1c27eb1ea593 https://hg.mozilla.org/integration/b2g-inbound/rev/bd5678610d2a https://hg.mozilla.org/integration/b2g-inbound/rev/9204dc228884

Does the gaia patch works without the Gecko patch? Because currently the Gecko patch is not in m-c, and it might lead to bugs...

Yes, the gaia patch works by itself. It makes the STR in comment #0 reproducable so that I can fix the issue in gecko.

Thanks, now I'm at ease :)

https://hg.mozilla.org/mozilla-central/rev/cd3d36c8fdae https://hg.mozilla.org/mozilla-central/rev/1c27eb1ea593 https://hg.mozilla.org/mozilla-central/rev/bd5678610d2a https://hg.mozilla.org/mozilla-central/rev/9204dc228884