Closed Bug 765805 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect)

16 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

(Depends on 1 open bug)

Details

(Whiteboard: [tablet])

Attachments

(4 files, 6 obsolete files)

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.
Attached patch WIP (obsolete) — Splinter Review
This doesn't yet handle thumbnail changes for tabs that load in the background while the tab panel is open.
Attachment #634090 - Attachment is obsolete: true
Attachment #634192 - Flags: review?(sriram)
Without this patch, the thumbnail will remain blank after you add a new tab while the sidebar is open.
Attachment #634197 - Flags: review?(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.
Attachment #634195 - Flags: review?(sriram) → review+
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.
Attachment #634197 - Flags: review?(sriram) → review+
I forgot to refresh the patch after fixing some syntax errors.
Attachment #634197 - Attachment is obsolete: true
Attachment #634199 - Flags: review?(sriram)
Attached patch patch (obsolete) — Splinter Review
...and somehow managed to introduce another syntax error.  Whoops!
Attachment #634199 - Attachment is obsolete: true
Attachment #634199 - Flags: review?(sriram)
Attachment #634202 - Flags: review?(sriram)
Comment on attachment 634202 [details] [diff] [review]
patch

...and this doesn't seem to be working correctly.  I'll take a closer look.
Attachment #634202 - Flags: review?(sriram)
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.
Attachment #634202 - Attachment is obsolete: true
Attachment #634217 - Flags: review+
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.
Attachment #634192 - Attachment is obsolete: true
Attachment #634192 - Flags: review?(sriram)
Attachment #634565 - Flags: review?(sriram)
Remove a line that snuck in by mistake.
Attachment #634565 - Attachment is obsolete: true
Attachment #634565 - Flags: review?(sriram)
Attachment #634567 - Flags: review?(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.
Attachment #634567 - Flags: review?(sriram) → review+
Blocks: 763726
[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.
Attachment #635096 - Flags: review+
Attachment #635096 - Flags: approval-mozilla-aurora?
Depends on: 766865
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.
Attachment #635096 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Depends on: 753102, 755070
Whiteboard: [tablet]
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.
tracking-fennec: ? → ---
Depends on: 770270
Depends on: 768254
Verified/fixed on:
Nightly Fennec 16.0a1 (2012-07-11)
Aurora Fennec 15.0a2 (2012-07-11)
Using:
Samsung Galaxy Tab (3.1)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.