Closed Bug 808740 Opened 12 years ago Closed 10 years ago

French typography and word prediction in Gaia are not in sync

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.0 verified, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S4 (12sep)
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: gerard-majax, Assigned: janjongboom)

References

Details

Attachments

(2 files, 2 obsolete files)

This bugs is a little child of bug 808015, about the issue of exclamation mark, and probably others. Basically, in french typography, '!', ';', ':' and some others must be preceded and followed by space, and at least the preceding one should be non breaking. The current behavior when word prediction is enabled is not following this (maybe complex) rule. As far as I can tell, for autocapitalization on my Android 2.3.3 device (HTC Desire Z), when you press backspace and retype the letter, it does not get autocapitalized. Maybe the same kind of behavior could be implemented in Gaia's virtual keyboard: if what has been rewritten is being erased by the user, then do not bother to re-modify it; i.e. if the user types 'Hello ! I'm a user.', it gets changed to 'Hello! I'm a user.', let's say we just type 'Hello ! ', which gets changed to 'Hello! ', then we press backspace twice and retype ' !', I would expect the keyboard to to rechange it to '! '. Given the current code status, I get this kind of behavior might be tricky to implement. According to djf when I explained to him, this is linked to word prediction. This feature is cool and works not that bad. Maybe the ponctuation change could be disabled on a locale-basis ? This way it would not get in the way of the user, and it's an easier code change to verify.
Summary: French typography and word predicition in Gaia are not in sync → French typography and word prediction in Gaia are not in sync
This issue is still happening on Buri, latest v1.2 Com Ril Gaia 2ef9bc3c7a6de228b63e6ba3613eb0c0dd639c59 SourceStamp 4a94d2ea9d37 BuildID 20131028004002 Version 26.0a2 Would be nice to have this fixed since Geeksphones are distributing French in their builds (and we will probably need it soon anyways ;) )
Jan, Any ETA for this? We are shipping French on 1.3
Flags: needinfo?(janjongboom)
Are you addressing this in the re-architecture thing? @Theo, this won't be in 1.3.
Flags: needinfo?(janjongboom) → needinfo?(timdream)
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #6) > Are you addressing this in the re-architecture thing? No. Reading this bug I don't think the fix is in any way different with or without bug 956169 or bug 1023729. latin.js (renamed to prediction.js in the dev_app/demo-keyboard) is where we should address this bug. It's the right place to fix too now and forever because it supposed to work with all Latin-based script. :)
Flags: needinfo?(timdream)
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #6) > Are you addressing this in the re-architecture thing? > > @Theo, this won't be in 1.3. I know, this was just to stress on the fact that we are now officially shipping French. Could be great to get that fixed on 2.0, though.
After talking with several people, we should get that fixed. Beyond the fact that this behavior is wrong in French, this is really frustrating when you're typing. Nominating for 2.0
blocking-b2g: --- → 2.0?
+1 French is shipping soon, I agree we should absolutely get a fix for this!
Too risky to consider for 2.0 at this point, so not blocking.
blocking-b2g: 2.0? → backlog
(In reply to Jason Smith [:jsmith] from comment #11) > Too risky to consider for 2.0 at this point, so not blocking. How do we know that this is risky?
(In reply to Fabrice Desré [:fabrice] from comment #12) > (In reply to Jason Smith [:jsmith] from comment #11) > > Too risky to consider for 2.0 at this point, so not blocking. > > How do we know that this is risky? I thought that was comment 6 was implying.
Attached patch punctuation.patch (obsolete) (deleted) — Splinter Review
This patch let keyboard layout opt out from the punctuation auto-correction, which is the issue we have with the French layouts.
Attachment #8450696 - Flags: review?(timdream)
Comment on attachment 8450696 [details] [diff] [review] punctuation.patch I think the variable should be put into dataPromise in switchIMEngine() and send to latin.js from inputMethodManager.switchCurrentIMEngine(). I don't think the approach of this patch is good enough compare to what I said above because that value does not change during runtime, so it shouldn't an API in the InputMethodGlue.
Attachment #8450696 - Flags: review?(timdream)
Something like { suggest: values[1].suggestionsEnabled && !isGreekSMS(), correct: values[1].correctionsEnabled && !isGreekSMS(), correctPunctuation: layoutLoader.currentModifiedLayout.autoCorrectPunctuation } in switchIMEngine() Note that this should be |autoCorrectPunctuation| with capital C.
Attached patch punctuation.patch v2 (obsolete) (deleted) — Splinter Review
Thanks Tim, that's simpler indeed!
Assignee: nobody → fabrice
Attachment #8451105 - Flags: review?(timdream)
Comment on attachment 8451105 [details] [diff] [review] punctuation.patch v2 r+ if you have tested this manually. Unfortunately this part of code is not covered in any unit tests. We (the non-Franch speakers :P) can make our best effort not to regress this feature and maybe adding unit tests when the code is finally refactored, but if that sub-optimal you may want to add integration tests in the mean time.
Attachment #8451105 - Flags: review?(timdream) → review+
We actually have tests, and I broke them ;) (https://tbpl.mozilla.org/?rev=e9f91a8011e01afa49a59ac9359442670b7b447e&tree=Gaia-Try) Gonna fix that before landing...
I am now working on bug 1041411 and I will touch the code in the patch here. Please do try to land this bug before I do so you don't have to rebase :)
Flags: needinfo?(fabrice)
Thanks for the heads up Tim! I'll try to fix the tests, but it's not high on my list right now :(
Flags: needinfo?(fabrice)
The changing of ' !' to '! ' and the likes is very annoying, especially for French, and sometimes messy (in daily usage of trunk fxos, I sometimes saw it delete real letters instead of a space). Additionally to adapting it to French punctuation rules, I think this kind of punctuation auto-correct should be disabled entirely if auto-correct is off (see bug 1020200).
[Blocking Requested - why for this release]: This should have been fixed for the initial release of a phone in France. It would be awesome to get it fixed before the next release.
blocking-b2g: backlog → 2.1?
Rik, if you have time to investigate the test failures with my patch this is all needs to be fixed before we land in 2.1
I don't have time. That's why I'm asking this to be a blocker, to be sure that it's on the radar of the Keyboard team.
Let's see if I can fix this.
Flags: needinfo?(janjongboom)
Attached file Patch v3 (deleted) —
So keyboard changed a lot since patch v2, so had to redo it. Also added tests. Tested on device and seems to work nice.
Assignee: fabrice → janjongboom
Attachment #8450696 - Attachment is obsolete: true
Attachment #8451105 - Attachment is obsolete: true
Attachment #8482387 - Flags: review?(timdream)
Flags: needinfo?(janjongboom)
Comment on attachment 8482387 [details] Patch v3 Looks good and thanks!
Attachment #8482387 - Flags: review?(timdream) → review+
This should land on v2.1 so bug 1061439 (a v2.1 blocker) can land on top of it. Jan, could you request for approval?
Flags: needinfo?(janjongboom)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #33) > This should land on v2.1 so bug 1061439 (a v2.1 blocker) can land on top of > it. > > Jan, could you request for approval? Is that the right issue? I don't see the relevance to this one.
Flags: needinfo?(janjongboom) → needinfo?(timdream)
git told me to resolve conflict here https://github.com/mozilla-b2g/gaia/pull/23655/files?diff=unified#diff-8a5c7281da7398a20e7b65db6c6d02bbR495 while I rebase that patch yesterday. instead of creating a 2.1 specific patch, I figured we should just uplift the feature here.
Flags: needinfo?(timdream)
Verified fixed on 2.2 with: Gaia af04d8bc2111d4ea146239a89ff602206b85eaf5 Gecko https://hg.mozilla.org/mozilla-central/rev/acbdce59da2f BuildID 20140903160205 Version 35.0a1 ro.build.version.incremental=110 ro.build.date=Fri Jun 27 15:57:58 CST 2014 B1TC00011230 Thanks a lot for this patch! :)
Status: RESOLVED → VERIFIED
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #36) > git told me to resolve conflict here > > https://github.com/mozilla-b2g/gaia/pull/23655/files?diff=unified#diff- > 8a5c7281da7398a20e7b65db6c6d02bbR495 > > while I rebase that patch yesterday. > > instead of creating a 2.1 specific patch, I figured we should just uplift > the feature here. I like my patches in and stuff, but to me this would only make sense if French would be a target language for 2.1.
Flags: needinfo?(timdream)
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #38) > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from > comment #36) > > git told me to resolve conflict here > > > > https://github.com/mozilla-b2g/gaia/pull/23655/files?diff=unified#diff- > > 8a5c7281da7398a20e7b65db6c6d02bbR495 > > > > while I rebase that patch yesterday. > > > > instead of creating a 2.1 specific patch, I figured we should just uplift > > the feature here. > > I like my patches in and stuff, but to me this would only make sense if > French would be a target language for 2.1. It is, Tako requirement.
Comment on attachment 8482387 [details] Patch v3 [Approval Request Comment] [Bug caused by] (feature/regressing bug #): Implementation of auto punctuation in 1.0 (or 1.1, before I joined) [User impact] if declined: Bad experience for users of languages where English punctuation rules do not apply, like French. Requirement for Tako. [Testing completed]: No [Risk to taking this patch] (and alternatives if risky): It's part of core keyboard, but haven't seen any bugs that might be related to this since landing. However I don't know how much users are on 2.2. [String changes made]: -
Attachment #8482387 - Flags: approval-gaia-v2.1?
Flags: needinfo?(timdream)
Attachment #8482387 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Unset flag because this is already fixed in v2.1.
blocking-b2g: 2.1? → ---
Attached video video of issue verify (deleted) —
This issue has been verified successfully on Flame v2.1 & v2.2 STR: 1. Launch Message app. 2. Use French typography to input "Hello". 3. Tap Space key and then input "!". **It will show "Hello !". See attachment: verify_video.MP4 Reproducing rate: 0/5 Flame 2.1 versions: Gaia-Rev 5372b675e018b6aac97d95ff5db8d4bd16addb9b Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b Build-ID 20141127001201 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141127.035005 FW-Date Thu Nov 27 03:50:16 EST 2014 Bootloader L1TC00011880 Flame 2.2 versions: Gaia-Rev 80bc1445959db79e9d2e947cc56e1eb7b0d3d0f0 Gecko-Rev https://hg.mozilla.org/mozilla-central/rev/7dfad34d265b Build-ID 20141127040204 Version 36.0a1 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141127.074535 FW-Date Thu Nov 27 07:45:46 EST 2014 Bootloader L1TC00011880
For tracking purposes - A user of Firefox OS (ZTE Open C v1.3) reported this issue in the SUMO forums: https://support.mozilla.org/en-US/questions/1041274
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: