619679 - [Maemo] Dismissing the virtual keyboard causes the top bar with "All pages", "Bookmarks" and "History" to disappear

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

Steps to reproduce:
- Keep Fennec in landscape mode with the hardware keyboard closed
- Tap on the url bar
- Tap another time on the url bar, the software keyboard should come up.
- Dismiss the software keyboard by clicking above it.

Expected result:
- The awesome bar should just look the same as it was before the software keyboard appeared and disappeared.

Actual result:
- The top bar, containing "All pages", "Bookmarks" and "History" disappeared.

Tested, using:
Mozilla/5.0 (Maemo; Linux armv7l; rv:2.0b8pre) Gecko/20101214 Firefox/4.0b8pre
Fennec/4.0b3

Comment 1

[Tony Chung [:tchung]]

flagging for blocking.  please add a screeshot

Comment 2

[Martijn Wargers (dead)]

Attachment: screenshot

Comment 3

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

Dismissing the VKB (as in step 4 in comment 0) seems to fire a focus or keydown event. This is handled by the awesomescreen and it loads the first item in the result into the edit box. This causes the button bar ("all pages", etc) to disappear - which is normal behavior.

Comment 4

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

Yes, I can reproduce this

Comment 5

[Aakash Desai [:aakashd]]

I'd love a browser chrome test for this.

Comment 6

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

A browser-chrome test for the native vkb is not easy

Comment 7

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

Attachment: Patch (m-c)

The problem came from the platform side, the platform assume that the popup is closed during the compositionstart event fired by the IME, and so during the compositionend event a fake DOM_VK_DOWN is dispatched.

But the popup is not really closed during the compositionstart, so this patch ensure the popup is _really_ closed before turning the flag on.

Comment 8

[Neil Deakin]

This patch seems to assume that ClosePopup will fail to close the popup. Why does it fail?

Comment 9

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

(In reply to comment #8)
> This patch seems to assume that ClosePopup will fail to close the popup. Why
> does it fail?

It fails because popupOpen is a readonly property in the case of fennec, http://hg.mozilla.org/mobile-browser/file/99268b046596/chrome/content/bindings.xml#l252, so event if ClosePopup() thought it is set to false this can be wrong (http://hg.mozilla.org/mozilla-central/diff/9b2a99adc05e/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp)

Comment 10

[Neil Deakin]

(In reply to comment #9)
> (In reply to comment #8)
> > This patch seems to assume that ClosePopup will fail to close the popup. Why
> > does it fail?
> 
> It fails because popupOpen is a readonly property in the case of fennec,
> http://hg.mozilla.org/mobile-browser/file/99268b046596/chrome/content/bindings.xml#l252

Since the popupOpen property isn't readonly in the interface isn't that the bug?

Comment 11

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

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > This patch seems to assume that ClosePopup will fail to close the popup. Why
> > > does it fail?
> > 
> > It fails because popupOpen is a readonly property in the case of fennec,
> > http://hg.mozilla.org/mobile-browser/file/99268b046596/chrome/content/bindings.xml#l252
> 
> Since the popupOpen property isn't readonly in the interface isn't that the
> bug?

Yes, it seems we should fix that.

Comment 12

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

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > This patch seems to assume that ClosePopup will fail to close the popup. Why
> > > does it fail?
> > 
> > It fails because popupOpen is a readonly property in the case of fennec,
> > http://hg.mozilla.org/mobile-browser/file/99268b046596/chrome/content/bindings.xml#l252
> 
> Since the popupOpen property isn't readonly in the interface isn't that the
> bug?

This is partly true, in Fennec this property is readonly for the moment but even if we decide to change it to be read-write this won't mean we can't intercept this call and prevent it to close the popup depending on some conditions.
If an extension extends our binding for example. I think we should not assume that turning this property to false _really_ close the popup.

Comment 13

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

Ah, OK now I understand the issue better. Fennec does want to control the popup itself.

Comment 14

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

Neil,
I think the patch is pretty low risk for Firefox since it used the autocomplete.xml binding with the "normal" popupOpen property, but by extending the XBL we really don't want to be closed unexpectingly which is why the property is read-only on the fennec side.

By the way I don't see the point of closing the autocomplete popup for Fennec when a compositionstart event is fired, this seems to be something specific to desktop application...

Comment 15

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

Thanks!

Comment 16

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

Attachment: tests mobile-browser

I've added some test for mobile-browser to ensure we won't regress on that.

Comment 17

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

Comment on attachment 507584 [details] [diff] [review]
tests mobile-browser

Remember to check that the test doesn't break on devices.

Comment 18

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

Comment on attachment 507078 [details] [diff] [review]
Patch (m-c)

Approved with tests

Comment 19

[Doug Turner (:dougt)]

mc = http://hg.mozilla.org/mozilla-central/rev/53a26f3050bc

Comment 20

[Doug Turner (:dougt)]

mb = http://hg.mozilla.org/mobile-browser/rev/b77fa308749e

Comment 21

[Anna (Waverley)]

I can still reproduce it on:

Build ID: Mozilla /5.0 (Maemo;Linux armv7l;rv:2.2a1pre) Gecko/20110406 Firefox/4.2a1pre Fennec /4.1a1pre 

Device: Nokia N900 (Maemo GTK)

opening the bug : see the screenshot

Comment 22

[Anna (Waverley)]

Attachment: The actual result

Comment 23

[Martijn Wargers (dead)]

Thanks for testing, I already had filed a new bug for this, bug 642771.

Comment 24

[Anna (Waverley)]

VERIFIED FIXED:

Build ID: Mozilla/5.0 (Maemo; Linux armv7l; rv:6.0a1) Gecko/20110510
Firefox/6.0a1 Fennec/6.0a1

Device: Nokia N900(Maemo GTK)