386202 - Make <tab>'s attribute "selected" work correctly

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Kai Engert (:KaiE:)]

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

Comment 1

[Neil Deakin]

Attachment: check the existing selection differently

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 3

[Neil Deakin]

(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.

Comment 4

[Mike Schroepfer]

(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

[:Gavin Sharp [email: [e-mail]]]

Neil probably already made the changes locally, I suspect he'll land the patch when he gets back.

Comment 6

[Mike Schroepfer]

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)

Comment 7

[Neil Deakin]

Attachment: address comment

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could this have caused bug 403724? The tree is currently closed so it would be
great if someone could look into it asap.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, trying to back this one out too to attempt to fix bug 403724 :(

Comment 10

[Reed Loden [:reed]]

(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.