435077 - remove second <stack> and "background" div

Status: VERIFIED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

I have a patch that removes the second <stack> and "background" <div> in deckbrowser. It depends on the patch in bug 346189.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: removes stacks, uses a div to contain the panning canvas

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 322996 [details] [diff] [review]
removes stacks, uses a div to contain the panning canvas

>diff -r 40b4edc3d415 chrome/content/deckbrowser.xml
>--- a/chrome/content/deckbrowser.xml	Fri May 23 09:57:47 2008 -0700
>+++ b/chrome/content/deckbrowser.xml	Wed May 28 01:36:49 2008 -0400
>@@ -7,29 +7,16 @@
>     xmlns:html="http://www.w3.org/1999/xhtml"
>     xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> 
>-  <binding id="deckpage">

If you're going to remove this, you should adjust the code later on that creates a <deckpage>, and the associated style rules in browser.css (probably easier to just leave it for now, there's a bunch of cleanup to do).

>   <binding id="deckbrowser">

>+      <xul:deck anonid="renderspace" class="deckbrowser-renderspace" style="overflow:hidden;" flex="1">
>+        <html:div style="overflow:hidden;background-image:url('checkerboard.png');" anonid="canvas-background">
>+	  <html:canvas  anonid="canvas" moz-opaque="true"/>
>+	</html:div>
>         <xul:deck anonid="container" class="deckbrowser-container" flex="1">
>         </xul:deck>
>-      </xul:stack>
>+      </xul:deck>

Fix up the indentation/alignment here? It'd be nice to using self-closing elements (e.g. <xul:deck/>) where possible, and add spaces between CSS properties and values (e.g. "overflow: hidden; background-image: url('checkerboard.png');").

>@@ -96,6 +83,7 @@
> 
>             this._container.appendChild(b);
>             this._container.selectedPanel = b;
>+            this._renderspace.selectedPanel = this._container;
>             this._browsers = null;

>@@ -162,7 +150,7 @@
>             if (aBrowser.constructor.name == "Number")
>               aBrowser = this.browsers[aBrowser];
>             this._container.selectedPanel = aBrowser;
>-
>+            this._renderspace.selectedPanel = this._container;

Not sure why these are needed? Won't the call in endPan catch all cases?

>+            canvas.style.marginLeft = "0px";
>+            canvas.style.marginTop = "0px";
>+            canvas.style.marginRight = "0px";
>+            canvas.style.marginBottom = "0px";

Probably want to factor all of these into a _setPosition(aCanvas) so that it's easier to switch back/forth to .left/.top or whatever.

>             // Toggle browser visibility so the browser is visible
>             this._container.style.visibility = "visible";
>+            this._renderspace.selectedPanel = this._container;

You can remove the visibility toggling, right?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 322996 [details] [diff] [review]
removes stacks, uses a div to contain the panning canvas

>diff -r 40b4edc3d415 chrome/content/deckbrowser.xml

>+        <html:div style="overflow:hidden;background-image:url('checkerboard.png');" anonid="canvas-background">

This file should be in the skin/ dir, too, and you should probably style it with a class and associated rule in browser.css.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Bug 431843 mostly addressed this. The deckbrowser internals aren't set in stone yet and we'll surely be revisiting them again, but don't think there's anything that needs to be tracked at the moment.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

reworked the browser code for beta3.