Closed Bug 106656 Opened 18 years ago Closed 17 years ago

tabs' use of child nodes is inconsistent

Categories

(Core :: XUL, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: jag+mozilla)

References

Details

(Whiteboard: px)

Attachments

(4 files, 2 obsolete files)

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
Attached patch Proposed patch (obsolete) — Splinter Review
Keywords: patch, review
Attached patch Avoid reselecting current tab (obsolete) — Splinter Review
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
Attached patch Fixed typoSplinter Review
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
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1
Blocks: 102905
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?
Obviously this would depend on the existence of the "deselect" event.
OS: Windows 95 → All
Hardware: PC → All
--> default owner
Assignee: hyatt → jaggernaut
Status: ASSIGNED → NEW
Target Milestone: mozilla1.0.1 → ---
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?
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.
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
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...
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+
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+
Sorry for not spotting the problem sooner :-(
<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
James, well the good news is that the setter now works :-P
Attachment #102388 - Flags: review+
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 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+
Fix was checked in by Jan Varga.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: px
You need to log in before you can comment on or make changes to this bug.