Closed
Bug 106656
Opened 23 years ago
Closed 22 years ago
tabs' use of child nodes is inconsistent
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
People
(Reporter: neil, Assigned: jag+mozilla)
References
Details
(Whiteboard: px)
Attachments
(4 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
timeless
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
timeless
:
review+
bzbarsky
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
The problems as I see them:
1. <constructor> and selectedItem setter don't use childNodes like other methods
2. selectedIndex setter doesn't return val
3. selectedIndex written in terms of selectedItem is inefficient
4. onselect event gets fired before end of setter
Reporter | ||
Comment 1•23 years ago
|
||
Reporter | ||
Updated•23 years ago
|
Reporter | ||
Comment 2•23 years ago
|
||
Comment 3•23 years ago
|
||
Comment on attachment 55045 [details] [diff] [review]
Proposed patch
I take it that you meant to obsolete this first version, no?
Attachment #55045 -
Attachment is obsolete: true
Reporter | ||
Comment 4•23 years ago
|
||
Reporter | ||
Comment 5•23 years ago
|
||
Comment on attachment 55066 [details] [diff] [review]
Avoid reselecting current tab
I'm not used to the new attachment system yet :-)
Attachment #55066 -
Attachment is obsolete: true
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1
Comment 6•23 years ago
|
||
I'm probably going to be writing some code that consumes the "select" event from
the tabbrowser. As I've been thinking about what I need to do with this, I've
realized that I probably need two events: a "deselect" event which would happen
while the previously-selected tab is still current (so I can access and save the
state of that document), and then a "select" event which would happen after the
newly-selected tab becomes current. This would seem to parallel nicely with the
use of "load" and "unload" events.
Would that be practical? Would it be a bad idea?
Reporter | ||
Comment 7•23 years ago
|
||
Obviously this would depend on the existence of the "deselect" event.
Comment 8•23 years ago
|
||
--> default owner
Assignee: hyatt → jaggernaut
Status: ASSIGNED → NEW
Target Milestone: mozilla1.0.1 → ---
Comment 9•22 years ago
|
||
Is there any hope of this bug ever being fixed? There's talk of backing out the
linktoolbar because it doesn't work right with tabs (see bug 102905) and fixing
that properly absolutely requires functioning select events (deselect, as
mentioned above, would be nice, but isn't required for a first pass).
This is issue #4 mentioned in the original bug summary.
Does the existing patch work? Is it suitable for review? Has it rotted, and if
so how badly?
By the way, who is the maintainer of tabbrowser these days?
Reporter | ||
Comment 10•22 years ago
|
||
While these patches apply, I have since discovered a minor optimization:
var tabpanels = parent.getElementsByTagNameNS(
"http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
"tabpanels");
// This will cause an onselect event to fire for the tabpanel element.
if (tabpanels.length && tabpanels[0])
tabpanels[0].selectedIndex = val;
should be:
var tabpanels = parent._tabpanels;
// This will cause an onselect event to fire for the tabpanel element.
if (tabpanels)
tabpanels.selectedIndex = val;
And jag is currently the module/component owner of tabbrowser.
Reporter | ||
Comment 11•22 years ago
|
||
Reporter | ||
Comment 12•22 years ago
|
||
Ooh! I've just discovered a bug in the current tabbox.xml
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tabbox.xml#236
var tab = tabs.length >= val ? null : tabs[val];
This should have read
var tab = val >= tabs.length ? null : tabs[val];
Note that my patch completely rewrites the function so it is unaffected.
Severity: normal → major
Comment 13•22 years ago
|
||
Btw, I forget the answer to one of the questions I raised by email and I figure
it would be nice to have the answer in the bug anyway, for historical purposes.
When a tab is selected, two separate "select" events are raised, one on the
tabs, and one on the tabbox. This makes sense internally to the tabbrowser
itself, but from the perspective of an observer outside it is surprising and
confusing to receive two bubbled events for the same selection (not to mention a
potential performance penalty if you don't code carefully and exclude one of them).
Is there any reason not to, say, prevent bubbling one or other of the events out
of the tabbrowser?
In unrelated news, if someone is willing to do the actual testing part of a
review of this patch, I've already done the code-review part, so perhaps we can
specify a combined r=sballard/someone? Of course, a full review by someone else
wouldn't hurt either...
Assignee | ||
Comment 14•22 years ago
|
||
Comment on attachment 97924 [details] [diff] [review]
Optimized use of this.childNodes
sr=jag. Just make sure you've thoroughly tested this.
For example, selectedIndex's setter used to return a tab object instead of the
val. Good fix, but make sure none of the callers was expecting a tab.
Also, the old code would only set first-tab if there was just one tab, now that
tab will also get last-tab. Could this confuse our skins?
Attachment #97924 -
Flags: superreview+
Reporter | ||
Comment 15•22 years ago
|
||
Actually, the selectedIndex setter is unusable and always returns null;
nobody uses last-tab="true" so I don't forsee any problems there either.
Attachment #97924 -
Flags: review+
Reporter | ||
Comment 16•22 years ago
|
||
Sorry for not spotting the problem sooner :-(
Comment 17•22 years ago
|
||
<bitter> Cheers Neil, you just cost me 4 hours </bitter>
(don't worry, I'm not actually mad, for the first 3.5 hours I fully believed the
bug was in *our* code...)
But, uh, what testing did you do? (This'll teach me to try and work on the tip,
I suppose)
Oh and why on earth does selectedIndex throw NS_ERROR_FAILURE instead of
something useful, like a string? is there some requirement to only throw XPCOM
error codes so they can be passed back to C++ sensibly?
Anyway, you owe me a pint :-P
Reporter | ||
Comment 18•22 years ago
|
||
James, well the good news is that the setter now works :-P
Attachment #102388 -
Flags: review+
Comment 19•22 years ago
|
||
Comment on attachment 102388 [details] [diff] [review]
Fix errors in last patch
sr=bzbarsky, under the usual "I assume you tested" thing. ;)
Attachment #102388 -
Flags: superreview+
Comment 20•22 years ago
|
||
Comment on attachment 102388 [details] [diff] [review]
Fix errors in last patch
a=asa for checkin to 1.2beta (on behalf of drivers)
Attachment #102388 -
Flags: approval+
Reporter | ||
Comment 21•22 years ago
|
||
Fix was checked in by Jan Varga.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•22 years ago
|
Whiteboard: px
You need to log in
before you can comment on or make changes to this bug.
Description
•