Closed
Bug 370742
Opened 18 years ago
Closed 18 years ago
<tabbox> cleanup
Categories
(Toolkit :: XUL Widgets, defect)
Toolkit
XUL Widgets
Tracking
()
RESOLVED
FIXED
People
(Reporter: enndeakin, Assigned: enndeakin)
References
Details
Attachments
(3 files)
(deleted),
patch
|
asaf
:
first-review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
enndeakin
:
first-review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
asaf
:
first-review+
|
Details | Diff | Splinter Review |
Mainly, to clean up the tabbox/tabs/tab/tabpanels bindings to handle nsIDOMXULSelectControlElement methods properly.
Patch will be available soon.
Assignee | ||
Comment 1•18 years ago
|
||
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•18 years ago
|
||
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.
Attachment #255598 -
Flags: first-review?(mano) → first-review+
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 3•18 years ago
|
||
(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.
Assignee | ||
Updated•18 years ago
|
Flags: in-testsuite?
Comment 4•18 years ago
|
||
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
Attachment #255685 -
Flags: first-review?(enndeakin)
Assignee | ||
Comment 5•18 years ago
|
||
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?
Attachment #255685 -
Flags: first-review?(enndeakin) → first-review+
Comment 6•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
So, I'm assuming there was some really good reason to break backwards compat by removing the tab.selected setter, right?
Comment 9•18 years ago
|
||
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•18 years ago
|
||
That's bug 371055, which has been fixed AFAIK.
Comment 11•18 years ago
|
||
indeed, it is.
Assignee | ||
Comment 12•18 years ago
|
||
(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•18 years ago
|
||
The real irony (and annoyance) is that code was changed (at neil@parkwaycc.co.uk's request) to *specifically* set the selected property, before that interface even existed.
Comment 14•18 years ago
|
||
"Missed" review comment and sync'ing with XPFE patch.
Attachment #256375 -
Flags: first-review?(mano)
Updated•18 years ago
|
OS: Mac OS X → All
Hardware: PC → All
Version: unspecified → Trunk
Comment 15•18 years ago
|
||
Comment on attachment 256375 [details] [diff] [review]
(Cv1-TK) <tabbox.xml> (nits)
[Checkin: Comment 16]
r=mano.
Attachment #256375 -
Flags: first-review?(mano) → first-review+
Comment 16•18 years ago
|
||
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
}
Attachment #256375 -
Attachment description: (Cv1-TK) <tabbox.xml> (nits) → (Cv1-TK) <tabbox.xml> (nits)
[Checkin: Comment 16]
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•