Closed
Bug 405390
Opened 17 years ago
Closed 17 years ago
Sized windows flicker at incorrect size when opening
Categories
(Toolkit :: XUL Widgets, defect, P2)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
()
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
enndeakin
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
Flags: blocking1.9?
Comment 1•17 years ago
|
||
(In reply to comment #0)
> 2) Click the javascript: URI in the URL field
What javascript: URI? :)
Assignee | ||
Updated•17 years ago
|
Comment 2•17 years ago
|
||
I'm not able to reproduce this on Linux or other platforms. Anyone else see this?
Assignee | ||
Comment 3•17 years ago
|
||
Did you try reproducing with Seamonkey?
Comment 4•17 years ago
|
||
Bz - why did you nom this?
Assignee | ||
Comment 5•17 years ago
|
||
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...
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment 6•17 years ago
|
||
(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)
Assignee | ||
Comment 7•17 years ago
|
||
Is this actually the fix? Or would it break something else?
Comment 8•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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
Assignee | ||
Comment 11•17 years ago
|
||
> 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•17 years ago
|
||
(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•17 years ago
|
||
(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•17 years ago
|
||
Feel free to nominate a reviewed patch, but this won't block final release.
Flags: tracking1.9+
Assignee | ||
Updated•17 years ago
|
Attachment #292016 -
Flags: review?(neil)
Updated•17 years ago
|
Flags: wanted1.9+
Comment 15•17 years ago
|
||
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.
Attachment #292016 -
Flags: review?(neil)
Assignee | ||
Comment 16•17 years ago
|
||
> 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).
Assignee | ||
Updated•17 years ago
|
Attachment #292016 -
Flags: review?(enndeakin)
Comment 17•17 years ago
|
||
(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?
Assignee | ||
Comment 18•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 292016 [details] [diff] [review]
This works like a charm
I'd say this is ok as it fixes the bug.
Attachment #292016 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 20•17 years ago
|
||
Comment on attachment 292016 [details] [diff] [review]
This works like a charm
requesting approval
Attachment #292016 -
Flags: approval1.9?
Comment 21•17 years ago
|
||
Comment on attachment 292016 [details] [diff] [review]
This works like a charm
a1.9=beltzner
Attachment #292016 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 22•17 years ago
|
||
Fixed, with the try/catch removed. I have no idea how to test this.
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Updated•14 years ago
|
Assignee: nobody → bzbarsky
You need to log in
before you can comment on or make changes to this bug.
Description
•