XUL Widgets: *-box is too much, and can be removed...

NEW
Unassigned

Status

()

Core
XUL
15 years ago
8 years ago

People

(Reporter: Alfred Kayser, Unassigned)

Tracking

({helpwanted})

Trunk
helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

15 years ago
In many widgets an extra box container is used.
For example in button.xml, button-box is defined
as a container between button and its elements.
It can be easily removed, saving a lot of XUL overhead
in terms of memory, performance, and even diskspace.
Many styling rules can be simplified from:
button > .button-box > .button-icon to
button > .button-icon.

Other examples are: radio, checkbox, textbox, etc.

The only location where it is really used is in the
classic theme for the dotted focus ring in the buttons.
As only classic needs, why not move the whole *-box containers
out of the base, and only define an extra binding rule in the
classic theme itself (just like the modern theme).
Keywords: helpwanted
(Reporter)

Comment 1

15 years ago
Patch coming up. I've removed a number of 'internal-box' things,
which resulted in small changes in Classic (only for the Win/Unix
variant), and a very small change in Modern.
The impact on other themes should be small (said the author
of many mozilla themes, such as Wood, LittleMozilla, Nautipolis, etc).

It would be good if this bug is 'fixed' before or during the
period all themes are to be changed for the splitup after the 1.4 release.
(Reporter)

Comment 2

14 years ago
Created attachment 132577 [details] [diff] [review]
Complete patch, removing the redundant 'internal-box' stuff...

Removed:
* button-box
* checkbox-label-box
* button-internal-box
* menulist-editable-box (note: check the xbl:inherit="context")
* titlebar-box
* radio-label-box
* textbox-input-box
And some changes in 'classic' and 'modern' to accomodiate the changes...
Note, that in most themes, the change will allmost unvisible...
Certainly no visible impact for my themes ;-)

This fix is important as it does save a lot of memory footprint and increases
performance generally!
(Reporter)

Updated

14 years ago
Attachment #132577 - Flags: review?(blake)
(Reporter)

Updated

14 years ago
Attachment #132577 - Flags: review?(blake) → review?(varga)

Comment 3

13 years ago
Comment on attachment 132577 [details] [diff] [review]
Complete patch, removing the redundant 'internal-box' stuff...

This is just a visual review, I haven't tried it out as such.

>-      <xul:hbox class="box-inherit button-box" xbl:inherits="align,dir,pack,orient"
>-                align="center" pack="center" flex="1">
I don't see you compensating for the default align and pack anywhere.

>-      <xul:hbox class="menulist-editable-box textbox-input-box" xbl:inherits="context" flex="1">
We can't get rid of this, there's a very important XBL binding for it.

>-.button-box {
>+.button-box
>   padding: 1px 4px 2px 3px;
> }
Typo?

>-button[type="menu-button"] {
>+button[type="menu-button"] > .button-box {
>   -moz-box-align: center;
>   -moz-box-pack: center;
>   margin: 0;
Why does this need to be changed?

>+button {
>+  -moz-binding: url("chrome://global/skin/globalBindings.xml#button");
>+}
What about menubuttons and dual menubuttons?

>+checkbox {
>+  -moz-binding: url("chrome://global/skin/globalBindings.xml#checkbox");
>+}
Nit: Would be nice to have the existing radio binding after this new checkbox
one.

>+  <binding id="checkbox" extends="chrome://global/content/bindings/checkbox.xml#checkbox">
Would be nice to have the checkbox-with-spacing binding here too, then followed
by the existing radio bindings.

>-  -moz-border-radius: 4px;
>+  -moz-border-radius: 5px;
Any reason for this change? (Also for the tabs)

>-checkbox[disabled="true"] > .checkbox-label-box > .checkbox-label {
>+checkbox[disabled="true"] > .checkbox-label {
>   color: #8C99AB !important;
> }
[Here I was thinking that colour inherited ;-) ]
Attachment #132577 - Flags: review?(varga) → review-
(Reporter)

Comment 4

13 years ago
Thanks for the review. 
The patch is allready a bit old.
I will try to checkout your comments and provide with a cleaner patch (hopefully
soon)

The textbox is not that important, but button, radio and checkbox are the most
critical ones. So, the next version will not touch the scary textbox stuff.

I thinki the radius change from 3 to 5 was intended, making it look slightly
better with the removed 'internal-box' stuff. I will check this out however.

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets

Updated

8 years ago
Assignee: hyatt → nobody
You need to log in before you can comment on or make changes to this bug.