370742 - <tabbox> cleanup

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Neil Deakin]

Mainly, to clean up the tabbox/tabs/tab/tabpanels bindings to handle nsIDOMXULSelectControlElement methods properly.

Patch will be available soon.

Comment 1

[Neil Deakin]

Attachment: cleanup

Changes:

- add tabs/tabpanels properties to tabbox
- don't throw exceptions when getting/setting selection
- tabs inherits from basecontrol
- allow selection using selected or value attributes
- add value property to tabs
- set tabbox.selectedIndex when changing tabs
- remove extraneous whitespace
- make some fields in tabs and tab 'private'
- tab should inherit from control-item
- support disabled tabs

Comment 2

[Asaf Romano (gone)]

Comment on attachment 255598 [details] [diff] [review]
cleanup

>Index: toolkit/content/widgets/tabbox.xml
>===================================================================
>@@ -11,17 +11,17 @@
>           xmlns:xbl="http://www.mozilla.org/xbl">
> 
>   <binding id="tab-base">
>     <resources>
>       <stylesheet src="chrome://global/skin/tabbox.css"/>
>     </resources>
>   </binding>
> 
>-  <binding id="tabbox" display="xul:box"
>+  <binding id="tabbox"
>            extends="chrome://global/content/bindings/tabbox.xml#tab-base">

hrm, why is display="xul:box" no longer necessary?

>-      <property name="_tabs">
>+      <property name="_tabs" readonly="true" onget="return this.tabs;"/>
>+      <property name="_tabpanels" readonly="true" onget="return this.tabpanels;"/>


Might as well just remove them or mark (as in, comment) them as deprecated.



>@@ -307,25 +348,24 @@
>       <property name="selectedItem">
...
>         </getter>
>-        
>+
>         <setter>
>         <![CDATA[
>           if (!val)
>-            throw Components.results.NS_ERROR_NULL_POINTER;
>+            return val;
>           if (!val.selected) {

I kinda prefer if (val && !val.selected).

>@@ -508,33 +551,34 @@
> 
>       <property name="selectedIndex">
>         <getter>
>         <![CDATA[
>           var indexStr = this.getAttribute("selectedIndex");
>           return indexStr ? parseInt(indexStr) : -1;
>         ]]>
>         </getter>
>-                        
>+
>         <setter>
>         <![CDATA[
>+          if (val < 0 || val >= this.childNodes.length)
>+            return;

return val;

>-      <property name="label">
>+      <property name="control" readonly="true">
>         <getter>
>-          return this.getAttribute("label");
>+          <![CDATA[
>+            var parent = this.parentNode;
>+            if (parent &&
>+                parent instanceof Components.interfaces.nsIDOMXULSelectControlElement)

nit: you don't need to null-check when using instanceof.
 

r=mano otherwise.

Comment 3

[Neil Deakin]

(In reply to comment #2)
> >-  <binding id="tabbox" display="xul:box"
> >+  <binding id="tabbox"
> >            extends="chrome://global/content/bindings/tabbox.xml#tab-base">
> 
> hrm, why is display="xul:box" no longer necessary?
> 

Box frames aren't constructed by tag, they are constructed by display type.

Comment 4

[[e-mail]]

Attachment: xpfe

We currently need our tabbrowser.xml to work with either tabbox.xml so I ported over some of the easier changes to keep them roughly in sync:
* Removed display="xul:box", changed inheritance
* Renamed _tabs, _tabpanels and _selected
* Stopped throwing exceptions

Comment 5

[Neil Deakin]

Comment on attachment 255685 [details] [diff] [review]
xpfe

> 
>-      <property name="label">
>-        <getter>
>-          return this.getAttribute("label");
>-        </getter>
>-        <setter>  
>-          this.setAttribute("label", val);
>-          return val;
>-        </setter>
>-      </property>
>-

You'll want to extend general.xml#control-item if you take the label out.

Did you mean the change 'extends' for tabs and tab?

>       <!-- XXX -->                
>-      <property name="selected">
>+      <property name="_selected">
>         <getter>
>           return this.getAttribute("selected") == "true" ? true : false;
>         </getter>

If you want to keep the _selected getter, you could just return this.selected;

You can remove the XXX comment though.

Is there something in the toolkit tabbox.xml that xpfe can't use that it can't just use the toolkit file?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

This checkin broke the location bar when combined with Tab Mix Plus
https://addons.mozilla.org/firefox/1122/

There are no errors in the error console, however, so I have no idea what's going wrong.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Turning on strict warnings in the error console, I get two warnings:
Error: setting a property that has only a getter
Source File: chrome://tabmixplus/content/minit/tablib.js
Line: 91

Error: getBrowser().isBlankTab is not a function
Source File: chrome://tabmixplus/content/tab/tab.js
Line: 923

Comment 8

[:Gijs (he/him)]

So, I'm assuming there was some really good reason to break backwards compat by removing the tab.selected setter, right?

Comment 9

[Dão Gottwald [:dao]]

This didn't only break tab mix plus, but also switching tabs via Ctrl+[1-9].

Error: setting a property that has only a getter
Source File: chrome://browser/content/browser.js
Line: 1179

Comment 10

[James Ross]

That's bug 371055, which has been fixed AFAIK.

Comment 11

[Dão Gottwald [:dao]]

indeed, it is.

Comment 12

[Neil Deakin]

(In reply to comment #8)
> So, I'm assuming there was some really good reason to break backwards compat by
> removing the tab.selected setter, right?
> 

Yes, because it is a bug that is was not readonly (see http://lxr.mozilla.org/mozilla/source/dom/public/idl/xul/nsIDOMXULSelectCntrlItemEl.idl), and a bug in any code that was relying on this undocumented behaviour.

Comment 13

[James Ross]

The real irony (and annoyance) is that code was changed (at [e-mail]'s request) to *specifically* set the selected property, before that interface even existed.

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-TK) <tabbox.xml> (nits) [Checkin: Comment 16]

"Missed" review comment and sync'ing with XPFE patch.

Comment 15

[Asaf Romano (gone)]

Comment on attachment 256375 [details] [diff] [review]
(Cv1-TK) <tabbox.xml> (nits)
[Checkin: Comment 16]

r=mano.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 256375 [details] [diff] [review]
(Cv1-TK) <tabbox.xml> (nits)
[Checkin: Comment 16]


Checkin: {
2007-02-25 12:29	bugzilla%standard8.demon.co.uk 	mozilla/toolkit/content/widgets/tabbox.xml 	1.42
}