Closed Bug 690816 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

Firefox 9
defect
Not set
normal

Tracking

(firefox10 fixed)

VERIFIED FIXED
Firefox 10
Tracking Status
firefox10 --- fixed

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Keywords: ux-control, Whiteboard: [pushed])

Attachments

(2 files, 5 obsolete files)

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.
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)
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)
Status: NEW → ASSIGNED
Whiteboard: [has patch]
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
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)
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)
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.
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+
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 :)
Flags: in-litmus?(fennec)
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)
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+
https://hg.mozilla.org/mozilla-central/rev/f6ca22334209
https://hg.mozilla.org/mozilla-central/rev/290504bc9669
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111004 Firefox/10.0a1 Fennec/10.0a1
Status: RESOLVED → VERIFIED
Testcase added: https://litmus.mozilla.org/show_test.cgi?id=33521
Flags: in-litmus?(fennec) → in-litmus+
Blocks: 775070
You need to log in before you can comment on or make changes to this bug.