Closed Bug 1085764 Opened 5 years ago Closed 3 years ago

[flameKK][v2.1]Be able to input Simplified-Chinese in SMS subject after reaching maxium length

Categories

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

All
Gonk (Firefox OS)
defect

Tracking

(tracking-b2g:backlog)

RESOLVED WONTFIX
tracking-b2g backlog

People

(Reporter: xshen, Unassigned)

References

Details

(Whiteboard: [p=2][sms-most-wanted])

Attachments

(3 files)

Steps:
1)Choose Language to "Simplified Chinese"
2)Go to homescreen->Message->New Message->click on "..." to pop up "Add Subject"
3)choose "拼" on the keyboard(Original is En on the left bottom of keyboard)
4)Input some Chinese Characters(more than 5 lines) in the subject and observe the screen.

Expected Result:
when reaching maxinum of length of subject, there will be a notification of "Maxinum length of subject reached" and the user is not able to input anything.

Acutal Result:
1)when user input Chinese charactors by using "pin" on the keyboard, no notification of "Maxinum length of subject reached" is displayed.
2)when user delete some Chinese charactors and notification of “maxinum length of subject reached" is displayed.
3)After that, user is still able to input a lot of Chinese charactors.

Build version:
20141020001201

Device:
flame kk(v180 base)
Hey Steve, can you check this?

From what I looked in our code, I don't see how this is possible, but I'm sure you'll understand this better :)
Flags: needinfo?(schung)
I think this issue is similar to word suggestion issue: When user trying to insert a suggested word that makes overall length exceed the limitation, we can not prevent this input since it's not triggered by keydown event. One more problem in Chinese IME is the suggestion list is not only generated by the typed word on the input, but also spelling word that cached in memory. And keyDown preventDefault didn't stop caching this for avoiding the suggestion list display. I think the reasonable solution is in gecko input method module, so ni? Rudy for confirmation. 

BTW, we also discuss the possible solution of maxlength property for input element. Hi Julien, could you think of why we didn't simply apply this at first?
Flags: needinfo?(schung)
Flags: needinfo?(rlu)
Flags: needinfo?(felash)
(In reply to Steve Chung [:steveck] from comment #2)
> I think this issue is similar to word suggestion issue: When user trying to
> insert a suggested word that makes overall length exceed the limitation, we
> can not prevent this input since it's not triggered by keydown event. One
> more problem in Chinese IME is the suggestion list is not only generated by
> the typed word on the input, but also spelling word that cached in memory.
> And keyDown preventDefault didn't stop caching this for avoiding the
> suggestion list display. I think the reasonable solution is in gecko input
> method module, so ni? Rudy for confirmation. 

Maybe we can have another event that we can cancel?

> 
> BTW, we also discuss the possible solution of maxlength property for input
> element. Hi Julien, could you think of why we didn't simply apply this at
> first?

Because we use a contenteditable so that we can have new lines.
The first implementation used maxlength :)
Flags: needinfo?(felash)
To solve this case, it seems we have 2 options, 
 Option 1: still do preventDefault() in keydown (or keypress) event when it exceeds char count limit.
           However, for now, we don't have this event when you click word suggestion in Chinese keyboard.
           

 Option 2: Trim the value of the input field in "input" event.
           This may encounter the same issue as bug 1082394, that the input field would be cleared during text composition.

So, it seems not that easy to solve this kind of issue without using maxlength.
Anther option would be just keep the user input as is, and set the trimmed text content to the API, but this would involve UX change, so maybe we could evaluate this after we make sure the above 2 options are impossible or difficult enough.

Yuan,
Could you please also check if it is possible to do option 1?
Thanks.
Flags: needinfo?(rlu) → needinfo?(xyuan)
See Also: → 1082394
Steve, we don't have the notification at step 1), is it because suggestions do not trigger any event? Maybe this happens in english too?

Rudy, step 3, the user is inputting "normal" chinese characters, not using the suggestion; how come we don't prevent this? Do we input chinese characters differently than latin characters?
Flags: needinfo?(schung)
Flags: needinfo?(rlu)
(In reply to Rudy Lu [:rudyl] from comment #4)
> To solve this case, it seems we have 2 options, 
>  Option 1: still do preventDefault() in keydown (or keypress) event when it
> exceeds char count limit.
>            However, for now, we don't have this event when you click word
> suggestion in Chinese keyboard.
>            
> 
>  Option 2: Trim the value of the input field in "input" event.
>            This may encounter the same issue as bug 1082394, that the input
> field would be cleared during text composition.
> 
> So, it seems not that easy to solve this kind of issue without using
> maxlength.
> Anther option would be just keep the user input as is, and set the trimmed
> text content to the API, but this would involve UX change, so maybe we could
> evaluate this after we make sure the above 2 options are impossible or
> difficult enough.
> 
> Yuan,
> Could you please also check if it is possible to do option 1?
> Thanks.
Sorry, I don't understand what you want to do in option 1. But option 2 is feasible, as I found a way to solve text composition issue of bug 1082394 (see bug 1082394 comment 11).
Flags: needinfo?(xyuan)
(In reply to Julien Wajsberg [:julienw] from comment #5)
> Steve, we don't have the notification at step 1), is it because suggestions
> do not trigger any event? Maybe this happens in english too?
> 
> Rudy, step 3, the user is inputting "normal" chinese characters, not using
> the suggestion; how come we don't prevent this? Do we input chinese
> characters differently than latin characters?
Yes.
For inputting Chinese chars, we use InputMethod API's setComposition() and endComposition() methods to submit the chars instead of plain sendKey() as in sending Latin chars.

And setComposition() related APIs would not trigger keydown event if I understand correctly.
  

--
Yuan, what I said in option 1 is about we won't have keydown/keypress event for setComposition() call, and we cannot change that, right?
Flags: needinfo?(rlu)
We just verified the patch in Bug 1082394 work like a charm, which is option 2 without the aforementioned problem.

Please help give a try to see if the same solution could be applied to Message app.
Thanks.
(In reply to Rudy Lu [:rudyl] from comment #7)
> --
> Yuan, what I said in option 1 is about we won't have keydown/keypress event
> for setComposition() call, and we cannot change that, right?
See bug 354358. It is controversial whether or not to dispatch key events during composition and we tend not to do so.
[Blocking Requested - why for this release]: broken functionality with the chinese keyboard (and maybe other keyboards as well?)
blocking-b2g: --- → 2.1?
what would happen if user proceed to send out the message with long title (which exceeds the ma length)?
Flags: needinfo?(echang)
Attached image MMS-sent-failedpng.png
Same as what I had.
(In reply to xshen from comment #12)
> Created attachment 8511869 [details]
> MMS-sent-failedpng.png
Flags: needinfo?(echang)
When the message with long title which exceeds the max length, the MMS is failed to be sent out. Please see the screenshot MMS-sent-failedpng.png.
(In reply to Julien Wajsberg [:julienw] from comment #5)
> Steve, we don't have the notification at step 1), is it because suggestions
> do not trigger any event? Maybe this happens in english too?
> 
Yes, suggestions won't trigger the keydown event in English keyboard. It's not a regression but existed at very beginning.
Flags: needinfo?(schung)
Assignee: nobody → schung
To triage: broken funcionality for chinese/other keyboard, but also when adding suggestions that exceed the limit in any keyboard.
Whiteboard: [p=2]
Target Milestone: --- → 2.1 S8 (7Nov)
I see two issues here once about input method only for Chinese and the second is suggestion which should be a different bug ? Can we file two separate bugs here ? Also  are we sure this has impacted other keyboard. Given this is not a new regression and since we may not be shipping 2.1 in Chinese markets I am hesitant to block on this.
just quick test from my side: 
i can reproduce with ENG typing. With suggestion, I can exceed the limit.
The message won't get sent out like comment 12.
Flags: needinfo?(echang)
triage: non-blocking.
reason: not a regression. not a very common case to key in more than 40 characters/letters.
blocking-b2g: 2.1? → backlog
(In reply to bhavana bajaj [:bajaj] from comment #17)
> I see two issues here once about input method only for Chinese and the
> second is suggestion which should be a different bug ? Can we file two
> separate bugs here ?

It's actually the same bug that appears in 2 different cases.
Remove from sprint because it's not a blocker anymore.
No longer blocks: sms-sprint-2.1S8
Target Milestone: 2.1 S8 (7Nov) → ---
Hi julien, it's the WIP patch about the textContent trim while input changed. But it still has other issues:

1) In English suggestion, the text would be trimmed even you select the word from suggestion list. But the problem is the suggestion cache will still keeps the words we typed, and when input manager need to replace the string with suggestion, it will replace with the length you typed. Since the typed word in cache does not match the word in textContent now, it will fetch the wrong range from the existing textContent and replace.

2) In current master it's pretty hard to type anything in subject with Chinese IME, so the check if the input event is in compose mode. The problem is when we remove the placeholder sibling, it will also trigger the input event with empty textContent, and this make user unable to show the first typed char in the subject.

I think both issue need input management support, so ni? Rudy about these issues as well.
Flags: needinfo?(rlu)
Attachment #8513312 - Flags: feedback?(felash)
As our discussion, 
  1) should be a keyboard app issue that it does not register an event handler for "surroundingtextchange" event, so it won't get notified when the content app changes the text content.

  2) I am not sure about this case, could you please give a simplified test case so that we could narrow  down to the possible root cause?

Thanks.
Flags: needinfo?(rlu)
Flags: needinfo?(echang)
Depends on: 1091550
Depends on: 1090883
Comment on attachment 8513312 [details] [diff] [review]
0001-Content-trim-WIP.patch

Review of attachment 8513312 [details] [diff] [review]:
-----------------------------------------------------------------

looks good to me :)

::: apps/sms/js/subject_composer.js
@@ +49,5 @@
>    function onInput(e) {
>      /* jshint validthis: true */
> +    var priv = privateMembers.get(this);
> +
> +    // For Chinese IME: If the word is still composing, we should skip the

as I understand, this is to compose a character, not a word? (I understand that in Chinese it could be the same, but not necessarily for other languages; for example "ê" could use composition too: "^" first and "ê" then; although it's not the case for me on Linux :) ).

@@ +82,5 @@
> +      range.selectNodeContents(target);
> +      range.collapse(false);
> +      selection = window.getSelection();
> +      selection.removeAllRanges();
> +      selection.addRange(range);

if I understand correctly, you do this to put the caret at the end.

I have 2 thoughts:
* shouldn't this be in focus() too?
* should we make this a separate function then? (not the "textContent =" line of course)
* see Compose.focus, we're doing it differently. I don't know which one is better, but I think we should stick to one way.
Attachment #8513312 - Flags: feedback?(felash) → feedback+
blocking-b2g: backlog → ---
Priority: -- → P1
Duplicate of this bug: 1173296
See duped bug 1173296, this is also happening with pasted text.
Whiteboard: [p=2] → [p=2][sms-most-wanted]
And I see we don't prevent the user from sending the message (the "send" button is still enabled).
Some offline discussion with current UX Bryant:
When the length is over the limitation, we'll prefer to keep the string and disable the send button instead of string strimming because we want to let user to adjust the string by themselves. Trimming the string would trigger bug 1090883 and user might not want to see the string is trimmed after suggestion/pasting. Son it'll need copywriter's input for the proper string.
(In reply to Steve Chung [:steveck] from comment #29)
> Some offline discussion with current UX Bryant:
> When the length is over the limitation, we'll prefer to keep the string and
> disable the send button instead of string strimming because we want to let
> user to adjust the string by themselves. Trimming the string would trigger
> bug 1090883 and user might not want to see the string is trimmed after
> suggestion/pasting. Son it'll need copywriter's input for the proper string.

Sounds like a good plan.

Note that some of the previous comments are incorrect -- app do receive events whenever there is an action on the input box -- besides keydown keyup keypress, there are compositionstart/update/end, paste. The 'input' event is the last resort of any value change anyway.

All you need to do is walk to my desk and ask me in person .... :)
Assignee: schung → nobody
Mass closing of Gaia::SMS bugs. End of an era :(
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.