405390 - Sized windows flicker at incorrect size when opening

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Start the browser
2)  Click the javascript: URI in the URL field

EXPECTED RESULTS:  Window comes up at the right size

ACTUAL RESULTS:  Window comes up at default browser size, then resizes to the right size.

This is a regression from bug 386202.  Specifically this line:

   tabpanels.selectedIndex = val;

that didn't use to run before does run now.  I have no idea how it's causing the problem, but commenting that one line out makes things work again.

Note that I can reproduce this only some of the time in Firefox, but reliably in SeaMonkey.

Comment 1

[Reed Loden [:reed]]

(In reply to comment #0)
> 2)  Click the javascript: URI in the URL field

What javascript: URI? :)

Comment 2

[Neil Deakin]

I'm not able to reproduce this on Linux or other platforms. Anyone else see this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Did you try reproducing with Seamonkey?

Comment 4

[Mike Schroepfer]

Bz - why did you nom this?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Because it looks terrible, leads to windows being placed at the wrong places, and is a regression?

Basically, it's bad enough that I've downgraded to a build that doesn't have this bug...

Comment 6

[[e-mail]]

(In reply to comment #0)
> This is a regression from bug 386202.  Specifically this line:
> 
>    tabpanels.selectedIndex = val;
> 
> that didn't use to run before does run now.  I have no idea how it's causing
> the problem, but commenting that one line out makes things work again.

tabpanels.selectedIndex = val; is roughly the same as
tabpanels.selectedPanel = tabpanels.childNodes[val];

This compares the passed-in panel with the cached current panel. The cache starts off null, so this always fails on the first selection. The tabpanels thus dispatches a select event, which the tabbrowser catches to call updateCurrentBrowser(). Something in there may be triggering this, maybe the save and restoration of focus. (I don't see the bug myself.)

One way to verify this theory would be to change the default value of _selectedPanel in tabbox.xml to this.childNodes.item(this.selectedIndex)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: This works like a charm

Is this actually the fix?  Or would it break something else?

Comment 8

[[e-mail]]

Well the other approach is to make tabbrowser expect an initial select event. I don't know how well that would work with XBL e.g. could the tab constructor fire before the tabbrowser constructor in which case things would get confused?

Comment 9

[Neil Deakin]

We could probably just remove the cached stuff, since it's just storing a child node. In fact, the <tabpanels> implementation of selectedIndex/selectedPanel should be similar to the <deck> one, which doesn't use a cache.

Comment 10

[[e-mail]]

OK, but there are some minor differences between tabpanels and deck:
1. tabpanels' selectedIndex is an integer, while deck's is a string
2. tabpanels' selectedIndex setter range-checks val
3. deck's selectedIndex checks val directly agains the existing value

Comment 11

[Boris Zbarsky [:bzbarsky]]

> could the tab constructor fire before the tabbrowser constructor

Yes.  In fact, generally would for the tab that's in the tabbrowser binding, I think.

Comment 12

[Neil Deakin]

(In reply to comment #10)
> OK, but there are some minor differences between tabpanels and deck:
> 1. tabpanels' selectedIndex is an integer, while deck's is a string
> 2. tabpanels' selectedIndex setter range-checks val
> 3. deck's selectedIndex checks val directly agains the existing value

Minor differences, I think bz's patch may be ok for now as it fixes the bug. I assume something is setting selectedIndex on the tabpanels before the cached value is accessed?

Comment 13

[[e-mail]]

(In reply to comment #12)
>I assume something is setting selectedIndex on the tabpanels before the cached
>value is accessed?
Yes, as per comment #0 the tabs element now generates a selection on creation.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Feel free to nominate a reviewed patch, but this won't block final release.

Comment 15

[[e-mail]]

Comment on attachment 292016 [details] [diff] [review]
This works like a charm

As I mentioned previously, I am unable to reproduce the effect of the bug, so I'm not sure what I'm supposed to review here, however I do wonder which exception you're trying to catch.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> As I mentioned previously, I am unable to reproduce the effect of the bug

That can't possibly be a prerequisite for reviewing this!  I review patches for bugs I can't reproduce all the time.

I'm just implementing what you proposed in comment 6.  I guess I'll try enn, since he seems to think this patch is ok...

> however I do wonder which exception you're trying to catch.

If the index is out of range for the list, there will be an exception.  I have no idea whether it could be out of range, but I see no reason it couldn't (e.g. if the list is 0-length, any index value will be out of range).

Comment 17

[[e-mail]]

(In reply to comment #16)
>I'm just implementing what you proposed in comment 6.
Well, that's another reason why I shouldn't review it ;-)

>If the index is out of range for the list, there will be an exception.
I thought item() just returned null if the index was out of range?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ah, I might be confusing item() with [].  I checked, and item() in fact does not throw.

So I'll take out the try/catch.

Comment 19

[Neil Deakin]

Comment on attachment 292016 [details] [diff] [review]
This works like a charm

I'd say this is ok as it fixes the bug.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 292016 [details] [diff] [review]
This works like a charm

requesting approval

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 292016 [details] [diff] [review]
This works like a charm

a1.9=beltzner

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed, with the try/catch removed.  I have no idea how to test this.