Last Comment Bug 690816 - [tabletui] When Fennec starts, remember whether the landscape sidebar was open or closed
: [tabletui] When Fennec starts, remember whether the landscape sidebar was ope...
Status: VERIFIED FIXED
[pushed]
: ux-control
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Firefox 9
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Matt Brubeck (:mbrubeck)
:
:
Mentors:
Depends on:
Blocks: 655762 690740 686417 688836 775070
  Show dependency treegraph
 
Reported: 2011-09-30 10:18 PDT by Matt Brubeck (:mbrubeck)
Modified: 2012-08-28 10:42 PDT (History)
4 users (show)
andreea.pod: in‑litmus+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1/2: refactor TabsPopup show/hide methods (3.30 KB, patch)
2011-09-30 15:25 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
2/2: Store the sidebar state in a persistent pref (2.54 KB, patch)
2011-09-30 15:47 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
1/2: refactor TabsPopup show/hide methods (v2) (4.75 KB, text/plain)
2011-09-30 16:28 PDT, Matt Brubeck (:mbrubeck)
no flags Details
1/2: refactor TabsPopup show/hide methods (v3) (5.28 KB, patch)
2011-09-30 17:44 PDT, Matt Brubeck (:mbrubeck)
lucasr.at.mozilla: review+
Details | Diff | Splinter Review
2/2: Store the sidebar state in a persistent pref (v2) (2.65 KB, patch)
2011-09-30 17:46 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
1/2: refactor TabsPopup and tab sidebar methods (v4) (16.42 KB, patch)
2011-10-03 08:39 PDT, Matt Brubeck (:mbrubeck)
lucasr.at.mozilla: review+
Details | Diff | Splinter Review
2/2: Persist the sidebar state (4.08 KB, patch)
2011-10-03 08:44 PDT, Matt Brubeck (:mbrubeck)
lucasr.at.mozilla: review+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2011-09-30 10:18:28 PDT
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.
Comment 1 Matt Brubeck (:mbrubeck) 2011-09-30 15:25:49 PDT
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.
Comment 2 Matt Brubeck (:mbrubeck) 2011-09-30 15:47:29 PDT
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.
Comment 3 Matt Brubeck (:mbrubeck) 2011-09-30 15:51:30 PDT
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
Comment 4 Matt Brubeck (:mbrubeck) 2011-09-30 16:28:34 PDT
Created attachment 563887 [details]
1/2: refactor TabsPopup show/hide methods (v2)

Minor update to use the new methods from browser.js.
Comment 5 Matt Brubeck (:mbrubeck) 2011-09-30 17:44:48 PDT
Created attachment 563906 [details] [diff] [review]
1/2: refactor TabsPopup show/hide methods (v3)

No code changes, just rebased on top of bug 690973.
Comment 6 Matt Brubeck (:mbrubeck) 2011-09-30 17:46:48 PDT
Created attachment 563908 [details] [diff] [review]
2/2: Store the sidebar state in a persistent pref (v2)

Rebased.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-30 18:45:32 PDT
(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 8 Matt Brubeck (:mbrubeck) 2011-09-30 19:12:20 PDT
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.
Comment 9 Lucas Rocha (:lucasr) 2011-10-03 02:33:42 PDT
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.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-03 05:50:06 PDT
(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 :)
Comment 11 Matt Brubeck (:mbrubeck) 2011-10-03 08:39:46 PDT
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.
Comment 12 Matt Brubeck (:mbrubeck) 2011-10-03 08:44:51 PDT
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.
Comment 13 Lucas Rocha (:lucasr) 2011-10-03 12:34:22 PDT
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! :-)
Comment 14 Lucas Rocha (:lucasr) 2011-10-03 12:34:56 PDT
Comment on attachment 564202 [details] [diff] [review]
2/2: Persist the sidebar state

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

Cool.
Comment 17 Aaron Train [:aaronmt] 2011-10-04 06:09:22 PDT
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111004 Firefox/10.0a1 Fennec/10.0a1
Comment 18 Andreea Pod 2011-10-14 06:42:24 PDT
Testcase added: https://litmus.mozilla.org/show_test.cgi?id=33521

Note You need to log in before you can comment on or make changes to this bug.