602962 - Undo close tab thumbnail dissapears when rotating screen orientation

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tony Chung [:tchung]]

Undo close tab thumbnail dissapears when rotating screen orientation.   Occurs on android and maemo 2010-10-08 nightlies

Repro:
1) install fennec nightly 2010-10-08 on maemo and android
2) open a new tab and load a page (portrait mode)
3) open tab sidebar, and close the tab
4) notice the undo tab store thumbnail appears at the bottom of the sidebar
5) Verify changing screen orientation to landscape, will remove the undo tab thumbnail (and vice versa)

Expected:
- switching screen orientation should not remove the undo tab thumbnail

Actual:
- undo tab thumbnail gone

Comment 1

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

Attachment: Patch v0.1 (Front-End)

The patch keep the scrollbox position and restore them to the previous value when they are resetted to 0,0 when the window is resize.
This way we're not calling Browser.hideSidebars and we don't remove the "last close tab" from the tabs sidebars.

Comment 2

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

Attachment: Patch v0.1 (m-c)

This patch add a onbeforeresize event to allow Fennec to save its scrollbox position before the UI is beeing resized (by a screen rotation for example).
I still need to have some tests for the patch but I would like to have feedback before to make sure I'm not doing something stupid.

Comment 3

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

Attachment: Patch v0.2 (Front-End)

The new patch add a little check to ensure we're locking the toolbar if needed

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 487618 [details] [diff] [review]
Patch v0.1 (m-c)

We must not add non-prefixed new events which content can access.

Also, it is not quite clear to my why you need this new event.
Fennec UI gets some event from OS, when the device is rotated, and based
on that it modifies the UI (== rotates it). Couldn't you add something to the
event listener which causes the UI rotation?

Comment 5

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

(In reply to comment #4)
> Comment on attachment 487618 [details] [diff] [review]
> Patch v0.1 (m-c)
> 
> We must not add non-prefixed new events which content can access.

I've changed beforeresize -> MozBeforeResize on the patch I will upload

> Also, it is not quite clear to my why you need this new event.
> Fennec UI gets some event from OS, when the device is rotated, and based
> on that it modifies the UI (== rotates it). Couldn't you add something to the
> event listener which causes the UI rotation?

I should have described more why we need it here.
Fennec used scrollboxes extensively, the main view is composed of 3 scrollboxes nested for allowing panning of the chrome UI and of the content.

When the UI is resized, we want to store the scrollboxes position as they were _before_ the resize occur to restore it _after_ the resize, otherwise if you go from portrait to landscape (so from a smaller with to a larger width) and you have the right sidebar displayed before the resize you will end up with a screen position at 0,0 and the left sidebar displayed while the right sidebar is out of view.

Also the title of the bug and my previous description used the word "rotation" but basically this happen on a resize even if the orientation is not changing, so I don't think we want to add something to the "rotation" event because we want this code to works on Desktop which is the main emulator used.

let me know if you want to know more or if something is not clear enough

Comment 6

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

Attachment: Patch v0.2 (m-c)

beforeresize -> MozBeforeResize

Comment 7

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

Attachment: Patch v0.3 (Front-end)

beforeresize->MozBeforeResize

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 487795 [details] [diff] [review]
Patch v0.2 (m-c)

Seems OK if we really have to have it

Comment 9

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

Comment on attachment 487796 [details] [diff] [review]
Patch v0.3 (Front-end)

Do we need Browser.shouldHideSidebars ? Can't we just put a boolean in Browser.controlsPosition?

r+, but try to remove Browser.shouldHideSidebars

Comment 10

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

Attachment: Patch v0.3 (m-c)

The patch add MozBeforeResize as an event and add some tests for this bug using the MozBeforeResize event

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 488473 [details] [diff] [review]
Patch v0.3 (m-c)


> void
>+PresShell::FireBeforeResizeEvent()
...
>+  nsEventStatus status = nsEventStatus_eIgnore;
No need for this.

>+
>+  nsPIDOMWindow *window = mDocument->GetWindow();
>+  if (window) {
>+    nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);
>+    nsEventDispatcher::Dispatch(window, mPresContext, &event, nsnull, &status);
No need for the last two parameters.

The patch looks good, but I still don't quite like the approach.
Though, perhaps there is some use case for this all even in web content.

Comment 12

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

This bug is all Vivien - reassigning

Comment 13

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

Attachment: Patch (m-c) for checkin

Comment 14

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

Attachment: Patch (Front-End) for checkin

Comment 15

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

(In reply to comment #13)
> Created attachment 489535 [details] [diff] [review]
> Patch (m-c) for checkin

I ran this through Try and got too many consistent oranges to push to trunk without investigating.

I checked Vivien's Try run and it was orangey too. But different orange.

Comment 16

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

Attachment: Patch (m-c) for checkin

http://hg.mozilla.org/try/rev/d62f84e14c6e

This one works well on the try server, there is one unstarred orange in mochitest-ipcplugins but I don't think this is related, I

Comment 17

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

pushed m-c patch:
http://hg.mozilla.org/mozilla-central/rev/2cd50afa5a00

Comment 18

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

pushed front-end part:
http://hg.mozilla.org/mobile-browser/rev/5da924908006

Comment 19

[Ioana Chiorean]

https://litmus.mozilla.org/show_test.cgi?id=15189

Comment 20

[Andreea Pod]

VERIFIED FIXED on:
Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110315
Firefox/4.0b13pre Fennec /4.0b6pre
Device: HTC Desire