414078 - collect bottom bars into vbox for better extensibility

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: collects bottom bars in vbox

The findbar and statusbar currently sit underneath the hbox containing the tabbrowser and sidebar but outside any containing box besides the top-level window.  This means that extensions and themes that want to apply changes to all the chrome below the content have to modify each bar individually and hope no other chrome gets added.

If those two bars sat inside a vbox, their appearance and behavior would be exactly the same, but it'd be easier for extensions and themes to make changes to them collectively and robustly.

For example, an extension (like Personas) or theme could add a single background image to the entire lower chrome, which currently isn't possible.  And a theme could modify the appearance of the border between the chrome and the content in a way that wouldn't break if Firefox or an extension later added another bar.

Here's a patch that encloses those two bars in a vbox.  The patch calls the vbox the "bottombox" for similarity to the "toolbox" at the top of the window, but perhaps this should instead be "bottomChrome", "statusBox", or the like.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Requesting wanted-firefox3 for this simple, low-risk extensibility win.

Comment 2

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

Comment on attachment 299337 [details] [diff] [review]
collects bottom bars in vbox

This patch no longer applies for some reason, a diff -w would have been quicker to review! :)

r=me for just adding the vbox around these elements. I wonder if a more specific name (like browser-bottombox?) would be less likely to conflict with the IDs of elements added by extensions? Odds are probably pretty low, I guess this should be fine.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: unrotted, and renames vbox to "browser-bottombox"

Here's a version of the patch that applies to the trunk and changes the ID "bottombox" to "browser-bottombox" as suggested by Gavin.  This is the version of the patch I'll check in if approved to land this change.

Requesting approval for this low risk change that improves the extensibility and themability of the chrome underneath the browser element (i.e. the statusbar and the find bar).

I'll attach a -w version of the patch shortly to make it easier to see what has changed.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: -w version of patch v2

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: fixes trivial conflict with recent change

This is the same as the previous patch but fixes a trivial conflict with a recent change in the same area.  This is the version of the patch I'm going to check in.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.432; previous revision: 1.431
done

Comment 7

[Martin Stubenschrott]

I *think* that patch broke the display of my extension, so one should at least note this change in the release notes for Extension writers.

I had:

        <statusbar id="status-bar" class="hl-StatusLine">
            <hbox insertbefore="statusbar-display" id="vimperator-statusline" flex="1" height="10" hidden="false" align="center">
                <textbox class="plain" id="vimperator-statusline-field-url" readonly="false" flex="1" crop="end"/>
                <label class="plain" id="vimperator-statusline-field-inputbuffer"    flex="0"/>
                <label class="plain" id="vimperator-statusline-field-progress"       flex="0"/>
                <label class="plain" id="vimperator-statusline-field-tabcount"       flex="0"/>
                <label class="plain" id="vimperator-statusline-field-bufferposition" flex="0"/>
            </hbox>
            <!-- just hide them since other elements expect them -->
            <statusbarpanel id="statusbar-display" hidden="true"/>
            <statusbarpanel id="statusbar-progresspanel" hidden="true"/>
        </statusbar>

in my *.xul file, and before 2008-02-08 my custom "statusbar"-like hbox appeared just before the original statusbar but on the same line. Now it appears as a separate line just after the original statusbar.

The strange thing is, in the DOM inspector i now get:

vbox: browser-bottombox
  ->findbar: FindToolbar
  ->statusbar: status-bar
[some nodeName=script]
statusbar: status-bar
  ->hbox: vimperator-statusbar

Actually the strange thing is, how can two elements have the same id instead that they are merged?

Maybe this has nothing to do with this commit, i just noticed that the change happened about that time.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #7)
> Actually the strange thing is, how can two elements have the same id instead
> that they are merged?

Good question, it's a puzzlement.  Is the "statusbar" element in your overlay directly inside the <overlay> element, or is it inside some other element?

Comment 9

[Martin Stubenschrott]

Well, its <window <overlay <statusbar... /> /> />

The whole file can be seen here (and yeah, it might not be a good example of an .xul since it's my first extension, but well, it always worked):

http://www.mozdev.org/source/browse/vimperator/src/content/vimperator.xul?rev=1.14;content-type=text%2Fplain

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> The whole file can be seen here (and yeah, it might not be a good example of 
> an .xul since it's my first extension, but well, it always worked)

It looks like you're layering elements from the XUL window being overlaid, i.e.:

  <overlay>
    <window id="main-window">
      ...
      <statusbar id="status-bar">
        ...
      </statusbar>
    </window>
  </overlay>

I think that is what is causing the problem.  You can put both the <window> and the <statusbar> in the overlay, but you should make them both children of the <overlay> element, i.e.:

  <overlay>
    <window id="main-window">
      ...
    </window>
    <statusbar id="status-bar">
      ...
    </statusbar>
  </overlay>

Then any changes in the position of the status bar relative to the window won't affect your extension.  The same goes for other nested elements from the file being overlaid.

Comment 11

[Martin Stubenschrott]

Oh that seems to fix it! Thank you so much!

(And sorry, for using this bug report for fixing my extension bugs, it was just that i didnt change anything and suddenly it stopped working)

Comment 12

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #11)
> Oh that seems to fix it! Thank you so much!
> 
> (And sorry, for using this bug report for fixing my extension bugs, it was just
> that i didnt change anything and suddenly it stopped working)

No problem, I'm glad to hear it's been fixed.  I'll post to mozilla.dev.extensions about this general issue in case others are having similar problems.