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

Status: RESOLVED FIXED

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip

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.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

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.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

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!

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Addressed comments.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mozilla-central/rev/e7aeeb1a3728

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

bug 606033 opened.  Note: VKB doesn't stay open when trying to open a panel.