583341 - "Lose" focus if MeeGoTouch VKB is closed by the user

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Steffen Imhof]

Attachment: Patch to move focus away on VKB close

As per the system's default behaviour whenever the user closes the VKB manually the focus is removed from the inputfield.

Because the focus has to be somewhere the attached patch moves it to the root window in this case.

Comment 1

[Steffen Imhof]

Attachment: Accompanying patch for Fennec

There is a small patch for Fennec needed to keep the awesomebar functional. In its original state the autocompleter resets itself after losing the focus. So you cannot click on any link in the popup.

Comment 2

[Jeremias Bosch (:jbos)]

Comment on attachment 461661 [details] [diff] [review]
Patch to move focus away on VKB close

Change review on behalf of doug

Comment 3

[Jeremias Bosch (:jbos)]

Comment on attachment 461664 [details] [diff] [review]
Accompanying patch for Fennec

Change review on behalf of doug

Comment 4

[Steffen Imhof]

Attachment: Patch changed to use new #define name

Updated patch to use new #define name as changed in 583039.

As far as I know, Mark is on vacation... who could review the Fennec patch?

Comment 5

[Benjamin Stover (:stechz)]

Comment on attachment 461664 [details] [diff] [review]
Accompanying patch for Fennec

I am not seeing any code that acts on this "_dontBlur" property?

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 462106 [details] [diff] [review]
Patch changed to use new #define name



Comment why this is only needed for Meego.

>+#ifdef MOZ_PLATFORM_MEEGO

Use maemo platform #define

>+    if ( ((QFocusEvent*)aEvent)->reason() == Qt::OtherFocusReason

Extra space after the first (

>+         && mWidget->isVKBOpen()) {


>+        nsCOMPtr<nsIFocusManager> fm = do_GetService("@mozilla.org/focus-manager;1");
>+        if (fm) {
>+            nsCOMPtr<nsIDOMWindow> domWindow;
>+            fm->GetActiveWindow( getter_AddRefs(domWindow) );
>+
>+            nsCOMPtr<nsIDOMElement> newFocus;
>+            fm->MoveFocus(domWindow, nsnull, nsIFocusManager::MOVEFOCUS_ROOT, 0,
>+              getter_AddRefs(newFocus));
>+        }

It isn't exactly clear why you want to focus the active window here.  Why not just the current widget, or the top level window.  You can do this without the focus manager.

Comment 7

[Steffen Imhof]

(In reply to comment #5)
> Comment on attachment 461664 [details] [diff] [review]
> Accompanying patch for Fennec
> 
> I am not seeing any code that acts on this "_dontBlur" property?

The code is in content/widgets/autocomplete.xml
The basic idea of setting this _dontBlur is that the AutoCompleteController is not destroyed when the focus is lost. Because that prevents the popup links from working.

I think that it is safe to do so, because the _dontBlur gets reset when re-focussing. But then again, I might miss something.

Comment 8

[Steffen Imhof]

(In reply to comment #6)
> >+        nsCOMPtr<nsIFocusManager> fm = do_GetService("@mozilla.org/focus-manager;1");
> >+        if (fm) {
> >+            nsCOMPtr<nsIDOMWindow> domWindow;
> >+            fm->GetActiveWindow( getter_AddRefs(domWindow) );
> >+
> >+            nsCOMPtr<nsIDOMElement> newFocus;
> >+            fm->MoveFocus(domWindow, nsnull, nsIFocusManager::MOVEFOCUS_ROOT, 0,
> >+              getter_AddRefs(newFocus));
> >+        }
> 
> It isn't exactly clear why you want to focus the active window here.  Why not
> just the current widget, or the top level window.  You can do this without the
> focus manager.

I am playing around with this code at the moment. The crucial point is, that we have to do something about the xulrunner internal focus. The system focus is moved away anyway.

Some more explanation:

Consider the following steps:
 - User clicks into Awesomebar
 - Awesomebar is focused, IME is triggered, VKB pops up
 - User closes VKB manually
 - Now the focus is removed from the QWidget, hence the OnFocusOutEvent

If we don't do anything now, you can see the cursor still blinking in the Awesomebar (xulrunner focus still in there) but you cannot type with the hardware keyboard. And if you click into the Awesomebar again, there is no focus change, IME is not triggered and the VKB does not pop up again.

So what we want to achieve is, to remove the xulrunner focus, too. And this is why I thought using the focus manager might be the right thing to do.

I have a slightly improved version, which uses nsIFocusManager::ClearFocus() for more clarity, but that is still using the focus manager.

Comment 9

[Steffen Imhof]

A completely different approach might be to create some new UI event like "Lose focus" or "IM closed by user" and handle that inside the xulrunner code in a more suitable place for the focus manager.

But as long as there are no other platforms with a similar behaviour, I'd prefer the change to be in the widget code.

Comment 10

[Steffen Imhof]

Attachment: Slightly improved version, still using focus manager

I updated the patch to use the ClearFocus() method of the focus manager. As indicated above I think we have to use the focus manager or a completely different approach. But I am open for any ideas...

Comment 11

[Jeremias Bosch (:jbos)]

Push on that.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

We talked a little about this problem face-2-face. Comment 8 has a good summary of the problem:

"(After the user manually closes the VKB) If we don't do anything now, you can see the cursor still blinking in the Awesomebar, focus still in there, but you cannot type with the hardware keyboard. And if you click into the Awesomebar again, there is no focus change, IME is not triggered and the VKB does not pop up again."

We need to devise a fix for the problem. Should the fix be in Mozilla? Probably, if we want our textboxes to work like native textboxes, with respect to the VKB and hardware keyboard.

When the user manually closes the VKB, the OS expects the text editing widget to no longer be focused. But something has to be focused. That's why the patches here set focus to the window.

Are there better things to focus? Maybe. Fennec currently uses this trick in the front-end code to get :active CSS pseudo styling to work.

I think this bug ties into bug 588313 as well.

Comment 13

[Jeremias Bosch (:jbos)]

This fix a cirtical misbehavior of the browser regarding the meego vkb. We can't have a meego browser without having this covered. 

The fix Mark is talking about is about adding a third focus state. This seems to be a longterm goal and one which needs to be sorted out and agreed on in many departments. Our approach here handles the issue for Meego only. Its a very simple fix and can easly be removed in case we have the real fix.

The fennec patch should be moved to a own bug, this patch cover also the case that the user can't open any awesomebar/bookmark item after minimizing the browser while staying within the awesomebar page.

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 462601 [details] [diff] [review]
Slightly improved version, still using focus manager

Should not it be under MEEGO define? I  don't see any reason to confuse default Qt build with some third-party IM plugin initialization

Comment 15

[Jeremias Bosch (:jbos)]

Attachment: Fixed Comments

Thanks Oleg, i fixed it.

Comment 16

[Oleg Romashin (:romaxa)]

Comment on attachment 468342 [details] [diff] [review]
Fixed Comments

I think with current meego implementation and current mozilla API it is the only way to handle VKB behavior correctly.

We need semi-focus  state API  from mozilla and related support from platform to be able to keep focus in widget without IME helpers and other features, and listening for HW keys.

I would like to have comment here about bug number with semi-focus implementation

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Semi-focus for textboxes is bug 588313

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 461664 [details] [diff] [review]
Accompanying patch for Fennec

Doesn't seem to cause problems, though we should watch for any side effects

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 461664 [details] [diff] [review]
Accompanying patch for Fennec

Is this patch worth landing without the platform patch? I mean, does this patch add value on it's own?

Comment 20

[Jeremias Bosch (:jbos)]

(In reply to comment #19)
> Comment on attachment 461664 [details] [diff] [review]
> Accompanying patch for Fennec
> 
> Is this patch worth landing without the platform patch? I mean, does this patch
> add value on it's own?

It fixes a second bug too.

the bug here was:
"After minimizing the browser while in awesomebar page and returning. The User cant load a webpage by clicking on a item in the history result list."

That was a regression after we fixed NS_ACTIVATE/NS_DEACTIVATE in qt port.
NS_DEACTIVATE is fired in case the user minimize the browser. This cause that the focusmanager remove the focus from the textfield.

NS_ACTIVATE is fired when the browser gains again the user focus (i.e. throu maximize). If there was a NS_DEACTIVATE before, the focusmanager will set the focus back to the last focused textfield.

Now: in case the focus gets set into the awesomebar the history result gets internally invalidated but the user still sees the result. Because of that the user can still see the items but he cant load any webpage. 

that little patch avoid the internal blurring of textfield, but does allow to remove the visual focus from the input field. Since it never gets blurred the history result does not get invalidated and the user can still click and load the items.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 461664 [details] [diff] [review]
Accompanying patch for Fennec

pushed:
http://hg.mozilla.org/mobile-browser/rev/da97693be9ba

Comment 22

[Oleg Romashin (:romaxa)]

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

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 468342 [details] [diff] [review]
Fixed Comments

>+#ifdef MOZ_PLATFORM_MAEMO > 5

Do you mean #if rather than #ifdef?

Comment 24

[Jeremias Bosch (:jbos)]

yes you are right. I will fix this as first tomorrow.