Closed
Bug 386202
Opened 18 years ago
Closed 17 years ago
Make <tab>'s attribute "selected" work correctly
Categories
(Toolkit :: XUL Widgets, defect, P2)
Toolkit
XUL Widgets
Tracking
()
RESOLVED
FIXED
People
(Reporter: KaiE, Assigned: enndeakin)
References
Details
(Keywords: regression)
Attachments
(2 files)
(deleted),
patch
|
Gavin
:
review+
mtschrep
:
approval1.9-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
In the past, when you used a statement like
<tab id="tab1">
<tab id="tab2" selected="true">
the selected attribute was ignored, tab1 was selected, and the contents of tab1 were displayed.
Bug 370742 introduced a kind of regression, originally reported as 373525 (which we worked around for now).
The regression and new behavior is:
- tab2 is shown as selected
- the contents of tab1 are displayed
Assignee | ||
Comment 1•17 years ago
|
||
Comment 2•17 years ago
|
||
Comment on attachment 276796 [details] [diff] [review]
check the existing selection differently
>Index: toolkit/content/widgets/tabbox.xml
This doesn't suppress the tabpanels "select" event from being fired in the case where you're setting the selectedIndex to it's current value, as you mentioned in 373525 comment 13, right? Is that OK?
>Index: toolkit/content/tests/widgets/test_tabbox.xul
>+function test_tabbox()
>+ // setting selectedPanel to null should not do anything
>+ tabbox.selectedPanel = null;
>+ test_tabbox_State(tabbox, "tabbox selectedPanel null", 0, tabs.firstChild, tabpanels.lastChild);
>+function test_tabpanels(tabpanels, tabbox)
>+ tabbox.selectedPanel = null;
>+ test_tabbox_State(tabbox, "tabpanels selectedPanel null", 0, tab, tabpanels.lastChild);
Looks like you forgot to s/tabbox/tabpanel/ on the first line here? Copying the comment over would be good, too.
Attachment #276796 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 3•17 years ago
|
||
(In reply to comment #2)
> (From update of attachment 276796 [details] [diff] [review])
> >Index: toolkit/content/widgets/tabbox.xml
>
> This doesn't suppress the tabpanels "select" event from being fired in the case
> where you're setting the selectedIndex to it's current value, as you mentioned
> in 373525 comment 13, right? Is that OK?
We discussed and tested this, and the tabpanels event is actually being supressed here by this patch.
> Looks like you forgot to s/tabbox/tabpanel/ on the first line here? Copying the
> comment over would be good, too.
>
Will fix this.
Assignee | ||
Updated•17 years ago
|
Attachment #276796 -
Flags: approval1.9?
Updated•17 years ago
|
Flags: in-testsuite?
Comment 4•17 years ago
|
||
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 276796 [details] [diff] [review] [details])
> > >Index: toolkit/content/widgets/tabbox.xml
> >
> > This doesn't suppress the tabpanels "select" event from being fired in the case
> > where you're setting the selectedIndex to it's current value, as you mentioned
> > in 373525 comment 13, right? Is that OK?
>
> We discussed and tested this, and the tabpanels event is actually being
> supressed here by this patch.
>
> > Looks like you forgot to s/tabbox/tabpanel/ on the first line here? Copying the
> > comment over would be good, too.
> >
>
> Will fix this.
>
Is there a new patch required to address?
Comment 5•17 years ago
|
||
Neil probably already made the changes locally, I suspect he'll land the patch when he gets back.
Flags: blocking1.9?
Comment 6•17 years ago
|
||
Comment on attachment 276796 [details] [diff] [review]
check the existing selection differently
minusing approval in anticipation of new patch. Please attach new patch and re-request (change looks reasonable just want to approve right patch)
Attachment #276796 -
Flags: approval1.9? → approval1.9-
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assignee | ||
Comment 7•17 years ago
|
||
Attachment #288348 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #288348 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 403724
Could this have caused bug 403724? The tree is currently closed so it would be
great if someone could look into it asap.
Ok, trying to back this one out too to attempt to fix bug 403724 :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 10•17 years ago
|
||
(In reply to comment #9)
> Ok, trying to back this one out too to attempt to fix bug 403724 :(
Relanded at 2007-11-14 19:43.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•