Show SIP button and a toolbar on windows mobile

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: blassey, Assigned: alexp)

Tracking

({mobile})

Trunk
ARM
Windows Mobile 6 Professional
mobile

Details

Attachments

(2 attachments, 7 obsolete attachments)

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

Updated

8 years ago
Assignee: nobody → alexp
(Assignee)

Comment 1

8 years ago
Created attachment 420462 [details] [diff] [review]
[Work in Progress] Fix

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

Comment 2

8 years ago
Created attachment 420865 [details] [diff] [review]
[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.
Attachment #420462 - Attachment is obsolete: true
(Assignee)

Comment 3

8 years ago
Created attachment 421305 [details] [diff] [review]
Fix

Do not show toolbar and SIP button when "ui.sip.showSIPButton" preference is set to FALSE.
Attachment #420865 - Attachment is obsolete: true
(Assignee)

Comment 4

8 years ago
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.
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.
(Assignee)

Comment 6

8 years ago
Created attachment 421371 [details] [diff] [review]
[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.
(Assignee)

Comment 7

8 years ago
(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.
(Assignee)

Comment 8

8 years ago
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.
(Reporter)

Updated

8 years ago
Attachment #421371 - Attachment is patch: true
Attachment #421371 - Attachment mime type: application/octet-stream → text/plain
Created attachment 421394 [details] [diff] [review]
patch

just a couple tweaks to get the awesome panel up in ~2s
Attachment #421305 - Attachment is obsolete: true
(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.
(Assignee)

Comment 11

8 years ago
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?
Attachment #421394 - Flags: review?(bugmail)
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.
Attachment #421394 - Flags: review?(mozbugz)
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
(Assignee)

Comment 14

8 years ago
Created attachment 421479 [details] [diff] [review]
Proper patch

Fixed context lines in the last patch.
Attachment #421394 - Attachment is obsolete: true
Attachment #421479 - Flags: review?(bugmail)
Attachment #421394 - Flags: review?(mozbugz)
Attachment #421394 - Flags: review?(bugmail)
(Assignee)

Updated

8 years ago
Attachment #421479 - Flags: review?(mozbugz)
(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 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?
Attachment #421479 - Flags: review?(mozbugz) → review-
(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
(Assignee)

Comment 18

8 years ago
(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.
(Assignee)

Comment 19

8 years ago
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?
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?
(Assignee)

Comment 21

8 years ago
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.
(Assignee)

Comment 22

8 years ago
Created attachment 421767 [details] [diff] [review]
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.
Attachment #421479 - Attachment is obsolete: true
Attachment #421479 - Flags: review?(bugmail)
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.
Attachment #421767 - Flags: review-
(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
(Assignee)

Comment 25

8 years ago
Created attachment 421962 [details] [diff] [review]
Patch v7

This variation hides the SIP button window. The rest still seems to work as in the previous patch.
Attachment #421767 - Attachment is obsolete: true
Attachment #421962 - Flags: review?(bugmail)
(Assignee)

Updated

8 years ago
Attachment #421962 - Flags: review?(mozbugz)
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.
Attachment #421962 - Flags: review?(mozbugz) → review-
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).
(Assignee)

Comment 28

8 years ago
(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.
IMO, the behavior of the current patch is better than the previous patches.
(Assignee)

Comment 30

8 years ago
(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!
(Assignee)

Comment 31

8 years ago
Created attachment 422290 [details] [diff] [review]
Patch v8

Added reset for software keyboard settings, and a couple of tweaks.
Attachment #421962 - Attachment is obsolete: true
Attachment #422290 - Flags: review?(bugmail)
Attachment #421962 - Flags: review?(bugmail)
(Assignee)

Updated

8 years ago
Attachment #422290 - Flags: review?(mozbugz)
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.
Attachment #422290 - Flags: review?(bugmail) → review+

Updated

8 years ago
Attachment #422290 - Flags: review?(mozbugz) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Blocks: 516468
dougt pushed http://hg.mozilla.org/mozilla-central/rev/569db7addb37
Status: NEW → RESOLVED
tracking-fennec: --- → 1.0a4-wm+
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Reporter)

Updated

8 years ago
Attachment #422290 - Flags: approval1.9.2.1?
pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b6f2819ebb9b
status1.9.2: --- → .2-fixed
(Reporter)

Updated

8 years ago
Duplicate of this bug: 515160
Attachment #422290 - Flags: approval1.9.2.2?
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
Status: RESOLVED → VERIFIED

Updated

7 years ago
Component: Windows Mobile → General
QA Contact: mobile-windows → general
You need to log in before you can comment on or make changes to this bug.