1075970 - [Keyboard] Changing text fields can cause the keyboard to intermittently stop displaying character selection indicators properly.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Martin Shuman [:Marty]]

Attachment: logcat-Keyboard.txt

Description:
Changing from one text field to another will often prevent the character selection pop-ups from displaying as the user types.

Note: Long pressing a character like 'a' will bring up the list of Special Character options improperly above the keyboard on the left side (see attached screenshot).

This issue is most noticable when switching from the Username to the Password fields of a log-in page, or when switching from a text field to the Rocketbar.  This issue has been seen in the Facebook app, Rocketbar, Contacts, and inputting a WiFi password.
   
Repro Steps:
1) Update a Flame device to BuildID: 20141001040205
2) Download and launch the Facebook app from the Marketplace
3) Input text into the Username/Email field of the log-in page.
4) Change to the Password field and input text.
5) Change back to the Username field and input more text
6) Repeat steps 4 and 5 several times.
  
Actual:
Keyboard stops displaying character selection indicator pop-ups
  
Expected: 
Keyboard continues to display character selection indicator pop-ups properly
  
Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20141001040205
Gaia: 0e280591881d44b80f456bc27e12d9114c218868
Gecko: 14665b1de5ee
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
  
Repro frequency: 2/5
See attached: screenshot, video clip, logcat

-------------------------------------------------------
This issue does NOT occur in Flame 2.1.
Keyboard character selection indicators are consistently displayed properly.


Environmental Variables:
Device: Flame 2.1 319MB
BuildID: 20141001000202 (Full Flash)
Gaia: 86a3626055ed58b39525424126870dc0a503e79f
Gecko: c20912812877
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 1

[Martin Shuman [:Marty]]

Attachment: Keyboard_Character.png

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Regression of core functionality of keyboard app.

Requesting a window.

Comment 3

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Cause: Bug 1044525 seems to be the cause for this issue.

B2g-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 2.2
BuildID: 20140929035801
Gaia: 44c06869d9cf80d74157ea94b161b572f918a793
Gecko: f8237e7ef676
Version: 35.0a1 (2.2) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

First Broken 
Environmental Variables:
Device: Flame 2.2
BuildID: 20140929042800
Gaia: afeb7316826fb2172070e27632270224bc2ced13
Gecko: 59e54655f006
Version: 35.0a1 (2.2) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Last Working gaia / First Broken gecko - Issue does NOT occur
Gaia: 44c06869d9cf80d74157ea94b161b572f918a793
Gecko: 59e54655f006

First Broken gaia / Last Working gekko - Issue DOES occur
Gaia: afeb7316826fb2172070e27632270224bc2ced13
Gecko: f8237e7ef676

Gaia Pushlog: https://github.com/mozilla-b2g/gaia/compare/44c06869d9cf80d74157ea94b161b572f918a793...afeb7316826fb2172070e27632270224bc2ced13

Comment 4

[Joshua Mitchell (Inactive)]

broken by Bug 1044525 - can you take a look?

Comment 5

[John Lu [:mnjul][Please NI or I'll miss]]

Sure. It does look like something that has to do with my patch. Keeping NI for reminding.

Comment 6

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Patch (PR @ Github)

The root cause of this bug is that we can have multiple rendered keyboard layouts for a single layout, and that multiplicity comes from different input types, whether you allow candidate panels, and so forth [1].

When we switch back to already-drawn layouts, the renderer reuses rendered layouts, and bypasses my TargetObj/DomElem map-setting routines; so when you switch to (for example) another input type and back to the original input type, the reverse map (TargetObj to DOMElement) is still mapping to the "another" input type's rendered layout.

As such, the reverse map (TargetObj to DOMElement) shall be able to map from one target object to multiple DOMElements (of different containers that can be indexed through keyboardClass as in [1]).

In this patch, the value of the reverse WeakMap is now an object, holding multiple DOMElements for that target object key'ed by the keyboardClass decided in [1]. Then we need to keep track of the currently drawn keyboardClass such that the mapping can work correctly. [2]

An alternative way to resolve this bug is to regenerate the whole reverse map when we reuse rendered keyboard layouts and bypass the actual rendering logics.

Tim, could you see if this amends the architectural design reasonably?

[1] https://github.com/mnjul/gaia/blob/b540d1c7b492555ba7ab8bd6a562413380107172/apps/keyboard/js/render.js#L131-L153

[2] On a second thought, I think we should use "currentlyDisplayedKeyboardClass" for that variable's name. That will be changed if not objected.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8499438 [details] [review]
Patch (PR @ Github)

This is a sad band-aid, but I am sure we could dismantle render.js to make this part of keyboard app saner in the future.

Comment 8

[John Lu [:mnjul][Please NI or I'll miss]]

TBPL testing before final landing: https://tbpl.mozilla.org/?rev=9845f86f0fc05f6c5ef839d94c4e6a7079e99253&tree=Gaia-Try

Comment 9

[John Lu [:mnjul][Please NI or I'll miss]]

Master: https://github.com/mozilla-b2g/gaia/commit/1d153c6a9fbbdf50191673d92b3d2aa24d17cb31

Comment 10

[John Lu [:mnjul][Please NI or I'll miss]]

Backed out for causing bug 1079091: https://github.com/mozilla-b2g/gaia/commit/636fdf9ae94a491fa90aeff096d24bfe2a94da8e

I'll post a revised patch later.

Comment 11

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Patch v2 (PR @ GitHub)

So here's a better patch to address this bug and it should probably risk the overall architecture less (if any) against further regressions.

As observed with this bug and bug 1079091, we have to ensure that a DOM element mapped to a target object can be mapped back to that very DOM element correctly. To achieve that, with this patch, when we insert an entry into the maps, we now generate a "reference stub" to the target object by |Object.create(targetObj)| and that "reference stub" is used for map look-up everywhere.

So, each rendered DOM layout key has a reference stub unique from that key of another rendered DOM layout, and the mappings should work correctly.

The created reference stubs are also |Object.freeze()|'en after being created to guard against unnecessary manipulation.

Tim & Rudy, could you check if this looks good (and tests good if you'd like to test)?

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8502247 [details] [review]
Patch v2 (PR @ GitHub)

Looks simple and good if you could confirm this indeed fix the issues mentioned.

Comment 13

[John Lu [:mnjul][Please NI or I'll miss]]

Master: https://github.com/mozilla-b2g/gaia/commit/8c3608c83ddc64cdfa5159ce216ad77f72cc83c9

Comment 14

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8502247 [details] [review]
Patch v2 (PR @ GitHub)

(Offline discussed with Rudy and he did not have concerns with this patch)

Comment 15

[KTucker [:KTucker][Inactive 3/4/2016]]

Adding qawanted to verify that this issue has been fixed.

Comment 16

[Seth Miko [:SMiko]]

Verified fixed on Flame 2.2 (319mb/full flash) 

Actual result: Keyboard continues to display character selection indicator pop-ups properly

Device: Flame 2.2
BuildID: 20141012040203
Gaia: 717ad4e8b7fc10ab8248500d00ba5ba0977fa8ab
Gecko: 44168a7af20d
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Blocking 2.2+ for all fixed regressions.