144301 - Shift-reload on a frameset page causes misbehavior in back/forward.

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Radha on family leave (not reading bugmail)]

From bug 135683:

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc2) Gecko/20020510

I tried to make sure this was the right place to post this comment.  I've been
seeing similar problems with back buttons and frames.  I created a test case
which has been able to reproduce the problem every time I've tested it so far.

1.) Go to this page:
http://www.embimedia.com/temp/moz_frames/index.html
2.) Click reload while holding the Shift key.
3.) Click the "Go forward" link.
4.) On the next page, click the button labeled "Go back".

The button labeled "Go back" and the browser back button both seem to be dead.

Seems like strange behavior to reproduce, but maybe this will help you track
down the problems.  I've seen similar trouble with the back button in other
cases, but this was the best test case I could come up with.

Comment 1

[Radha on family leave (not reading bugmail)]

Attachment: Patch to docshell

This patch takes care of the problem with shift-reload in OnNewURI(). Changes
to other methods LoadURI() and AddToSessionHistory() are primarily
consolidation or cleanup of the existing code.

Comment 2

[rpotts (gone)]

Comment on attachment 85677 [details] [diff] [review]
Patch to docshell

sr=[e-mail]

i think this looks ok ;-)
this kind of change is always tricky to follow!

Comment 3

[Adam Lock]

Comment on attachment 85677 [details] [diff] [review]
Patch to docshell

r=adamlock

The OnNewURI stuff looks fine. I think we need more context in future session
history patches though.

Comment 4

[Radha on family leave (not reading bugmail)]

This is a regression from few other bugs fixed long time ago. I would like to
get this in for 1.0.1 since I believe shift-reload is something used frequently
by web developers. 

Comment 5

[Radha on family leave (not reading bugmail)]

The patch attached here also has the one-liner patch for bug 128951. 128951 is
not a regression. 

Comment 6

[Radha on family leave (not reading bugmail)]

miloslaw:  can you help me  test this a little bit. This has been checked into
the trunk. Can you try out the patch for couple of days and let me know if you
see any regressions. Specifically navigating through subframes and pages that do
refresh. Thanks.

Comment 7

[Miloslaw Smyk]

Ok, I am ready to give my tree a spin, but a question first: how is session
history supposed to behave on shift-reload? Should it replace current entry with
a fresh one? Or should it create a new entry and put it after the current one?

Comment 8

[Mike Brittain]

This is still a reload of a page in the history, but with all cached files
essentially expired, right?  I don't see any reason why it should create a new
entry in the browser history.  It doesn't make sense that you'd be able to go
"back" to the old version before the reload.  It does make sense (to me) that
the current history entry should be overwritten in the case that the page title
or other attributes have been updated.  The same, I would assume, should go for
a normal (not-shift) reload.

Comment 9

[Miloslaw Smyk]

Sounds fair, but if you're surfing within frameset, reloading will take you back
to the original URL. So, perhaps reloading should be fixed to reload each frame
separately? Is this feasible at all?

Comment 10

[Andrew Hagen]

See bug 46845 for a related discussion.

Comment 11

[Radha on family leave (not reading bugmail)]

session history will replace the current entry with the new one for all pages
including framesets. I just need help making sure this hasn't regressed other
stuff in going back/forward or reloading. Thanks.

Comment 12

[Miloslaw Smyk]

Andrew, no sign of frames being discussed in bug #46845. But in fact this an
interesting one and I believe that correct solution has been proposed in comment
#58. ;)

Anyway, Radha - do what I do:

1. Visit http://java.sun.com/j2se/1.4/docs/api/index.html
2. Click java.lang in upper-left frame
3. Click Math in bottom-left frame
4. Do shift-reload
5. Observe that you're no longer reading description of the Math class, but
rather you're back to the beginning, i.e. just after step 1.

That's what is troubling me (somewhat).


As for the patch, I've been testing it for a few hours today, no problems
discovered so far. But I'll try. :)

Comment 13

[Radha on family leave (not reading bugmail)]

Miloslaw: At step 5, it is suppose to take you back to the very beginning. 
shift-reload supersedes what is in history and cache and loads the page as it is
in the server. The problem was, after you do shift-reload on a frameset page, if
you continue to browse in the site and do back, the back used to do nothing. The
patch takes care of this problem. 

Comment 14

[Radha on family leave (not reading bugmail)]

Fix checked into the trunk.

Comment 15

[Mike Brittain]

In response to Miloslaw, comment #12 -- There is a context menu (right-click)
This Frame > Reload Frame which can be used with the shift-key modifier to
completely reload the frame.  I believe I have been using this in development
with much success.

Comment 16

[Jaime Rodriguez, Jr.]

could this at all be related to bug 126826 | 'Back' and 'Forward' buttons not
working?

claudius - can you verify this fix on the trunk? thanks!

Comment 17

[Radha on family leave (not reading bugmail)]

This is not related to 126826. That bug is about back/forward completely not
enabled since startup due to possible corruption with profile/all.js. This is
about a specific problem on framesets when you do a shift-reload on them.

Comment 18

[Jaime Rodriguez, Jr.]

adt1.0.1- on the ADT's behalf. should you still believe this should go into
1.0.1, pls renominate by removing the minus (ex. adt1.0.1)from the keyword.

let's get it in the next release. thanks!

Comment 19

[sairuh (rarely reading bugmail)]

looks fixed, using 2003.02.19 comm trunk on win2k and linux rh8.0.

Comment 20

[Boris Zbarsky [:bzbarsky]]

This did in fact regress going back/forward when POSTDATA is involved....  see
bug 227672.