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)
Tracking
(b2g-v2.0 verified, b2g-v2.1 verified, b2g-v2.2 verified)
VERIFIED
FIXED
2.1 S4 (12sep)
People
(Reporter: gerard-majax, Assigned: janjongboom)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
text/x-github-pull-request
|
timdream
:
review+
fabrice
:
approval-gaia-v2.1+
|
Details |
(deleted),
video/mp4
|
Details |
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.
Updated•12 years ago
|
Summary: French typography and word predicition in Gaia are not in sync → French typography and word prediction in Gaia are not in sync
Comment 1•12 years ago
|
||
Code is located here :
https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/latin/latin.js#L470
Comment 2•11 years ago
|
||
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 ;) )
Assignee | ||
Comment 4•11 years ago
|
||
We're planning to fix this in https://bugzilla.mozilla.org/show_bug.cgi?id=956169
Comment 5•10 years ago
|
||
Jan, Any ETA for this? We are shipping French on 1.3
Flags: needinfo?(janjongboom)
Assignee | ||
Comment 6•10 years ago
|
||
Are you addressing this in the re-architecture thing?
@Theo, this won't be in 1.3.
Flags: needinfo?(janjongboom) → needinfo?(timdream)
Comment 7•10 years ago
|
||
(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)
Comment 8•10 years ago
|
||
(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.
Comment 9•10 years ago
|
||
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?
Comment 10•10 years ago
|
||
+1
French is shipping soon, I agree we should absolutely get a fix for this!
Comment 11•10 years ago
|
||
Too risky to consider for 2.0 at this point, so not blocking.
blocking-b2g: 2.0? → backlog
Comment 12•10 years ago
|
||
(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?
Comment 13•10 years ago
|
||
(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.
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
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.
Comment 17•10 years ago
|
||
Thanks Tim, that's simpler indeed!
Assignee: nobody → fabrice
Attachment #8451105 -
Flags: review?(timdream)
Comment 18•10 years ago
|
||
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+
Comment 19•10 years ago
|
||
We actually have tests, and I broke them ;) (https://tbpl.mozilla.org/?rev=e9f91a8011e01afa49a59ac9359442670b7b447e&tree=Gaia-Try)
Gonna fix that before landing...
Comment 20•10 years ago
|
||
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)
Comment 21•10 years ago
|
||
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)
Comment 23•10 years ago
|
||
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).
Comment 25•10 years ago
|
||
[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?
Comment 26•10 years ago
|
||
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
Comment 27•10 years ago
|
||
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.
Assignee | ||
Comment 30•10 years ago
|
||
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 31•10 years ago
|
||
Comment on attachment 8482387 [details]
Patch v3
Looks good and thanks!
Attachment #8482387 -
Flags: review?(timdream) → review+
Assignee | ||
Comment 32•10 years ago
|
||
Green try https://tbpl.mozilla.org/?rev=db40382677f20f87c19366250926caf6f89cdfc8&tree=Gaia-Try
Landed https://github.com/mozilla-b2g/gaia/commit/615be1f67ace487969b04110168486883f6b55c4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Comment 33•10 years ago
|
||
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)
Assignee | ||
Comment 34•10 years ago
|
||
(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)
Comment 36•10 years ago
|
||
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)
Comment 37•10 years ago
|
||
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
Assignee | ||
Comment 38•10 years ago
|
||
(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)
Comment 39•10 years ago
|
||
(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.
Assignee | ||
Comment 40•10 years ago
|
||
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)
Comment 41•10 years ago
|
||
French is one of the shipping locale.
https://l10n.mozilla.org/shipping/dashboard?tree=gaia
Updated•10 years ago
|
Attachment #8482387 -
Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Comment 42•10 years ago
|
||
Target Milestone: --- → 2.1 S4 (12sep)
Comment 47•10 years ago
|
||
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
Comment 48•10 years ago
|
||
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.
Description
•