878843 - Use mozKeyboard.replaceSurroundingText in autocorrect

Status: RESOLVED FIXED

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

Description

[Jan Jongboom [:janjongboom] (inactive)]

Follow up of bug 878387. Use mozKeyboard.replaceSurroundingText instead of sending BACKSPACE and keycode events.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

Comment 2

[David Flanagan [:djf]]

Comment on attachment 757442 [details] [diff] [review]
Patch

I don't understand what the tools/extensions/  stuff is. It looks like a copy of the gecko patch, but in gaia. I didn't know we could do this. And I don't know what the rules are for landing stuff there.  Is this how we're doing uplifts into b2g18?  I suspect that I can't just r+ and land this. Some kind of approval is probably required.  Or is it just there for testing until the gecko patch is uplifted to the b2g18 tree?

Does adding extensions like this just work without compilation?  That is *so* much easier than rebuilding gecko.

If your intent is to permanently put this code in tools/extensions/ then Fabrice is probably a better reviewer. I'm switching this to him.

Andreas raised a concern in bug 861515 about the events that get sent when words are replaced this way. I don't think the concern was ever addressed in that bug, so I think we should address it here before landing this: what kind of events are generated when an auto-correction is done this way. We need to be sure that web content has some way to monitor changes to their text fields.  Is there a textinput event or something that is fired?

I'm happy with the gaia keyboard code. If some kind of event is triggered, and if Fabrice is okay with the tools/extensions/ stuff, then this looks good to me.

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

The tools/extensions/ stuff is a copy from Gecko, and it's being used to run the keyboard code in desktop Firefox Nightly. vingtetun introduced this in gaia codebase. Updated it so the keyboard doesn't break in desktop. The code already landed in mozilla-central, so that's why I copied this to gaia.

The only changes that need to be reviewed are in apps/keyboard.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 757442 [details] [diff] [review]
Patch

David, the extensions here are for the desktop helper, not something we ship on device.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Landed in https://github.com/mozilla-b2g/gaia/commit/181f88ac62bf16499653b4d1124b87adc9c85ca6

Comment 6

[David Flanagan [:djf]]

Reverted: https://github.com/mozilla-b2g/gaia/commit/34ba4788729e110bcaa689e7e02b3553c1dc40fd

Landing this patch now breaks autocorrect for all nightly builds. Only people who build their own versions of gecko will be able to use it.

We need to wait to land the patch or rewrite the patch to only use replaceSurroundingText when it is available.

Comment 7

[eshapiro]

I patched the code to fallback to sending backspaces if replaceSurroundingText is not supported: https://github.com/mozilla-b2g/gaia/pull/10174.

Comment 8

[eshapiro]

Attachment: patch with fallback for older builds of gecko

Comment 9

[David Flanagan [:djf]]

Comment on attachment 757736 [details]
patch with fallback for older builds of gecko

r+ technically, but there is lint in the patch.

Since Evan is not around, and this needs to land tonight, I'll fix the lint and land it.

Comment 10

[David Flanagan [:djf]]

Attachment: link to patch on github

This patch has a complicated history: Jan created it originally. Then I backed it out and Evan fixed it to work with versions of gecko both with and without replaceSurroundingText. Then I fixed the lint in Evan's patch. Fabrice reviewed Jan's original patch. I reviewed Evan's patch, and the lint fixes are unreviewed.

Comment 11

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/bc48562d5ede4d0c96f10557d64ccf9a5204253c

Comment 12

[Fabien Cazenave [:kaze]]

This causes a regression with MMS, see bug 883129.

Comment 13

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

Closing since the bug was never backed out.