306660 - [FIX]Bottom frame in frameset doesn't load.

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Johnny Stenback (:jst)]

Load the URL and click on "Airport names" and you'll get a new window. The
bottom frame in that window should display help about how to search for airport
names, and once you click on a letter or do a search the result should be shown
in the bottom frame, but nothing is ever loaded there. This actually worked
before the fix for bug 306630 landed if you triggered it by manually loading
(using javascript: URLs in the opening page) one of the URLs that the page tries
to load in the frameset, but due to that bug nothing loaded at all in the frames
when the frameset was loaded.

I don't know what's going on here yet, but I'm guessing this is somehow related
to split windows. What I do see is that when you click on one of the links in
the top frameset the code does set location.href for the frame, but nothing is
ever loaded. I see no assertions or JS errors...

Comment 1

[Johnny Stenback (:jst)]

bz says he's in the know on what's going on here. We're creating layout frames
for the frameset children before all the frameset children are inserted into the
DOM and the frameset frame doesn't apparently deal with dynamic changes (for
historic reasons), but could now... Over to bz.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Belt-and-suspenders patch

There are two changes here.  The first is to not flush layout in Close() if the
document has no presshell.  The second is to deal with dynamic addition/removal
of frames in a frameset.  Both are enough to fix this bug, but I suspect we
need the latter to cover other cases where a reflow flush happens...  So I made
the document change just to prevent an extra reframe in this case.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch version

Comment 4

[Boris Zbarsky [:bzbarsky]]

That fixes bug 198251 too.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 194539 [details] [diff] [review]
Belt-and-suspenders patch

r=jst

Comment 6

[Scott MacGregor]

Hey guys, did this turn out to be split window fall out or a regression caused
by another recent change? Or is it a regression from another change? If so, can
you add the regredssion keyword? it makes it easier for the bug triage team. thanks!

Comment 7

[Boris Zbarsky [:bzbarsky]]

This is a regression from the fix for bug 253951

Comment 8

[Mike Schroepfer]

Can we get this landed on the trunk so we can verify?  Thanks!

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed.  Note that the trunk patch is the one that applies to branch now...

Comment 10

[Mike Schroepfer]

Verified on 9-6-08 trunk build.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed on the 1.8 branch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcases

Comment 13

[Boris Zbarsky [:bzbarsky]]

Checked in the tests.