Closed Bug 1196176 Opened 4 years ago Closed 4 years ago

Text selection dialog does not update its position in shortcut mode

Categories

(Core :: DOM: Selection, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
FxOS-S8 (02Oct)
Tracking Status
firefox43 --- affected

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 2 obsolete files)

Attached image paste_shortcut.png
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.
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Depends on: 1204872
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
Attachment #8663568 - Flags: review?(felash)
(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] [review]
[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 ?
Attachment #8663568 - Flags: review?(felash) → review+
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
Depends on: 571294
> 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 ?
Blocks: 1207005
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.
Attachment #8664133 - Flags: review?(mtseng)
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.
Attachment #8664134 - Flags: review?(mtseng)
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.
Attachment #8664135 - Flags: review?(mtseng)
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.
Attachment #8664136 - Flags: review?(mtseng)
Comment on attachment 8664133 [details]
MozReview Request: Bug 1196176 - No need to test mLastUpdateCaretMode. r=mtseng

https://reviewboard.mozilla.org/r/19923/#review18019
Attachment #8664133 - Flags: review?(mtseng) → review+
Comment on attachment 8664134 [details]
MozReview Request: Bug 1196176 - Hide carets for mouse down reason. r=mtseng

https://reviewboard.mozilla.org/r/19925/#review18023
Attachment #8664134 - Flags: review?(mtseng) → review+
Attachment #8664135 - Flags: review?(mtseng) → review+
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
Attachment #8664136 - Flags: review?(mtseng) → review+
Morris, thank you for the review.

Please help checkin both the gecko and gaia patch. Thanks.
Keywords: checkin-needed
Attachment #8664734 - Attachment is obsolete: true
gaia patch : https://github.com/mozilla-b2g/gaia/commit/4904b9ec4fab1178f9c8f3fb7290ee4166a81672
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S8 (02Oct)
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 :)
You need to log in before you can comment on or make changes to this bug.