532929 - Show SIP button and a toolbar on windows mobile

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

We have a preference (ui.sip.showSIPButton) to hide or show the SIP button on windows mobile; I think we should set it to true.  The software keyboard jumping around is jarring. Also, if you get stuck in a keyboard you don't want to use (we currently don't support multitap keyboards well), you end up having to leave Fennec, launch something else, click on a text box, change the keyboard and then return to fennec.   

If we do make this change, I think we should provide a "toolbar" behind the SIP button.  It doesn't have to be an OS tool bar (although it could).  But this space could be useful for contextual buttons.  Like ".com", or "bookmarks" in the url bar, or our form fill ui in content.

Comment 1

[Alex Pakhotin (:alexp)]

Attachment: [Work in Progress] Fix

Draft implementation.
It works, but needs polishing - sometimes the SIP button stays visible even though its window was hidden.

Comment 2

[Alex Pakhotin (:alexp)]

Attachment: [Work in Progress] Improved fix

This version works better, though there are still some cases when the toolbar does not show, for example after rotation of the screen to landscape and back to portrait. Have to add some special handling for this situation.

Comment 3

[Alex Pakhotin (:alexp)]

Attachment: Fix

Do not show toolbar and SIP button when "ui.sip.showSIPButton" preference is set to FALSE.

Comment 4

[Alex Pakhotin (:alexp)]

Brad, can you please check how this latest patch works on Omnia II?

The implementation seems to be logically correct, but it looks like there might be some weird timing(?) issues, when the toolbar and the whole keyboard are caused to hide sometimes.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

This ends up doing the right thing.  However it no takes more than 5s for the awesome panel to appear with this patch, as opposed to ~3s without it.

Comment 6

[Alex Pakhotin (:alexp)]

Attachment: [Work in Progress] Disable SoftKeyboardObserver (in the mobile tree)

Try-out change: disable SoftKeyboardObserver in browser.js. This observer doesn't seem to be needed now.

Comment 7

[Alex Pakhotin (:alexp)]

(In reply to comment #5)
> However it no takes more than 5s for the awesome panel to appear with this
> patch, as opposed to ~3s without it.

Does it happen every time or only on the first opening?
It is slow for me the first time, but all subsequent appearances of the awesome panel are ~3s on the HTC TouchPro.

Comment 8

[Alex Pakhotin (:alexp)]

Oh, forgot to mention - try to disable SoftKeyboardObserver - see the patch attached. Not sure if it will make any difference, but the observer may add something to that time.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

just a couple tweaks to get the awesome panel up in ~2s

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #7)
> Does it happen every time or only on the first opening?
> It is slow for me the first time, but all subsequent appearances of the awesome
> panel are ~3s on the HTC TouchPro.

It was every time.

Comment 11

[Alex Pakhotin (:alexp)]

Comment on attachment 421394 [details] [diff] [review]
patch

It works for me. Well, most of the time.
There still seems to be a chance for a toolbar to not show, but it's not clear yet why it may happen.

Brad, do you want to officially review it?

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 421394 [details] [diff] [review]
patch

I'd like dougt to have a look as well.  This sip code has a log of history and gotchas.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Alex, can you please post a patch with more context? 10+ lines would be ideal.

You might want to put this in your ~/.hgrc
diff = --git -U10 -p

Comment 14

[Alex Pakhotin (:alexp)]

Attachment: Proper patch

Fixed context lines in the last patch.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #13)
> You might want to put this in your ~/.hgrc
> diff = --git -U10 -p

in case anyone cares, the above line sets the default for hg diff, but not for hg export.  To set the default for both, apparently you should use:

[diff]
git = 1
showfunc = 1
unified = 10

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 421479 [details] [diff] [review]
Proper patch

why this change:

-        case SPI_SIPMOVE:


remove:

 PRBool          nsWindow::sSoftKeyMenuBar         = PR_FALSE;

this:

+  PRBool showSIPButton = show;
+  if (sShowSIPButton == TRI_FALSE)
+    showSIPButton = PR_FALSE

can be:

PRBool showSIPButton = (sShowSIPButton == TRI_FALSE);


Can we get rid of the SipSetInfo calls too?

do we need to delete dummyMenu?

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #16)
> (From update of attachment 421479 [details] [diff] [review])
> why this change:
> 
> -        case SPI_SIPMOVE:
> 

we get informed of any moves with a SPI_SETSIPINFO event, so listening for both we're getting two events for every move.

> Can we get rid of the SipSetInfo calls too?

I believe we can

Comment 18

[Alex Pakhotin (:alexp)]

(In reply to comment #16)
> PRBool showSIPButton = (sShowSIPButton == TRI_FALSE);

I guess you meant ...= (sShowSIPButton != TRI_FALSE);

I wouldn't do this because of two reasons:
1.Double-negative ("not false") is usually non-obvious for the readers of the code.
2.We also have to take into account the "show" argument of the ToggleSoftKB function.


> do we need to delete dummyMenu?

According to MSDN: "Resources associated with a menu that is assigned to a window are freed automatically. If the menu is not assigned to a window, an application must free system resources associated with the menu before closing."

So theoretically we don't need to destroy it.

Comment 19

[Alex Pakhotin (:alexp)]

I suggest for now to stick to my original patch:
https://bugzilla.mozilla.org/attachment.cgi?id=421305&action=edit
It adds the toolbar and shows the SIP button, but keeps other changes to soft-KB handling code minimal taking into account that "log of history and gotchas".
(Though I'll add nsWindow::sSoftKeyMenuBar change as per Doug's review as well.)

Then we'll move Brad's modification of this patch and other optimizations to the bug 516468, and do all experiments with NotifySoftKbObservers, SipSetInfo and SPI_* events handling there.

Any concerns?

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

I'm having a lot of trouble getting the soft kb to come up when placing focus in the search box at the top of google news. Does anyone else see this?

Comment 21

[Alex Pakhotin (:alexp)]

This might be caused by our hacky way to show and hide SIP using SIP windows directly. I'm testing an improved patch at the moment, which seems to work much better. Will attach it soon.

Comment 22

[Alex Pakhotin (:alexp)]

Attachment: Patch v6

This version uses SIP-related API (Sip* & SH*) to show and hide SIP button and software keyboard instead of dealing directly with windows. Seems to work better on HTC TouchPro.

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 421767 [details] [diff] [review]
Patch v6

>   if (aFullScreen) {
>     SetForegroundWindow(mWnd);
>-    SHFullScreen(mWnd, SHFS_HIDETASKBAR | SHFS_HIDESTARTICON);
>+    SHFullScreen(mWnd, SHFS_HIDETASKBAR | SHFS_HIDESTARTICON | SHFS_HIDESIPBUTTON);
>     SetRect(&rc, 0, 0, GetSystemMetrics(SM_CXSCREEN), GetSystemMetrics(SM_CYSCREEN));
I think we always want to hide the sip button when the keyboard isn't up.  

This could be another pref, but lets not worry about adding that now.


>   if (sShowSIPButton == TRI_UNKNOWN) {
>+    // Set it to a known value to avoid checking preferences every time
>+    // Note: ui.sip.showSIPButton preference change requires browser restart
you can add a call to nsIPrefBranch.addObserver() to get an event if and when this preference changes.  That will eliminate the need for a restart.


This seems to work better than the previous patch in terms of performance and showing the keyboard consistently (i.e. it fixes my previous comment).  However, the SIP button frequently doesn't hide when the keyboard and menubar hide.

I think you're on the right track here, just tame the SIP button and you got a winner.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #23)
> (From update of attachment 421767 [details] [diff] [review])
> >   if (aFullScreen) {
> >     SetForegroundWindow(mWnd);
> >-    SHFullScreen(mWnd, SHFS_HIDETASKBAR | SHFS_HIDESTARTICON);
> >+    SHFullScreen(mWnd, SHFS_HIDETASKBAR | SHFS_HIDESTARTICON | SHFS_HIDESIPBUTTON);
> >     SetRect(&rc, 0, 0, GetSystemMetrics(SM_CXSCREEN), GetSystemMetrics(SM_CYSCREEN));
> I think we always want to hide the sip button when the keyboard isn't up.  
nevermind, I read this backwards

> This could be another pref, but lets not worry about adding that now.
this still applies, however irrelevant it is

Comment 25

[Alex Pakhotin (:alexp)]

Attachment: Patch v7

This variation hides the SIP button window. The rest still seems to work as in the previous patch.

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 421962 [details] [diff] [review]
Patch v7

In widget/src/windows/nsWindow.h, do you need to declare sSoftKeyMenuBarHandle?

Drop comments like 

+        // (If there will be a pref to show SIP button even with a hidden keyboard, check it here)

Things change, and this may not be true when you go to add that pref.



You can kill those whitespaces too:

   sSIPInTransition = PR_TRUE;
-  HWND hWndSIP = FindWindowW(L"SipWndClass", NULL );
-  if (hWndSIP)
-    ShowWindow(hWndSIP, show ? SW_SHOW: SW_HIDE);
 
-  HWND hWndSIPB = FindWindowW(L"MS_SIPBUTTON", NULL ); 
-  if (hWndSIPB)
-      ShowWindow(hWndSIPB, show ? SW_SHOW: SW_HIDE);
-
-  SipShowIM(show ? SIPF_ON : SIPF_OFF);
+  SHSipPreference(wnd, show ? SIP_UP : SIP_DOWN);



Don't we have to remember the SIP button's position and reset it to the original?  I recall on some phones repositioning the keyboard was required.


+              if (SipGetCurrentIM(&clsid))
+              {
+                SipSetCurrentIM(&clsid);
+              }


drop the curly braces.

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

this seems to be working well in my testing. 

One thing to note is that we now start up with the sip button and menu bar showing, but not the keyboard (on the Omnia II).

Comment 28

[Alex Pakhotin (:alexp)]

(In reply to comment #27)
> One thing to note is that we now start up with the sip button and menu bar
> showing, but not the keyboard (on the Omnia II).

Start up is still screwed up. Seems to be related to the complex window resize performed at that time, which still needs to be investigated and fixed.

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

IMO, the behavior of the current patch is better than the previous patches.

Comment 30

[Alex Pakhotin (:alexp)]

(In reply to comment #26)
> (From update of attachment 421962 [details] [diff] [review])
> In widget/src/windows/nsWindow.h, do you need to declare sSoftKeyMenuBarHandle?

There is no such need for nsWindow.
nsWindowCE::sSoftKeyMenuBarHandle replaces local static variable sSoftKeyMenuBar previously defined in nsWindowCE::CreateSoftKeyMenuBar().

> Don't we have to remember the SIP button's position and reset it to the
> original?

A nice catch! We do need it!

Comment 31

[Alex Pakhotin (:alexp)]

Attachment: Patch v8

Added reset for software keyboard settings, and a couple of tweaks.

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 422290 [details] [diff] [review]
Patch v8

reviewed based on the interdiff
>+
>+  if (sDefaultSIPRect.top > 0) {
>+    SipSetDefaultRect(&sDefaultSIPRect);

Just a note, I believe it is possible to have a sip that has it's top at the top of the screen, either as a full screen keyboard (the omnia II has one) or something like a letter recognizer with a toolbar at the top.

Either way, we won't handle this situation very well to begin with (in ToggleSoftKB) if we're not showing the SIP button, so let's not worry about it right now.

Comment 33

[Brad Lassey [:blassey] (use needinfo?)]

dougt pushed http://hg.mozilla.org/mozilla-central/rev/569db7addb37

Comment 34

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b6f2819ebb9b

Comment 36

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2.2pre) Gecko/20100319 Namoroka/3.6.2pre Fennec/1.1a1


and

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.3a3pre) Gecko/20100315 Namoroka/3.7a3pre Fennec/1.1a1