106656 - tabs' use of child nodes is inconsistent

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

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

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[[e-mail]]

Attachment: Avoid reselecting current tab

Comment 3

[John Morrison]

Comment on attachment 55045 [details] [diff] [review]
Proposed patch

I take it that you meant to obsolete this first version, no?

Comment 4

[[e-mail]]

Attachment: Fixed typo

Comment 5

[[e-mail]]

Comment on attachment 55066 [details] [diff] [review]
Avoid reselecting current tab

I'm not used to the new attachment system yet :-)

Comment 6

[Stuart Ballard]

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?

Comment 7

[[e-mail]]

Attachment: How "deselect" might work

Obviously this would depend on the existence of the "deselect" event.

Comment 8

[David Hyatt]

--> default owner

Comment 9

[Stuart Ballard]

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?

Comment 10

[[e-mail]]

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.

Comment 11

[[e-mail]]

Attachment: Optimized use of this.childNodes

Comment 12

[[e-mail]]

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.

Comment 13

[Stuart Ballard]

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 14

[jag (Peter Annema)]

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?

Comment 15

[[e-mail]]

Actually, the selectedIndex setter is unusable and always returns null;
nobody uses last-tab="true" so I don't forsee any problems there either.

Comment 16

[[e-mail]]

Attachment: Fix errors in last patch

Sorry for not spotting the problem sooner :-(

Comment 17

[James Turner]

<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

Comment 18

[[e-mail]]

James, well the good news is that the setter now works :-P

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 102388 [details] [diff] [review]
Fix errors in last patch

sr=bzbarsky, under the usual "I assume you tested" thing.  ;)

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 102388 [details] [diff] [review]
Fix errors in last patch

a=asa for checkin to 1.2beta (on behalf of drivers)

Comment 21

[[e-mail]]

Fix was checked in by Jan Varga.