If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Undo close tab thumbnail dissapears when rotating screen orientation

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: tchung, Assigned: vingtetun)

Tracking

Bug Flags:
in-litmus +

Details

(Whiteboard: [fennec-checkin-postb2][has-patch])

Attachments

(4 attachments, 5 obsolete attachments)

3.75 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
12.21 KB, patch
smaug
: review+
roc
: review+
Details | Diff | Splinter Review
3.76 KB, patch
Details | Diff | Splinter Review
12.80 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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
tracking-fennec: --- → ?
Flags: in-litmus?
Assignee: nobody → mark.finkle
tracking-fennec: ? → 2.0b3+
Created attachment 487614 [details] [diff] [review]
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.
Attachment #487614 - Flags: review?(mark.finkle)
Created attachment 487618 [details] [diff] [review]
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.
Attachment #487618 - Flags: feedback?(roc)
Attachment #487618 - Flags: feedback?(Olli.Pettay)
Created attachment 487627 [details] [diff] [review]
Patch v0.2 (Front-End)

The new patch add a little check to ensure we're locking the toolbar if needed
Attachment #487614 - Attachment is obsolete: true
Attachment #487627 - Flags: review?(mark.finkle)
Attachment #487614 - Flags: review?(mark.finkle)

Comment 4

7 years ago
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?
Attachment #487618 - Flags: feedback?(Olli.Pettay) → feedback-
(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
Created attachment 487795 [details] [diff] [review]
Patch v0.2 (m-c)

beforeresize -> MozBeforeResize
Attachment #487618 - Attachment is obsolete: true
Attachment #487795 - Flags: feedback?(roc)
Attachment #487618 - Flags: feedback?(roc)
Created attachment 487796 [details] [diff] [review]
Patch v0.3 (Front-end)

beforeresize->MozBeforeResize
Attachment #487627 - Attachment is obsolete: true
Attachment #487796 - Flags: review?(mark.finkle)
Attachment #487627 - Flags: review?(mark.finkle)
Comment on attachment 487795 [details] [diff] [review]
Patch v0.2 (m-c)

Seems OK if we really have to have it
Attachment #487795 - Flags: feedback?(roc) → feedback+
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
Attachment #487796 - Flags: review?(mark.finkle) → review+
Whiteboard: [fennec-checkin-postb2][has-patch]
Created attachment 488473 [details] [diff] [review]
Patch v0.3 (m-c)

The patch add MozBeforeResize as an event and add some tests for this bug using the MozBeforeResize event
Attachment #487795 - Attachment is obsolete: true
Attachment #488473 - Flags: review?(roc)
Attachment #488473 - Flags: review?(Olli.Pettay)
Attachment #488473 - Flags: review?(roc) → review+
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.
Attachment #488473 - Flags: review?(Olli.Pettay) → review+
This bug is all Vivien - reassigning
Assignee: mark.finkle → 21
Created attachment 489535 [details] [diff] [review]
Patch (m-c) for checkin
Created attachment 489536 [details] [diff] [review]
Patch (Front-End) for checkin
(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.
Created attachment 491200 [details] [diff] [review]
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
Attachment #489535 - Attachment is obsolete: true
pushed m-c patch:
http://hg.mozilla.org/mozilla-central/rev/2cd50afa5a00
pushed front-end part:
http://hg.mozilla.org/mobile-browser/rev/5da924908006
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 626529

Updated

7 years ago
No longer depends on: 626529

Comment 19

7 years ago
https://litmus.mozilla.org/show_test.cgi?id=15189
Flags: in-litmus? → in-litmus+

Comment 20

7 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.