Staś Małolepszy :stas Reporter
	Description • 12 years ago

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).

	Christoph Kerschbaumer [:ckerschb]
	Comment 1 • 12 years ago

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?

	Dylan Oliver [:doliver]
	Comment 2 • 12 years ago

I think this probably blocks bug 838227, at least from a usability perspective.

	Dylan Oliver [:doliver]
	Comment 3 • 12 years ago

Oops, I didn't mean to clear the needinfo for Rudy.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 4 • 12 years ago

I'll take a look.

	Josh Carpenter [:jcarpenter]
	Comment 5 • 12 years ago

Adding UX Most Wanted, as keyboard is a top quality-improvement priority and this glitch will be annoying once auto correction is enabled.

	David Flanagan [:djf]
	Comment 6 • 11 years ago

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.

	Mike Lee [:mlee]
	Comment 7 • 11 years ago

Adding whiteboard tags for tracking via srumbu.gs.

	Leo
	Comment 8 • 11 years ago

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.

	Dylan Oliver [:doliver]
	Comment 9 • 11 years ago

Resetting QE2 milestone, I think it was cleared by mistake.

	Leo
	Comment 10 • 11 years ago

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.

	David Flanagan [:djf]
	Comment 11 • 11 years ago

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.

	Wayne Chang [:wchang]
	Comment 12 • 11 years ago

Triage Leo+ for QE milesone

	eshapiro Assignee
	Comment 14 • 11 years ago

Comment on attachment 753375 [details]
link to github pull request

><a href="https://github.com/mozilla-b2g/gaia/pull/9971/">
https://github.com/mozilla-b2g/gaia/pull/9971/
</a>

	David Flanagan [:djf]
	Comment 15 • 11 years ago

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 ?

	David Flanagan [:djf]
	Comment 16 • 11 years ago

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

	David Flanagan [:djf]
	Comment 17 • 11 years ago

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)

	Leo
	Comment 18 • 11 years ago

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

	David Flanagan [:djf]
	Comment 20 • 11 years ago

(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.

	Leo
	Comment 21 • 11 years ago

(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

	David Flanagan [:djf]
	Comment 22 • 11 years ago

(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.

	Leo
	Comment 23 • 11 years ago

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.

	Leo
	Comment 24 • 11 years ago

(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.

	:CSchmoeckel
	Comment 25 • 11 years ago

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

	Tony Chung [:tchung]
	Comment 26 • 11 years ago

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-.

	:CSchmoeckel
	Comment 27 • 11 years ago

Added Keyboard Suite Test Case #8447: [Keyboard] Text prediction text entry keeps the capitalization entered by the user

	David Flanagan [:djf]
	Comment 28 • 11 years ago

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