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

VERIFIED FIXED in Firefox 15

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

(Depends on: 1 bug)

16 Branch
Firefox 16
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 verified, firefox16 verified)

Details

(Whiteboard: [tablet])

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 634090 [details] [diff] [review]
WIP

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

Comment 2

6 years ago
Created attachment 634192 [details] [diff] [review]
1/3: Don't hide the tab sidebar when switching tabs
Attachment #634090 - Attachment is obsolete: true
Attachment #634192 - Flags: review?(sriram)
(Assignee)

Comment 3

6 years ago
Created attachment 634195 [details] [diff] [review]
2/3: Don't hide the tab sidebar when adding a new tab
Attachment #634195 - Flags: review?(sriram)
(Assignee)

Comment 4

6 years ago
Created attachment 634197 [details] [diff] [review]
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.
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+
(Assignee)

Comment 7

6 years ago
Created attachment 634199 [details] [diff] [review]
3/3: Update thumbnails for tabs added while the sidebar is open (v2)

I forgot to refresh the patch after fixing some syntax errors.
Attachment #634197 - Attachment is obsolete: true
Attachment #634199 - Flags: review?(sriram)
(Assignee)

Comment 8

6 years ago
Created attachment 634202 [details] [diff] [review]
patch

...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)
(Assignee)

Comment 9

6 years ago
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)
(Assignee)

Comment 10

6 years ago
Created attachment 634217 [details] [diff] [review]
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.
Attachment #634202 - Attachment is obsolete: true
Attachment #634217 - Flags: review+
(Assignee)

Comment 11

6 years ago
Created attachment 634565 [details] [diff] [review]
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.
Attachment #634192 - Attachment is obsolete: true
Attachment #634192 - Flags: review?(sriram)
Attachment #634565 - Flags: review?(sriram)
(Assignee)

Comment 12

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

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+
(Assignee)

Updated

6 years ago
Blocks: 763726
https://hg.mozilla.org/mozilla-central/rev/a56337bb8f95
https://hg.mozilla.org/mozilla-central/rev/b7d5c0ed0d26
https://hg.mozilla.org/mozilla-central/rev/ecd7d21b84ed
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
(Assignee)

Comment 16

6 years ago
Created attachment 635096 [details] [diff] [review]
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.
Attachment #635096 - Flags: review+
Attachment #635096 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 18

6 years ago
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

Updated

6 years ago
Whiteboard: [tablet]
(Assignee)

Comment 19

6 years ago
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: ? → ---
status-firefox15: --- → fixed
(Assignee)

Updated

6 years ago
Depends on: 770270
(Assignee)

Updated

6 years ago
Depends on: 768254

Comment 20

6 years ago
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
status-firefox15: fixed → verified
status-firefox16: --- → verified
You need to log in before you can comment on or make changes to this bug.