48422 - [FIX]Changing of frameset's ROWS/COLS reloads all children

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P3)

Description

[bcortez]

Milestone: M17
Build: 2000080908


  When programmatically changing the frameset dimensions (column width and row 
width) via JavaScript and the DOM, the existing child frames are reloaded again 
(which they shouldn't).  Also, I noticed that if you resize the frameset in this 
manner, you no longer have access to code across the frames. This is 
inconsistent with the behavior as a result of physically moving the frameset 
border using the mouse.  Manually changing the frameset dimensions (dragging the 
border) behaves as it should and does not reload the child frames and retains 
access to code across the frames. This is a serious inconsistency in behavior.

  In the Master Frameset I have a Javascript function defined which changes the  
column value of the parent frameset using the following code snippet...

        var frameset = document.getElementsByTagName("frameset");
        frameset.cols = "10,*";

  When this function is invoked from one of the child frames, the Master 
Frameset columns change to "10,*" as expected, however, the browser then reloads 
each child frame from scratch (firing the onload events along the way) and 
breaks the access to code across the frames! It doesn't request them from the 
server though since the request does not show up in the webserver logs.  It must 
be refreshing them from its copy in memory instead of simply repainting the 
viewable area on the fly (as I assume moving the border manually does).

  In addition, any reloading of the page as a result of programmatically 
changing the frameset dimensions causes the frameset to reload each of its child 
frames.  This effectively wipes out any DHTML changes that were made to the 
pages contained within the child frames after their initial load. The W3C DOM 
doesn't actually state what the behavior concerning changing the frameset 
dimensions should be, however, it shouldn't reload the child frames from 
scratch.  It should behave consistently as though the user moved the frame 
border manually and NOT reload the child frames.


NOTE: This is similar to the _major_ flaw in the NS4 level browser concerning 
the DHTML changes which may have occurred after the initial page load (the old 
'resize bug'). 


  IE5 handles changing the frameset dimensions consistently and DOES NOT reload 
the child frames.  Compare the behavior of both browsers for the follwoing URL:

         http://www.ccs.neu.edu/home/bcortez/mozilla_tests/testPage3.htm



Note: I also tested this on Win2K and it has the same behavior (no surprise 
there).

Comment 1

[Doron Rosenberg (IBM)]

over to event handling, could be dom 0

Comment 2

[Gervase Markham [:gerv]]

Confirming for evaluation.

Gerv

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

jst sez this is pollmann's.

Comment 4

[Eric Pollmann]

Yes, this is due to my admittedly hacky fix for bug 7913 which recently went
in.  Before that fix, changing ROWS or COLS didn't work at all!  :o  I have
fixes that do the right thing and reuse existing frames - these patches were
written in Oct 1999 and are attached to bug 7913.  Unfortunately, they have are
quite complex and have bit-rotted.  I don't know if fixing this bug is a
reasonable and safe change for beta3/final.  Marking Future, but I hope we can
get to this one.  Here's the official spiel:

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way
-- please attach your concern to the bug for reconsideration.

Comment 5

[Val Sharp]

Apologies if this is already known, but when I try the testcase in Windows 98 
with M17, when frameset.cols is changed the cols are not actually changed on the 
screen.

Comment 6

[Eric Pollmann]

Thanks Val!  Bug 7913 is incorrectly marked FIXED / M17.  I just changed it to
correctly be FIXED / M18.  To see that fix you'll have to download a nightly
build or the M18 build (when that comes out).

Comment 7

[Keyser Sose]

WORKSFORME
Platform: PC
OS: Windows 98
Mozilla Build: 2001012205

Marking as such.

Comment 8

[bcortez]

This bug is NOT fixed.  It should not be listed as RESOLVED WORKSFORME.  I just 
tested my ssample page on Mozill Nightly Build ID:2001012304 and it still fails 
as before. 

Platform: PC
OS: Windows 98
Mozilla Build: 2001012304 

Test URL: http://www.ccs.neu.edu/home/bcortez/mozilla_tests/testPage3.htm

Comment 9

[bcortez]

Mozilla Build ID: 2001070204

This BUG still does not wrk with my test URL:

http://www.ccs.neu.edu/home/bcortez/mozilla_tests/testPage3.htm

This bug is still incorrectly marked RESOLVED WORKSFORME !!!!

Can someone please confirm this problem???????

Comment 10

[Jan Carpenter]

updating QA Contact

amar, can you look into this? 
thanks!

Comment 11

[Eric Pollmann]

Bug was not fixed.

Comment 12

[Kevin McCluskey (gone)]

Bulk reassignin HTML FRAME/IFRAME bugs to Eric.

Comment 13

[bcortez]

Still fails as of Mozilla Build 2001110903 (Win2k).   Is this bug even on the 
radar screen???


New TestCase URL (same page, different URL):

http://www.ccs.neu.edu/home/bcortez/mozilla_tests/48422/index.html

Comment 14

[Håkan Waara]

bcortez, can you attach a simplified testcase to this bug? (strip off as much
HTML as possible, so we know where the culprit is)

In your testcase, also make it clear what the expected/actual behaviour is.

Thanks

Comment 15

[bcortez]

Well, as I mentioned in my comment onn 11/09, there is a simple test case URL 
listed.  I will list it again here for clarity.  It doesn't get much simpler 
that this test case....


http://www.ccs.neu.edu/home/bcortez/mozilla_tests/48422/index.html

Comment 16

[Alexandru Savulov]

adding a weak dependency

Comment 17

[Kevin McCluskey (gone)]

Bulk re-assigning all of Eric's HTMLFrame bugs to John.

Comment 18

[Tarmo Järvalt]

Attachment: Simple testcase for dynamically changing cols and rows with JS

Testcase with 3 frames, changing both cols and rows.
Works with IE 5.5 SP2, doesn't with Gecko/20020221.

Currently for some reason Gecko/20020221 doesn't change rows until columns get
changed.

Comment 19

[bcortez]

Platform: PC
OS: Windows 2000
Mozilla Build: 2002030308 

After testing with the Mozilla build listed above, I get different strange 
frameset behavior such as:

1. When "Click to hide/show frameRight" is pressed, the bottom frame gets 
hidden bu the browser display is not refreshed.
2. Subsequently, when "Click to hide/show frameLeft" is then pressed, the 
display is refreshed and the 'frameRight' is now hidden.
3. This refresh error occurs when the frame is hidden then shown as well.


It seems that this latest "simple" testcase (attachment id=70924) doesn't 
perform _any_ Javascript function calls _across_ the framesets.  This is what I 
explained gets broken in my original post when I stated, "you no longer have 
access to code across the frames".  

The simple testcase _does not_ address this problem.  Therefore, I don't think 
it should be considered a valid test case for this bug since it doesn't 
reproduce the error properly.  It does, however, introduce a new erroneous 
behavior with dynamic frame resizing using the DOM method calls.

Comment 20

[Kevin McCluskey (gone)]

*** Bug 81889 has been marked as a duplicate of this bug. ***

Comment 21

[Corrado Berti]

suggesting change the summary to "programmatically changing of frameset's...."
since the frames aren't reloaded if user resize them by dragging the 
frameset's borders.

Comment 22

[Kai Lahmann (is there, where MNG is)]

*** Bug 109252 has been marked as a duplicate of this bug. ***

Comment 23

[Torsten Rendelmann]

Attachment: extended test: tests also the rows property changes

The "70924: Simple testcase for dynamically changing cols and rows with JS"
does test cols changes only. This new test includes also rows changes.

And: changing the rows property with JS does not yet work! While changing cols
work s a bit...

Comment 24

[Erik Arvidsson]

Can we get a target milestone for this? This is important DOM1 stuff and I 
would like to see a higher priority for this bug.

I'm adding DOM1 keyword.

http://www.w3.org/TR/REC-DOM-Level-1/level-one-html.html#ID-43829095

Comment 25

[Felix Miata]

I wonder if http://bugzilla.mozilla.org/show_bug.cgi?id=145222#c6 has any
bearing on this?

Comment 26

[John Keiser (jkeiser)]

*** Bug 146701 has been marked as a duplicate of this bug. ***

Comment 27

[Boris Zbarsky [:bzbarsky]]

*** Bug 63980 has been marked as a duplicate of this bug. ***

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 29

[Boris Zbarsky [:bzbarsky]]

That just happens to fix bug 109322 as well.

Comment 30

[John Keiser (jkeiser)]

Comment on attachment 90596 [details] [diff] [review]
Proposed patch

r=jkeiser with a few comment caveats:

1. switch aSpecs and aValues around in nsHTMLFramesetFrame::GenerateRowCol
2. comment saying SetAttr overriding one in nsGenericHTMLElement
3. JavaDoc comments on the member vars!
4. A comment on that SetAttr() hint weirdness would be quite nice
5. Need @param on the JavaDoc comments for GetRowSpec/GetColSpec, and @return
if there are error messages

Patch is quite spiffy otherwise, with the exception of the necessary, and
unavoidable, SetAttr voodoo.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase to test changing "rows" in a way that should add more frames

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: Same thing but with "cols"

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v 2.0

addresses jkeiser's comments.  Passes all testcases attached to this bug.

Comment 34

[Boris Zbarsky [:bzbarsky]]

I may as well take this...

Comment 35

[Håkan Waara]

Wow, nice fix Boris!

Comment 36

[Johnny Stenback (:jst)]

Comment on attachment 90751 [details] [diff] [review]
Patch v 2.0

- In nsHTMLFrameSetElement::GetRowSpec()

+  nsHTMLValue value;
+  nsAutoString rows;
+
+  if (!mRowSpecs) {
+    if (NS_CONTENT_ATTR_HAS_VALUE == GetHTMLAttribute(nsHTMLAtoms::rows,
value) &&
+	 eHTMLUnit_String == value.GetUnit()) {
+      value.GetStringValue(rows);

This is the only scope where |rows| is used, so move the declaration of that
nsAutoString into this scope.

- In nsHTMLFrameSetElement::GetColSpec(), same thing with the |cols| variable.

- In nsHTMLFrameSetElement::ParseRowCol()

+  nsAutoString rowsCols(aValue);
+
+  if (!rowsCols.IsEmpty()) {
...

|rowsCols| is only used inside this scope, so why not check if
(!aValue.IsEmpty()) and do the string construction and copy only if that's the
case?

- In nsHTMLFrameSetElement::ParseRowColSpec()

+  static const PRUnichar ASTER('*');
+  static const PRUnichar PERCENT('%');
+  static const PRUnichar COMMA(',');

Please name these differently, the all uppercase names suggest that these are
macro's, how about kAster, or, sAster in stear?

- In nsHTMLFrameSetElement::ParseRowColSpec()

+      // Treat * as 1*
+      if ((eFramesetUnit_Relative == aSpecs[i].mUnit) &&
...
+      }
+
+      // Otherwise just convert to integer.
+      else {
...
+      }

Loose the empty line between the end of the if and the else, and ideall move
the comment above the else inside the else. The current way is too inviting for
someone to go add code between the if and the else, not good (IMO).

...

+	 mDocument->QueryInterface(NS_GET_IID(nsIHTMLDocument),
+				   getter_AddRefs(htmlDocument));

How about htmlDocument = do_QueryInterface(mDocument)...

sr=jst with those minor issues fixed.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: fix jst's issues

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 92065 [details] [diff] [review]
fix jst's issues

this has r=jkeiser/sr=jst

Comment 39

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 40

[Tarmo Järvalt]

There seems to be a redraw bug lurking somewhere because changing rows height
with JS doesn't redraw the whole frame. After moving the frame manually or
dragging another window over it redraws.

Visible in first attachement testcase:
http://bugzilla.mozilla.org/attachment.cgi?id=70924&action=view

This patch seems to fix bug 116542 but where would I go with the redraw problem,
here or new bug?

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1b) Gecko/20020808

Comment 41

[John Keiser (jkeiser)]

Please file a new bug, thanks :)

Comment 42

[Boris Zbarsky [:bzbarsky]]

Yes, new bug.  Especially because I cannot reproduce the redraw problem on Linux
(so chances are, it's Windows-only).

Comment 43

[Tarmo Järvalt]

Filed bug 161954 about the rendering issue.

Comment 44

[Boris Zbarsky [:bzbarsky]]

*** Bug 162320 has been marked as a duplicate of this bug. ***

Comment 45

[Matthias Versen [:Matti]]

*** Bug 162814 has been marked as a duplicate of this bug. ***

Comment 46

[John Keiser (jkeiser)]

*** Bug 163578 has been marked as a duplicate of this bug. ***

Comment 47

[Boris Zbarsky [:bzbarsky]]

*** Bug 167963 has been marked as a duplicate of this bug. ***

Comment 48

[Boris Zbarsky [:bzbarsky]]

*** Bug 165748 has been marked as a duplicate of this bug. ***

Comment 49

[Jason Hiltz-Laforge]

Attachment: Patch to be ported to the 1.0.x stream

Comment 50

[Jason Hiltz-Laforge]

1.0.x patch deleted.  Added prematurely.  Just figuring out how this bugzilla
works :-)