Closed Bug 604192 Opened 11 years ago Closed 11 years ago

IME opened into the chrome process should not be dismissed by the content process

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

(Whiteboard: [VKB])

Attachments

(1 file, 2 obsolete files)

Attached patch wip (obsolete) — Splinter Review
In a Fennec e10s enabled build, the virtual keyboard opened on the chrome side can be close by the content side, which is a wrong behavior.

We still want the VKB to be opened if it is not opened on the content side for now.
Summary: [VKB] IME opened into the chrome process should not be dismissed by the content process → IME opened into the chrome process should not be dismissed by the content process
Whiteboard: [VKB]
Component: DOM: Events → Event Handling
Attached patch Patch (obsolete) — Splinter Review
The patch prevent content process to dismiss/update any states of the virtual keyboard (IME) as long as the keyboard is already opened in chrome and the currently focused element is editable.
Attachment #482982 - Attachment is obsolete: true
Attachment #483160 - Flags: review?(jimnchen+bmo)
tracking-fennec: --- → 2.0b3+
Comment on attachment 483160 [details] [diff] [review]
Patch

Will this be temporary? Bug 583976 will probably fix this. It seems the problem is caused by content never losing focus (akin to Bug 599053)

> bool
> TabParent::RecvSetIMEEnabled(const PRUint32& aValue)
> {
>   nsCOMPtr<nsIWidget> widget = GetWidget();
>-  if (widget)
>+  if (widget && CanAlterIMEState())
>     widget->SetIMEEnabled(aValue);
>+
>   return true;
> }
>
> bool
> TabParent::RecvGetIMEOpenState(PRBool* aValue)
> {
>   nsCOMPtr<nsIWidget> widget = GetWidget();
>-  if (widget)
>+  if (widget && CanAlterIMEState())
>     widget->GetIMEOpenState(aValue);
>   return true;
> }

Extra line before return in RecvSetIMEEnabled

>+PRBool
>+TabParent::CanAlterIMEState()

I don't think the name is very clear. Maybe SuppressContentIME?

>+{
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget)
>+    return true;
>+
>+  // if the VKB is already opened, and there is a focused, editable content in
>+  // chrome, do not react to IME request
>+  PRUint32 currentEnabledState;
>+  nsresult rv = widget->GetIMEEnabled(&currentEnabledState);
>+  if (NS_FAILED(rv))
>+    return false; // This platform doesn't support controling the IME state.
>+
>+  // don't prevent request if the VKB is already closed
>+  if (currentEnabledState == nsIWidget::IME_STATUS_DISABLED)
>+    return true;

Why do you have to check if it's open or not? I think all content calls should be prevented if chrome has control.

>+
>+  nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID);

Use nsFocusManager::GetFocusManager()

>+  if (fm) {
>+    nsCOMPtr<nsIDOMElement> focusedElement;
>+    fm->GetFocusedElement(getter_AddRefs(focusedElement));
>+
>+    if (focusedElement) {
>+      nsCOMPtr<nsIContent> content = do_QueryInterface(focusedElement);

Use fm->GetFocusedContent()


Thanks!
(In reply to comment #2)
> Comment on attachment 483160 [details] [diff] [review]
> Patch
> 
> Will this be temporary? Bug 583976 will probably fix this. It seems the problem
> is caused by content never losing focus (akin to Bug 599053)

IMO yes it will be temporary but since 583976 is not a b2 blocker I would like to be sure the VKB is displayed when needed and not altered by content.

I would addressed your comments soon, thanks for your code review.
Attached patch Patch v0.2Splinter Review
Addressed comments.
Assignee: nobody → 21
Attachment #483160 - Attachment is obsolete: true
Attachment #483928 - Flags: review?(jimnchen+bmo)
Attachment #483160 - Flags: review?(jimnchen+bmo)
Comment on attachment 483928 [details] [diff] [review]
Patch v0.2

> 
>+PRBool
>+TabParent::AllowContentIME()
>+{
>+  nsFocusManager* fm = nsFocusManager::GetFocusManager();
>+  NS_ENSURE_TRUE(fm, PR_FALSE);
>+
>+  nsCOMPtr<nsIContent> focusedContent = fm->GetFocusedContent();
>+  if (focusedContent && focusedContent->IsEditable())
>+    return false;

PR_FALSE

>+
>+  return true;

and PR_TRUE. Looks good otherwise.
Attachment #483928 - Flags: review?(jimnchen+bmo) → review+
http://hg.mozilla.org/mozilla-central/rev/e7aeeb1a3728
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
bug 606033 opened.  Note: VKB doesn't stay open when trying to open a panel.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.