Closed Bug 851565 Opened 8 years ago Closed 7 years ago

Inserting words from text prediction should keep user's capitalization

Categories

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

All
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, b2g18 fixed)

VERIFIED FIXED
1.1 QE2 (6jun)
blocking-b2g leo+
Tracking Status
b2g18 --- fixed

People

(Reporter: stas, Assigned: eshapiro)

References

Details

(Whiteboard: [TD-29915])

Attachments

(2 files)

Spin-off of bug 851314.

The code responsible for inserting the predicted word should capitalize it if the original word typed by the user was capitalized.  This already includes the case when the word is at the beginning of the sentence (because it will be typed by the user auto-capitalized).
Assignee: nobody → mozilla
Rudy, can you take a look at this? I think the engine should not be responsible for that. What do you think? Can you become the owner of this bug?
Flags: needinfo?(rlu)
I think this probably blocks bug 838227, at least from a usability perspective.
Blocks: 838227
Flags: needinfo?(rlu)
Oops, I didn't mean to clear the needinfo for Rudy.
Flags: needinfo?(rlu)
I'll take a look.
Assignee: mozilla → rlu
Flags: needinfo?(rlu)
Adding UX Most Wanted, as keyboard is a top quality-improvement priority and this glitch will be annoying once auto correction is enabled.
Whiteboard: u=user c=keyboard s=ux-most-wanted
I think this will be fixed by bug 860462. Even though that bug is about auto-correction, it also fixes this word correction bug.  So I'm taking this off of Rudy's list.
Assignee: rlu → dflanagan
Depends on: 860462
Adding whiteboard tags for tracking via srumbu.gs.
Whiteboard: u=user c=keyboard s=ux-most-wanted → c=keyboard
Whiteboard: c=keyboard → u=user c=keyboard s=ux-most-wanted
Blocks: 797170
Whiteboard: u=user c=keyboard s=ux-most-wanted → ux-tracking
Whiteboard: ux-tracking → ux-tracking, [TD-29915]
Target Milestone: --- → 1.1 QE2
This issue is not yet not solved in v1 train.
When user wants to write like,let say
"Configure Device" using word suggestion,
It will only be written like "configure device" which is not the same.

This issue need to be fixed for v1 train.
blocking-b2g: --- → leo?
Whiteboard: ux-tracking, [TD-29915] → ux-tracking
Target Milestone: 1.1 QE2 → ---
Whiteboard: ux-tracking → [TD-29915]
Resetting QE2 milestone, I think it was cleared by mistake.
Target Milestone: --- → 1.1 QE2
This issue is already fixed in master Gaia.
But not uplifted in V1 train.
Master and V1 train have different implementation way  of text prediction.
Will this be uplifted in V1 train?
Please comment.
Flags: needinfo?(dflanagan)
It has not yet been decided whether the new text prediction on master will be uplifted. At this point it seems unlikely.

I plan to fix this bug on v1-train before the QE2 deadline even if we do not uplift the new text prediction code.
Flags: needinfo?(dflanagan)
Blocks: 873934
Triage Leo+ for QE milesone
blocking-b2g: leo? → leo+
Priority: -- → P1
Assignee: dflanagan → eshapiro
Attachment #753375 - Flags: review?(dflanagan)
Comment on attachment 753375 [details]
link to github pull request

A few things to fix before landing this.  See github comments for details.

Also, we have a one-commit per patch rule, so please squash your commits and re-push.  You probably don't have to open a new pull request to do that.

I use 'git rebase -i' to squash.  Let me know if you'd like help with that.

When you've made the changes, just reset the review flag back to ?
Attachment #753375 - Flags: review?(dflanagan) → review-
Attachment #753375 - Flags: review- → review?(dflanagan)
Comment on attachment 753375 [details]
link to github pull request

The code looks good, and it works as desired on v1-train.

r=djf on commit b239dda
Attachment #753375 - Flags: review?(dflanagan) → review+
Landed on v1-train: https://github.com/mozilla-b2g/gaia/commit/37d7a218f06629b9e1dee6d3ce4d6a8276d84ae0

(This bug was already fixed on master, so this patch only needs to be on v1-train)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Hi David,

I have a review point for this already landed PR. Please check this:
https://github.com/mozilla-b2g/gaia/commit/37d7a218f06629b9e1dee6d3ce4d6a8276d84ae0#L0L284

Also made some few changes to support capitalization for any letter in the input string and suggestions will still support them. Could you please check them.
https://gist.github.com/leob2g/5641974
Flags: needinfo?(eshapiro)
Flags: needinfo?(dflanagan)
(In reply to Leo from comment #18)
> Hi David,
> 
> I have a review point for this already landed PR. Please check this:
> https://github.com/mozilla-b2g/gaia/commit/
> 37d7a218f06629b9e1dee6d3ce4d6a8276d84ae0#L0L284

Evan and I have discussed this. The code is safe as it stands and will never pass an empty string to isCapitalized(). (Because we never ask for predictions on an empty string, and we now check that the predictions match the current input.)

> 
> Also made some few changes to support capitalization for any letter in the
> input string and suggestions will still support them. Could you please check
> them.
> https://gist.github.com/leob2g/5641974

I think that we should only look at the first letter. Any capitalization of other letters is more likely to be a typing error that should be corrected than something intentional.

If you disagree, you should open a new bug, and we can get UX input for it.
Flags: needinfo?(eshapiro)
Flags: needinfo?(dflanagan)
(In reply to David Flanagan [:djf] from comment #20)

Hi David,

> Evan and I have discussed this. The code is safe as it stands and will never
> pass an empty string to isCapitalized(). (Because we never ask for
> predictions on an empty string, and we now check that the predictions match
> the current input.)


05-27 15:48:12.600: E/GeckoConsole(147): [JavaScript Error: "TypeError: word[0] is undefined" {file: "app://keyboard.gaiamobile.org/js/imes/latin/latin.js" line: 288}]

This is the log of the exception which is produced when empty string is passed.
Steps to produce log. in V1 train.
Precondition : Word suggestion should be ON
Action :  Message >> New Message >> The cursor will be displayed at "To" field.

Check for the exception of empty string
  if (input !== typedWord || typedWord.length == 0) {
    //user typing faster than suggestions provided or 
    //the length of typed word is zero
    keyboard.sendCandidates([]);
Please check at 
     https://gist.github.com/leob2g/5641974



> I think that we should only look at the first letter. Any capitalization of
> other letters is more likely to be a typing error that should be corrected
> than something intentional.

What if user wants to write whole word or sentence in Uppercase, this code will 
still display letters in lowercase.
I think this issue code inlcude this situation.
Please Give your suggestion,do we need to file other bug for this situation.
Thank you
Flags: needinfo?(dflanagan)
(In reply to Leo from comment #21)
> (In reply to David Flanagan [:djf] from comment #20)
> 
> Hi David,
> 
> > Evan and I have discussed this. The code is safe as it stands and will never
> > pass an empty string to isCapitalized(). (Because we never ask for
> > predictions on an empty string, and we now check that the predictions match
> > the current input.)
> 
> 
> 05-27 15:48:12.600: E/GeckoConsole(147): [JavaScript Error: "TypeError:
> word[0] is undefined" {file:
> "app://keyboard.gaiamobile.org/js/imes/latin/latin.js" line: 288}]
> 
> This is the log of the exception which is produced when empty string is
> passed.
> Steps to produce log. in V1 train.
> Precondition : Word suggestion should be ON
> Action :  Message >> New Message >> The cursor will be displayed at "To"
> field.

Thanks for this. Concrete bug reports like this are very helpful.  Is there any user impact?  If this is just a message in the log, I'm not too worried about it. We should fix it, but I don't feel strongly that the fix needs to be on v1-train if the user never sees a bug.

> 
> Check for the exception of empty string
>   if (input !== typedWord || typedWord.length == 0) {
>     //user typing faster than suggestions provided or 
>     //the length of typed word is zero
>     keyboard.sendCandidates([]);
> Please check at 
>      https://gist.github.com/leob2g/5641974
> 
> 

This just fixes the symptom. It would be better to fix the cause and figure out why suggestions are being requested in that case.

> 
> > I think that we should only look at the first letter. Any capitalization of
> > other letters is more likely to be a typing error that should be corrected
> > than something intentional.
> 
> What if user wants to write whole word or sentence in Uppercase, this code
> will 
> still display letters in lowercase.

I'm not sure what the right thing to do there is.  What do Android and iOS do? If we're going to support ALL CAPS, I'd say that the test should be something like: if the first 3 letters typed by the user are uppercase, convert all suggestions to all uppercase.

It seems like this is not an high-priority use case, though, and I'm not sure it is worth the time to fix this on v1-train.  Maybe on master.

> I think this issue code inlcude this situation.
> Please Give your suggestion,do we need to file other bug for this situation.
> Thank you

Please file two new bugs for the two issues.
Flags: needinfo?(dflanagan)
Thanks David,
I have raised two bugs relating to these two issues.

Bug 876612 - [Keyboard] [V1-Train] Word suggestion still shows in lowercase even if the user wants to write whole word or sentence in Uppercase.

Bug 876573 - [keyboard] "Warning : Exception is raised when empty string is passed to keyboard (V1 - Train)

Both issues are reproduced in base code of V1-TRAIN, 
Masters prediction code has been implemneted  differently.
(In reply to David Flanagan [:djf] from comment #22)

> Thanks for this. Concrete bug reports like this are very helpful.  Is there
> any user impact?  If this is just a message in the log, I'm not too worried
> about it. We should fix it, but I don't feel strongly that the fix needs to
> be on v1-train if the user never sees a bug.

Yeah you are right David. For now, there is no effect based on user experience.The user will not see this exception.
 

> I'm not sure what the right thing to do there is.  What do Android and iOS
> do? If we're going to support ALL CAPS, I'd say that the test should be
> something like: if the first 3 letters typed by the user are uppercase,
> convert all suggestions to all uppercase.

Android and iOS support this feature.
In Android, word suggestion is displayed same way the user inputs the characters,lowercase for lowercase,and uppercase for uppercase.
The first 3 letter you mentioned seems to be good way to apply uppercase for word
 
> It seems like this is not an high-priority use case, though, and I'm not
> sure it is worth the time to fix this on v1-train.  Maybe on master. 

These issues has been reproduced in base code of V1 - Train .I think these issues has been already fixed in Master Gaia since it has different implementation code for prediction.
Word suggestions are now displayed and entered with the same capitalization as entered by the user. This includes words not at the start of the sentence and acronyms (ex: IBM's).
Tested on b2g/nightly/mozilla-central-unagi/latest.

Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/mozilla-central/rev/3c6f2394995d
Gaia: d2ad0f1a5a6be8a2d6df721ba3c2b33e037c423b
Status: RESOLVED → VERIFIED
Please add a testcase for this bug to moztrap for 1.1 testsuite.  If yes, mark this in-moztrap+ when completed.  If not, mark this in-moztrap-.
Flags: in-moztrap?
Added Keyboard Suite Test Case #8447: [Keyboard] Text prediction text entry keeps the capitalization entered by the user
Flags: in-moztrap? → in-moztrap+
This patch was reverted as part of bug 873934, but then equivalent code was uplifted from master to v1-train.  This remains fixed in v1-train
You need to log in before you can comment on or make changes to this bug.