765805 - [tablet] Don't close the tabs sidebar panel when switching or adding tabs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1. On a tablet, open the tab sidebar.
2. Add a tab or switch tabs.

Expected results: Tabs panel remains open.
Actual results: Tabs panel closes.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

This doesn't yet handle thumbnail changes for tabs that load in the background while the tab panel is open.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: 1/3: Don't hide the tab sidebar when switching tabs

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: 2/3: Don't hide the tab sidebar when adding a new tab

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: 3/3: Update thumbnails for tabs added while the sidebar is open

Without this patch, the thumbnail will remain blank after you add a new tab while the sidebar is open.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 634195 [details] [diff] [review]
2/3: Don't hide the tab sidebar when adding a new tab

Review of attachment 634195 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. There is a bug on "add tab not shown on tabs-ui". Please mark it as a duplicate of this.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 634197 [details] [diff] [review]
3/3: Update thumbnails for tabs added while the sidebar is open

Review of attachment 634197 [details] [diff] [review]:
-----------------------------------------------------------------

https://bugzilla.mozilla.org/attachment.cgi?id=634197&action=diff#a/mobile/android/base/GeckoApp.java_sec1

The shouldUpdateThumbnail() takes a Tab as a parameter. However your calls doesn't seem to pass this. Please add them, check the compilation.

This patch looks good to me with those changes.

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: 3/3: Update thumbnails for tabs added while the sidebar is open (v2)

I forgot to refresh the patch after fixing some syntax errors.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: patch

...and somehow managed to introduce another syntax error.  Whoops!

Comment 9

[Matt Brubeck (:mbrubeck)]

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

...and this doesn't seem to be working correctly.  I'll take a closer look.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: 3/3: Update thumbnails for tabs added while the sidebar is open

Carrying r=sriram from comment 6.  This fixes the syntax errors.  The other bug I was seeing turned out to be unrelated to this patch.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: 1/3: Don't hide the tab sidebar when switching tabs (v2)

This moves the autoHideTabs implementation to BrowserApp (as requested by Sriram on IRC), and adds a return value so callers can see whether anything happened.

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: 1/3: Don't hide the tab sidebar when switching tabs (v3)

Remove a line that snuck in by mistake.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 634567 [details] [diff] [review]
1/3: Don't hide the tab sidebar when switching tabs (v3)

Review of attachment 634567 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a56337bb8f95
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d5c0ed0d26
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd7d21b84ed

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a56337bb8f95
https://hg.mozilla.org/mozilla-central/rev/b7d5c0ed0d26
https://hg.mozilla.org/mozilla-central/rev/ecd7d21b84ed

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: roll-up patch for Aurora approval

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 739407

User impact if declined: Tab sidebar doesn't stay open as interact with the browser (UX regression from XUL Fennec).

Testing completed (on m-c, etc.): Landed on m-c June 20.

Risk to taking this patch (and alternatives if risky): This is mobile only and fairly low risk, mostly just adding some checks before automatically closing the tab bar.

String or UUID changes made by this patch: None.

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 635096 [details] [diff] [review]
roll-up patch for Aurora approval

[Triage Comment]
Approved for Aurora 15 in support of the new tablet UI feature.

Comment 18

[Matt Brubeck (:mbrubeck)]

These don't apply cleanly to Aurora 15.  Part 2 depends on bug 753102 because it uses TabEvents.ADDED, and part 3 depends on bug 755070 which moved the relevant code around and added the Tab.getThumbnailBitmap method.  It'll take a bit of work to rebase these patches, unless those bugs are landing on Aurora too.

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99b541bcb23a
https://hg.mozilla.org/releases/mozilla-aurora/rev/ccce6e6e322d
https://hg.mozilla.org/releases/mozilla-aurora/rev/c557dd6b817c

I rebased part 3 to apply without bug 755070.  It should be trivial to undo those changes later if bug 755070 ends up landing on the Fx15 branch.  I added some comments to make it clear when resolving the conflicts that would come up.

Comment 20

[Paul Feher]

Verified/fixed on:
Nightly Fennec 16.0a1 (2012-07-11)
Aurora Fennec 15.0a2 (2012-07-11)
Using:
Samsung Galaxy Tab (3.1)