Closed Bug 245224 Opened 21 years ago Closed 18 years ago

Selected attribute of <tab> doesn't work

Categories

(Core :: XUL, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: jag+mozilla)

References

Details

Attachments

(3 files, 4 obsolete files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040316 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040316 If I set selected="true" attribute on the tab element then this tab isnt' selected Reproducible: Always Steps to Reproduce: Actual Results: Tab with selected="true" attribute isn't selected Expected Results: Tab with selected="true" attribute should be selected
Attached file test case (deleted) —
This doesn't work in builds going back to 1.6 or so, at least... According to xulplanet it should work. Neil, any idea whether this should be working?
Actually this seems vaguely familiar...
Whiteboard: DUPEME
Confirming with a Linux build from yesterday. I couldn't find a dupe.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Summary: selected attribute of tab doesn't work → Selected attribute of <tab> doesn't work
Whiteboard: DUPEME
Blocks: 249552
Blocks: 256024
Attached file proposed patch (obsolete) (deleted) —
I offer to extend interface of tabbox to improve possibilities of tabbox. Also this improvments help to fix this bug easy. 1) I suggest to fire select event for the tab element (not tabs as before) 1) I suggest to fire unselect event for the tab element 2) I suggest to do in order to selected property of tab selects tab.
Attached file proposed patch (obsolete) (deleted) —
I offer to extend interface of tabbox to improve possibilities of tabbox. Also this improvments help to fix this bug easy. 1) I suggest to fire select event for the tab element (not tabs as before) 1) I suggest to fire unselect event for the tab element 2) I suggest to do in order to selected property of tab selects tab.
to comment#6: in attachment need to replace name of new method with _selectByIndex
Attachment #168001 - Attachment is obsolete: true
selected property of tab should have only getter (third suggestion of comment #6 is wrong): <property name="selected" readonly="true" onget="return this.getAttribute('selected') == 'true' ? true : false;"/>
I disagree with all three suggestions of comment 6. Select events always fire on the container element as they do with html <select> elements. As for comment 8, the setter is a convenience for the <tabs> element, but there's no direct way to enforce privacy except by writing tabs to use setAttribute everywhere.
If select event will be dispatched to tab element then it will bubbled to tabs element. If target of select event must be container then select and unselect events may fire for container. In this case we can place handlers of select and unselect elements on container and we can put "afterselected" and "beforeselected" attributes in these handlers. It's needed to add method (_selectByIndex() in attachment) like setter of selectedIndex (without check of selected item) to fix this bug.
Now I remember why we permit setting .selected on a tab - tabbrowser needs to do that on a tab just before it removes it. To fix this bug it might be simple enough to do this: try { this.selectedTab.selected = true; } catch (e) { this.selectedIndex = 0; }
Sorry, I mean this.selectedItem of course.
Ok. This fix is nicer. About select events. <html:select> and <xul:menulist> elements doesn't fire select event. Why should select event be fired for the container? What do you think about unselect event? I seem it is needed for tab unitializing. Is there a way to know previous selected tab?
Sorry, maybe I was thinking about the change event. I can't remember why we fire select events, I'm guessing accessibility, in which case aaron will know why we don't fire them on menulists.
(In reply to comment #14) > Sorry, maybe I was thinking about the change event. > I can't remember why we fire select events, I'm guessing accessibility, in which > case aaron will know why we don't fire them on menulists. For menulists, accessibility uses an event called "DOMMenuItemActive". I can't remember why we don't just use "select" within a menu. It was a few years ago when David Hyatt asked me to call it DOMMenuItemActive. I can't think of any reason to call it that instead of "select", but we'd need to change the accessibility mapping code.
To comment #11. I think It's wrong way to fix this bug because tab will be selected but select event will not be fired.
Why do you think setting the selected tab wouldn't fire the select event?
Because this.selectedItem.selected=true don't fire select event. I mean tab selection on initialization when selected attribute is setted on the tab.
Sorry, I'd already forgotton what I'd written... How about this.selectedItem = this.selectedItem; ?
Yes. It will fix the bug.
Attached patch Patch to respect initially selected tab (obsolete) (deleted) — Splinter Review
Attachment #168134 - Flags: superreview?(jag)
Attachment #168134 - Flags: review?(mconnor)
Sorry. I was wrong. This patch doesnt' fix the problem because selectedItem doesn't select selected element.
Comment on attachment 168134 [details] [diff] [review] Patch to respect initially selected tab You're right, I'm not thinking straight. In fact, if the first tab has selected="true" even the old code doesn't work.
Attachment #168134 - Flags: superreview?(jag)
Attachment #168134 - Flags: review?(mconnor)
I can see only one way: add some method witch will select element in any way as I propose before.
Can't we either 1) move the part of tabs.selectedIndex setter after the !tabs[val].selected to a separate method and call that from tabs constructor (comment 10) -or- 2) do something like either a removeAttribute("selected") or selected=false on the tab we need to select before calling: var selIdx; try { selIdx = this.selectedIndex; this.tabs[selIdx].removeAttribute("selected"); /*or .selected=false but both are ugly*/ } catch(e) { selIdx = 0; //[*] } this.selectedIndex = selIdx; Also keep in mind that there's also bug 256024, and to fix it we also need to replace [*] with code that checks tabpanels' selectedIndex attribute.
I think the second way isn't nice unlike the first way. I dont consider unselecting and subsequent selecting of tab as the right way. It's better to modify tabs.selectedIndex setter.
Attached patch Alternate approach (obsolete) (deleted) — Splinter Review
How about this version? Note that it never fires an initial select event.
Attachment #168134 - Attachment is obsolete: true
I don't think it will select correct panel on startup. And I'm not sure if not firing select event is cool.
Ah yes, this method doesn't select the correct tabpanel.
Attached patch Fixes tabpanels (deleted) — Splinter Review
I'm still not convinced we need a select event.
Attachment #169710 - Attachment is obsolete: true
>I'm still not convinced we need a select event. I still think it would be more sane to fire initial select event (If nothing else, there was a reson for it to be fired before, I suppose). But it doesn't really matter much to me. The fact it sets the selected tab in tabs constructor and the selected panel in tabbox constructor is quite confusing and at least deserving a comment, imo. (btw pardon my ignorance, are you sure constructors are always called in correct order?)
Attached file patch variant (deleted) —
Why don't you want to do approximately the same like in attachment? It will select intial tab and send message to initial tab. Or do like in variant 2 of comment#25.
Attachment #168002 - Attachment is obsolete: true
I am confused such simple bug is cause serious discussion. Need to select initial tab and send message to it. Nothing more. It is no important by the highest standarts what way will be choosed.
OK, I can see some benefits of your approach, but I'm still not convinced about the initial event. For instance, an HTML <select size="1"> element doesn't send an event when it auto-selects the first option.
Reasons why initial select event should be fired: 1. There is initial select event in the current tab realization (for element with selectedIndex=0). 2. 2.1 I can't understand for what we need to segregate initial select from another select events. 2.2 When I add handler on select event then I don't see a big difference how event was rised (initial, by user or programatically). I want to do some actions when tab is selected. 2.3 I can't find any example in witch I would be forced to use special processing when I handle initial select event. More I guess it's not right to use something special to process initial selec event. (You're right it's not objective reason). 3. I think it is not exactly right to compare html:select and tabs elements. Thease elements rise different events. Although onchange event (html:select element) behaves like onselect event (tabs element) when selected item is changed by user, but onchange event is not fired when selected item is changed programatically unlike onselect event.
Ben Goodger recently checked in some changes to toolkit's tabbox which would seem to allow the preselecting of the selected index in certain circumstances.
Setting the selected attribute on a <tab> shouldn't do anything anyway, so this is invalid. I'm not following the select event discussion though, is that a different issue?
This is not about setting the attribute from a script, but about a document with <tab selected="true"/>. XUL Planet's description implies this should work ("This attribute is set to true if the tab is selected by default.") The select event discussion is about whether a select event should be fired on load when such tab is selected. Now that I think of it, seems to me neil was right when he suggested to _not_ fire the initial event.
(In reply to comment #38) > > The select event discussion is about whether a select event should be fired on > load when such tab is selected. Now that I think of it, seems to me neil was > right when he suggested to _not_ fire the initial event. > If you insist on that initial select shouldn't be fired then it's ok for me. Though it's vexed question for me as before.
(In reply to comment #38) > This is not about setting the attribute from a script, but about a document > with <tab selected="true"/>. Yes, that should work. Setting it any other time should not change the selection. Widgets should not fire the select event on initial selection.
Depends on: 370742
Fix by bug 370742
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: