614355 - don't use fullscreen landscape keyboards on android

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Attachment: patch

Comment 1

[Doug Turner (:dougt)]

lets figure out what we want to do for b4.  on some devices, with suggestions, there is little room.  Making the keyboard smaller might be troublesome.

Comment 3

[Madhava Enros [:madhava]]

hi guys - could you put some screengrabs of this in action in the bug?

Comment 4

[Michael Wu [:mwu]]

Attachment: Fullscreen keyboard problem 1

This is what happens when we disable fullscreen keyboards. (or the keyboard doesn't go fullscreen for whatever reason, like the openwnn keyboard here seems to do) The visible part of the page is tiny, and doesn't show the field we clicked on.

Comment 5

[Michael Wu [:mwu]]

Attachment: Fullscreen keyboard problem 2

If you pan the field into view and start typing, the suggestions bar comes up and eats the remaining space for the page.

Comment 6

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

seems like if focus is in content and fennec gets resized past a certain threshold we should scroll the url bar off the screen

Comment 7

[Michael Wu [:mwu]]

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

Until these frontend issues are resolved, I don't want to land this.

Two ideas for improving the frontend behavior:
1. Ensure the awesomebar is hidden when we zoom into a field. There appears to be some race preventing the awesomebar from hiding properly on my N1 in landscape. (but not portrait)
2. Shorten the form helper to just the two buttons on the right on text fields. (hopefully doesn't impact perf too severely)

Comment 8

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

I'm not interested in landing this either, unless we limit it to cases where a significant number of pixels are still visible, such as the case with tablets.

Comment 9

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

(In reply to comment #7)

> Two ideas for improving the frontend behavior:
> 1. Ensure the awesomebar is hidden when we zoom into a field. There appears to
> be some race preventing the awesomebar from hiding properly on my N1 in
> landscape. (but not portrait)

Hiding the URLBar only gives us a few extra pixels of height. The problem #2 screen shot shows that this won't give real benefit over using the fullscreen keyboard.

> 2. Shorten the form helper to just the two buttons on the right on text fields.
> (hopefully doesn't impact perf too severely)

This model does scale to all form widgets which means we would be breaking the pattern for textboxes and other widgets.

Comment 10

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

Attachment: patch

Comment 11

[Paul [pwd]]

Can we not use the fullscreen editor in landscape (default behaviour on my Incredible S) and simply provide awesome bar behaviour through a tooltip in the same manner that spelling corrections are suggested?

Comment 12

[Doug Turner (:dougt)]

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

>+++ b/embedding/android/GeckoSurfaceView.java
>@@ -383,7 +383,10 @@ class GeckoSurfaceView
>             outAttrs.imeOptions = EditorInfo.IME_ACTION_SEND;
>         else if (mIMEActionHint != null && mIMEActionHint.length() != 0)
>             outAttrs.actionLabel = mIMEActionHint;
>-            
>+
>+        if (!mIMELandscapeFS)
>+            outAttrs.imeOptions |= EditorInfo.IME_FLAG_NO_EXTRACT_UI;

if (mIMELandscapeFS == false)

>+//  0: don't show fullscreen keyboard
>+//  1: always show fullscreen keyboard
>+// -1: show fullscreen keyboard based on threshold pref
>+pref("widget.ime.android.landscape_fullscreen", 0);
>+pref("widget.ime.android.fullscreen_threshold", 300); // in hundreths of inches

It is somewhat odd that you are setting this pref to OFF, but settings a threshold.  It seems that if the pref was off the threshold should be not defined.  Just a nit.

>+    nsresult rv;
>+    nsCOMPtr<nsIPrefBranch> prefs = 
>+        do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);

no rv param needed on this call since you are testing for |prefs|.

also, maybe it is time we start caching this lookup.

>+                if (NS_SUCCEEDED(rv)) {
>+                    if (nsWindow::GetAndroidScreenBounds().height  * 100 < landscapeFS * Bridge()->GetDPI())

nit: comments would be helpful to understand your math.

Comment 14

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

(In reply to comment #12)
> if (mIMELandscapeFS == false)
> 
> >+//  0: don't show fullscreen keyboard
> >+//  1: always show fullscreen keyboard
> >+// -1: show fullscreen keyboard based on threshold pref
> >+pref("widget.ime.android.landscape_fullscreen", 0);
> >+pref("widget.ime.android.fullscreen_threshold", 300); // in hundreths of inches
> 
> It is somewhat odd that you are setting this pref to OFF, but settings a
> threshold.  It seems that if the pref was off the threshold should be not
> defined.  Just a nit.
there is at least a thought about the front end adding primary UI to flip the pref, so if its flipped to -1 we want a threshold there to be reasonable


> >+                if (NS_SUCCEEDED(rv)) {
> >+                    if (nsWindow::GetAndroidScreenBounds().height  * 100 < landscapeFS * Bridge()->GetDPI())
> 
> nit: comments would be helpful to understand your math.

divides are expensive, its a reflex

Comment 15

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

pushed http://hg.mozilla.org/mozilla-central/rev/2f79122c5cb7

Comment 16

[Anna (Waverley)]

VERIFIED FIXED on: 

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:2.2a1pre) Gecko/20110412 Firefox/4.2a1pre Fennec /4.1a1pre 

Device: HTC Desire Z (Android 2.2)

Comment 17

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

Also verified on:

Mozilla/5.0 (Android; Linux armv71; rv2.2a1pre) Gecko/20110413 Firefox/4.2a1pre Fennec/4.1a1pre
Device: Droid 2 
OS: Android 2.2

Mozilla/5.0 (Android; Linux armv71; rv2.2a1pre) Gecko/20110413 Firefox/4.2a1pre Fennec/4.1a1pre
Device: Thunderbolt
OS: Android 2.2

Comment 18

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

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=15366
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=15367

Comment 19

[Aakash Desai [:aakashd]]

Is there a way for this bug to be triaged to a query of bugs landed in Fennec 6 (and not Fennec 5)?

Comment 20

[Matt Brubeck (:mbrubeck)]

I'll set the target milestone field to mozilla5.  Here's a query of all mozilla5 and Firefox5 bugs:

http://bugzil.la/ALL+target%3Amozilla5%2CFirefox5

Comment 21

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

(In reply to comment #19)
> Is there a way for this bug to be triaged to a query of bugs landed in
> Fennec 6 (and not Fennec 5)?

this is in both, with bug 649051 changing the default for the pref