Closed Bug 446673 Opened 12 years ago Closed 12 years ago

sidebar never goes away.

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dougt, Assigned: mfinkle)

Details

Attachments

(1 file, 1 obsolete file)

once the side bar is slide out from the right, it never goes back into place.

build from yesterday trunk.
This patch adds code to hide the UI controls after certain actions, like:
* panning content
* starring a page
* editing a bookmark
* navigating to a new page (leaves urlbar but hides sidebar)

There are other things we could do too. This patch doesn't implement the interactions as designed either. It just tries to get the UI out of the way.

I had to do some merge wrok to add the multiple document support into the UI since we changed a good bit of the UI since multi-docs landed.
Assignee: nobody → mark.finkle
Status: NEW → ASSIGNED
Attachment #331252 - Flags: review?(gavin.sharp)
Comment on attachment 331252 [details] [diff] [review]
v1: add some triggered show/hide code for UI

Additional note: this patch adds some simple animation when showing and hiding the UI components.

Performance might be too poor to leave it in.
Comment on attachment 331252 [details] [diff] [review]
v1: add some triggered show/hide code for UI

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+  showToolbar : function() {
>+  showSidebar : function() {
>+  showTablist : function() {
>+  hide : function() {

Mixing calls to _showMode with these helpers is a bit confusing, almost seems like it would be better to just use showMode for all of them (the constant names are pretty self descriptive).

>+function AnimationHelper(options) {

>+    try {
>+      var value = this._options.effect(this._frame);
>+        for (var i=0; i<this._elements.length; i++) {

Indentation is weird here.

>+        if (this._elements[i].doStep)
>+          this._elements[i].doStep(value);
>+        else
>+          this._elements[i](value);
>+      }
>+		}

tabs (here and elsewhere in this code)! Is this stuff from jquery licensed compatibly?

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>       if (whereURI) {
>-        this.currentBrowser.loadURI(whereURI, null, null, false);
>+        var self = this;
>+        setTimeout(function() { self.currentBrowser.loadURI(whereURI, null, null, false); }, 0);

Any idea why this is needed?

I think the animation stuff is probably too slow to enable now, it was a bit poky on my desktop even.
(In reply to comment #3)
> >+  showToolbar : function() {
> >+  showSidebar : function() {
> >+  showTablist : function() {
> >+  hide : function() {
> 
> Mixing calls to _showMode with these helpers is a bit confusing, almost seems
> like it would be better to just use showMode for all of them (the constant
> names are pretty self descriptive).

removed separate helpers, changed "_showMode" to "show" and made the PANELMODE constants a bit more descriptive.

> 
> >+function AnimationHelper(options) {
> 
> >+    try {
> >+      var value = this._options.effect(this._frame);
> >+        for (var i=0; i<this._elements.length; i++) {
> 
> Indentation is weird here.
> 
> >+        if (this._elements[i].doStep)
> >+          this._elements[i].doStep(value);
> >+        else
> >+          this._elements[i](value);
> >+      }
> >+		}
> 
> tabs (here and elsewhere in this code)! Is this stuff from jquery licensed
> compatibly?

Not jQuery, but we decided to remove animation for now anyway. I removed the code.

> 
> >diff --git a/chrome/content/browser.js b/chrome/content/browser.js
> 
> >       if (whereURI) {
> >-        this.currentBrowser.loadURI(whereURI, null, null, false);
> >+        var self = this;
> >+        setTimeout(function() { self.currentBrowser.loadURI(whereURI, null, null, false); }, 0);
> 
> Any idea why this is needed?

Timing. If we don't wait for the XUL window onload sequence to finish first, when we get to the BrowserUI.show(PANELMODE_URLVIEW) spawned from the progress listeners, the XUL elements are not sized correctly yet. The sidebar, tablist and toolbar end up scrunched in the upper left corner of the window.

Using setTimeout to allow the onload to finish, allows the XUL elements to have their correct sizes when BrowserUI.show is called.
Attachment #331252 - Attachment is obsolete: true
Attachment #331320 - Flags: review?(gavin.sharp)
Attachment #331252 - Flags: review?(gavin.sharp)
Attachment #331320 - Flags: review?(gavin.sharp) → review+
changeset:   83:72d428415f1c
tag:         tip
user:        Mark Finkle <mfinkle@mozilla.com>
date:        Fri Jul 25 14:10:35 2008 -0400
summary:     b=446673, r=gavin. Sidebar never goes away
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
verified with beta3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.