Closed Bug 808740 Opened 7 years ago Closed 5 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

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 ;) )
Duplicate of this bug: 925300
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) — 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) — 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)
Duplicate of this bug: 1042011
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).
Duplicate of this 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.
Duplicate of this bug: 1054751
Let's see if I can fix this.
Flags: needinfo?(janjongboom)
Attached file Patch v3
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] [review]
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)
Duplicate of this bug: 1047992
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] [review]
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? → ---
Duplicate of this bug: 823477
Duplicate of this bug: 1065935
Duplicate of this bug: 1101479
Attached video video of issue verify
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.