1240286 - Deleting text with backspace causes the action bar to glitch

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

When I'm editing text in a text field or textarea with the action bar visible and hit backspace, the action bar glitches. Normally it shows the "check" icon on the left and "select all"/"paste" on the right. During the backspace it briefly switches to have a few more icons and then immediately switches back. The extra icons like "copy", "cut", and the three-dot menu button. If I hold down the backspace key this happens for every character deleted and is very distracting. I can post a video if needed. This is not a recent regression, it's been happening for a couple of weeks at least but I kept forgetting to file a bug.

Nightly on Nexus 4, using default keyboard.

Comment 1

[Mark Capella [:capella]]

Quick changes between selection state (CaretMode == None/Cursor/Selection) while the ActionBar is visible, causes this. Sounds like we could add a deferredAction timer to coalesce these, similar to what we use currently to smooth UI jank around fast selection close->re-open transitions [0].


[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=5a814b26505b,&mark=1150,1175#1140

Comment 2

[Mark Capella [:capella]]

Attachment: bug1240286.diff

The basic issue is we're generating eSetSelection events is nsWindow to position us at start of IME text replacement logic. This is being heard by the selectionChanged observer in AccessibleCaretManager and passed to the ActionBarHandler.

For the the testcase at [0], observed in [1], and using the STR:
   1) tap into the input field immediately before the final string |problem|
   2) tap backspace twice.

Each backspace "selects" a char then replaces it with nothing, via: [2]. The AccessibleCaret state transitions from:

   CaretMode::Cursor -> Selection -> Cursor -> Selection -> Cursor

The ActionBarHandler notices, and tries to display different UI actions based on those changing states, and jank ensues. Instead of covering the jank, we should probably add a new selectionChange reason code that AccessibleCaretManager just ignores in the first place.

Note for kats, this shouldn't regress Bug 1230617.


[0] https://www.dropbox.com/s/v3fsmm837m5gicq/testActionBar_bug1240286.html?dl=0
[1] https://www.dropbox.com/s/jet0qiuuftjclbr/bug1240286_backspace.mp4?dl=0

[2] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=88a086706e1e&mark=2879-2884#2851

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8711466 [details] [diff] [review]
bug1240286.diff

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

r+ for nsWindow.cpp change, but you need another reviewer for nsISelectionListener.idl and AccessibleCaretManager.cpp.

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8711466 [details] [diff] [review]
bug1240286.diff

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

The AccessibleCaret part looks good to me. You might want to update the uuid in nsISelectionListener.idl like you did in Bug 1215959 before asking smaug for reviewing the dom code.

Comment 5

[Mark Capella [:capella]]

Attachment: bug1240286.diff

Carryover r+ from jchen, assumed from TYLin

r? to smaug as suggested ...

I updated the uuid in the .idl, though I think already, or soon, this won't be required anymore:

   https://groups.google.com/forum/#!msg/mozilla.dev.platform/n6qBpyxoI6I/4qxwFvBOAgAJ

Comment 6

[Mark Capella [:capella]]

Attachment: bug1240286.diff

smaug had asked why I couldn't use the same selection IME_REASON code, and the answer was basically that I wanted OnImeReplaceText() to always ignore eSetSelection events.

My worry was re-using that reason code might invoke [0], which has the potential to open/display carets that weren't already visible.

Walking back my train of thought, I realized that with the fix of Bug 1230617, and some testing for verification, we no longer needed that bit and it should have been removed.

Having done that, IME_REASON becomes generically "always ignore", and can be safely re-used for this bug, greatly simplifying things.

I've tested on my N7 and my GS3/CM12.1, with both the stock Google keyboard and the SwiftKey keyboard, with auto-correction ON and OFF. Additionally on my GS3 I tested with the Android (AOSP) keyboard with auto-correction set to OFF and to AGGRESSIVE.

I observe that this fixes this bug, and does not regress the previous one, in all cases.


[0] http://mxr.mozilla.org/mozilla-central/source/layout/base/AccessibleCaretManager.cpp?rev=1b65eca00113&mark=121-125#106

Comment 7

[Mark Capella [:capella]]

Try push ...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5fc242eff22

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/343d3e913f20

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/343d3e913f20