The default bug view has changed. See this FAQ.

[tabletui] When Fennec starts, remember whether the landscape sidebar was open or closed

VERIFIED FIXED in Firefox 10

Status

Fennec Graveyard
General
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

(Blocks: 2 bugs, {ux-control})

Firefox 9
Firefox 10
ux-control
Dependency tree / graph
Bug Flags:
in-litmus +

Details

(Whiteboard: [pushed])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
Steps to reproduce:
1. Open fennec in landscape tablet mode.
2. Hide the tab sidebar
3. Quit fennec
4. Open fennec in landscape tablet mode again

Expected results: Sidebar is still hidden.
Actual results: Sidebar is visible.

I think the sidebar state should be stored in a preference that is persistent across browsing sessions.  This lets people who don't want the sidebar to hide it once, instead of having to hide it every time the browser starts.
(Assignee)

Comment 1

6 years ago
Created attachment 563866 [details] [diff] [review]
1/2: refactor TabsPopup show/hide methods

No behavior change; this is just a minor rearrangement of the current code to be easier to understand and modify.
Attachment #563866 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 2

6 years ago
Created attachment 563873 [details] [diff] [review]
2/2: Store the sidebar state in a persistent pref

This adds a tiny bit of code that runs at startup, so there's a slight risk of performance impact.  I haven't tested this on device yet.
Attachment #563873 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Whiteboard: [has patch]
(Assignee)

Comment 3

6 years ago
Wes points out on IRC that the "persist" attribute has the same effect.  I'm not sure whether this would be better or worse than a pref:
https://developer.mozilla.org/en/XUL/Attribute/persist
(Assignee)

Comment 4

6 years ago
Created attachment 563887 [details]
1/2: refactor TabsPopup show/hide methods (v2)

Minor update to use the new methods from browser.js.
Attachment #563866 - Attachment is obsolete: true
Attachment #563866 - Flags: review?(lucasr.at.mozilla)
Attachment #563887 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 5

6 years ago
Created attachment 563906 [details] [diff] [review]
1/2: refactor TabsPopup show/hide methods (v3)

No code changes, just rebased on top of bug 690973.
Attachment #563887 - Attachment is obsolete: true
Attachment #563887 - Flags: review?(lucasr.at.mozilla)
Attachment #563906 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 6

6 years ago
Created attachment 563908 [details] [diff] [review]
2/2: Store the sidebar state in a persistent pref (v2)

Rebased.
Attachment #563873 - Attachment is obsolete: true
Attachment #563873 - Flags: review?(lucasr.at.mozilla)
Attachment #563908 - Flags: review?(lucasr.at.mozilla)
(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> Wes points out on IRC that the "persist" attribute has the same effect.  I'm
> not sure whether this would be better or worse than a pref:
> https://developer.mozilla.org/en/XUL/Attribute/persist

persist="value" would be a more XUL-ish to do it and since you are tracking an attribbute ("tablet_sidebar") it should work OK. Less code would be nice. persist might have the same "not saved when killed by OS" issues as the preference too.
(Assignee)

Comment 8

6 years ago
Comment on attachment 563908 [details] [diff] [review]
2/2: Store the sidebar state in a persistent pref (v2)

I'll try this the "persist" way before requesting review.
Attachment #563908 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 563906 [details] [diff] [review]
1/2: refactor TabsPopup show/hide methods (v3)

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

I wonder what are the cases where we actually need to specifically use showMenu/hideMenu or showSidebar/hideSidebar instead of the global hide/show methods. Why not simply use hide/show everywhere and hide the implementation details for portrait and landscape modes?

I'm giving a review+ here but I'd really like to have TabsPopup renamed to something less misleading before pushing this.

::: mobile/chrome/content/TabsPopup.js
@@ +141,5 @@
>        window.removeEventListener("resize", this.resizeHandler, false);
>      }
>    },
>  
> +  showSidebar: function showSidebar() {

I find it a bit awkward that something called TabsPopup has methods called showSidebar/hideSidebar. Maybe TabsPopup should have a different name like TabsUI or TabsList or something.
Attachment #563906 - Flags: review?(lucasr.at.mozilla) → review+

Updated

6 years ago
Blocks: 690740
(In reply to Lucas Rocha (:lucasr) from comment #9)

> I find it a bit awkward that something called TabsPopup has methods called
> showSidebar/hideSidebar. Maybe TabsPopup should have a different name like
> TabsUI or TabsList or something.

Don't forget TabsHelper! We love helpers in Fennec :)

Updated

6 years ago
Flags: in-litmus?(fennec)
(Assignee)

Comment 11

6 years ago
Created attachment 564198 [details] [diff] [review]
1/2: refactor TabsPopup and tab sidebar methods (v4)

This moves all of the tablet sidebar methods together into a new object, TabletSidebar.  TabsPopup once again deals only with the popup menu.

As a bonus, grabSidebar() and associated methods are now loaded lazily, so they won't even be parsed on non-tablet devices.
Attachment #563906 - Attachment is obsolete: true
Attachment #564198 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 12

6 years ago
Created attachment 564202 [details] [diff] [review]
2/2: Persist the sidebar state

Use the "persist" attribute to remember the state automatically.

Most of the changes here are necessary because "persist" won't work on removed attributes, so we need to set it to "false" instead.
Attachment #563908 - Attachment is obsolete: true
Attachment #564202 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 564198 [details] [diff] [review]
1/2: refactor TabsPopup and tab sidebar methods (v4)

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

This is much much nicer! :-)
Attachment #564198 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 564202 [details] [diff] [review]
2/2: Persist the sidebar state

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

Cool.
Attachment #564202 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 15

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ca22334209
https://hg.mozilla.org/integration/mozilla-inbound/rev/290504bc9669
Whiteboard: [has patch] → [pushed]
Target Milestone: --- → Firefox 10
(Assignee)

Comment 16

6 years ago
https://hg.mozilla.org/mozilla-central/rev/f6ca22334209
https://hg.mozilla.org/mozilla-central/rev/290504bc9669
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
status-firefox10: --- → fixed
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111004 Firefox/10.0a1 Fennec/10.0a1
Status: RESOLVED → VERIFIED

Comment 18

6 years ago
Testcase added: https://litmus.mozilla.org/show_test.cgi?id=33521
Flags: in-litmus?(fennec) → in-litmus+
(Assignee)

Updated

5 years ago
Blocks: 775070
You need to log in before you can comment on or make changes to this bug.