297887 - Form values are not correct with bfcache enabled

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Markus Hübner]

Using trunk build 20050609 on win-xp pro.
To reproduce try these steps:
1) Go to http://www.google.com and type in "mozilla" and hin enter.
2) Now type "browser" in the form-field and hit enter.

When you go back one page now you still see "browser" in the form field 
instead of "mozilla".

Comment 1

[Martijn Wargers (dead)]

See bug 293203, comment 21.
I think that bfcache enabled prevents the onload event to get fired when going
back into history (I think that's intentional, afaict from bug 274784).
So this is an example where bfcache breaks an url because of th lack of
onload-on-Back, see bug 274784, comment 5.

Comment 2

[Brendan Eich [:brendan]]

bryner has dibs on bfcache bugs, and can probably get content fixed here if that
is necessary.

/be

Comment 3

[Brian Ryner (not reading)]

fwiw, Safari and Opera behave the same way (neither of them fire the onload 
handler to the page when going back)

Comment 4

[Brendan Eich [:brendan]]

Alright, I'm slow today.  What does 1.0.x do that 1.1a1 with fact-back enabled
does not do?  The google SERP page does document.gs.reset() from its onload; the
main google page just sets focus.  I don't see a difference in the contents of
the input element in either page, following comment 0's steps (including
unnumbered step 3, and even a step 4 that goes back again).

I'm going to assume in the next sentence that I'm just confused above, and that
we need to fix this bug somehow, with browser-only code changes:

If we could discern that some pages need onload to be run, by monitoring what
DOM mutations onload causes, perhaps we could do something.

But in the better case, I would rather we not try to be too clever.  Instead, we
should work with other browser vendors to extend the DOM with the PageHide and
PageShow events that bryner has proposed.  Let's make progress in the DOM after
years of stagnation, and make it possible for page authors to help us to comply
with RFC 2616 (which BTW seems blissfully unaware of onload and dynamic content
issues it raises).

/be

Comment 5

[Brian Ryner (not reading)]

The difference is that the search box at the top of the page always shows what
you last typed into it, with fastback enabled.  That means if you type in a
different search term and submit, clicking back will show the different search
term, not the term which generated the search results on that page.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is one of the rare cases where you *don't* want to return to the page as
you left it. I've actually written pages like this myself although with a slight
modification. I had a <select size=1> that was used for navigation. When the
user selected something in the dropdown it navigated off to the selected page,
but when going 'back' to the page you'd want to show the original value (which
said something like "quick navigation") rather then the selected item.

Of course we could use some heuristics and detect when the 'onload' reset some
form and then reset it ourselvs on PageShow. But heuristics like that tend to
just annoy more then help (MS word being a prime example) so I defenetly don't
think it's the way to go.

So the only resonable solution IMHO is evangelize. There is already the option
to use the PageShow event and resetting when persisted=true.

An even better long term solution would be to have an attribute on the
form-control allowing the author to specify that a form or formfield should not
be persited across page transitions. We would then not stick the formfield in
the sessionhistory, and we'd reset it before bringing it back from bfcache.

Comment 7

[Hixie (not reading bugmail)]

sicking: Doesn't autocomplete="off" handle that case?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It should, but it does other things too which are not desiarable (i.e. it
disables autocomplete :) )

Comment 9

[Brian Ryner (not reading)]

I think we should probably leave the current behavior, however, it does seem
like a good idea to give the author an easy way to reset the form when the page
is restored.  Keeping the bug open for that.

Comment 10

[Brett Wilson]

Attachment: Testcase for onPagesShow/Hide events on body tag

Attached a testcase for a future patch that will implement onPageShow for the
body element to allow web page designers to detect this event.

Comment 11

[Brett Wilson]

Attachment: Testcase for onPagesShow/Hide events on "window.onpage...

Comment 12

[Brett Wilson]

Attachment: Patch to expose onPageShow/onPageHide

This patch exposes onPageShow and onPageHide via the body element and
window.onPage[Show|Hide]. The case of the existing events was changed to all
lower case, which is required for html attributes.

I changed case for more things than were required for completeness, like when
used as an argument to EqualsIgnoreCase.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 190175 [details] [diff] [review]
Patch to expose onPageShow/onPageHide

>--- dom/public/coreEvents/nsIDOMPageTransitionListener.h	15 Jun 2005 23:52:37 -0000	1.1
>+++ dom/public/coreEvents/nsIDOMPageTransitionListener.h	22 Jul 2005 17:43:26 -0000
>@@ -47,12 +47,12 @@ class nsIDOMEvent;
>  */
> #define NS_IDOMPAGETRANSITIONLISTENER_IID \
> { 0x24f4d69f, 0x6b0c, 0x48a8, { 0xba, 0xb7, 0x12, 0x50, 0xcb, 0x5e, 0x48, 0x79 } }
> 
> class nsIDOMPageTransitionListener : public nsIDOMEventListener {
>  public:
>   NS_DEFINE_STATIC_IID_ACCESSOR(NS_IDOMPAGETRANSITIONLISTENER_IID)
> 
>-  NS_IMETHOD PageShow(nsIDOMEvent* aEvent) = 0;
>-  NS_IMETHOD PageHide(nsIDOMEvent* aEvent) = 0;
>+  NS_IMETHOD pageshow(nsIDOMEvent* aEvent) = 0;
>+  NS_IMETHOD pagehide(nsIDOMEvent* aEvent) = 0;
> };
> #endif // nsIDOMPageTransitionListener_h__

These should stay uppercase for consistency with the rest of the
nsIDOM*Listener interfaces.

>--- dom/src/base/nsDOMClassInfo.cpp	29 Jun 2005 03:51:43 -0000	1.286
>+++ dom/src/base/nsDOMClassInfo.cpp	22 Jul 2005 18:05:06 -0000
>@@ -1093,17 +1095,16 @@ jsval nsDOMClassInfo::sAdd_id           
> jsval nsDOMClassInfo::sAll_id             = JSVAL_VOID;
> jsval nsDOMClassInfo::sTags_id            = JSVAL_VOID;
> jsval nsDOMClassInfo::sAddEventListener_id= JSVAL_VOID;
> 
> const JSClass *nsDOMClassInfo::sObjectClass = nsnull;
> 
> PRBool nsDOMClassInfo::sDoSecurityCheckInAddProperty = PR_TRUE;
> 
>-

Stray whitespace change?

Also, the changes to files under mozilla/toolkit need to be made to the
corresponding files in mozilla/xpfe.

Looks good otherwise.

Comment 14

[Brett Wilson]

Attachment: Patch to expose onPageShow/onPageHide

New patch implementing bryner's suggestions.

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 190416 [details] [diff] [review]
Patch to expose onPageShow/onPageHide

I assume that the changes to xpfe/components/shistory aren't supposed to be
part of this patch.  Looks good otherwise.

Requesting approval to land this -- this patch makes it easier for page authors
to interact with fastback by exposing "onpagehide" and "onpageshow" so that
they can be used on the <body> tag like onload.

Comment 16

[Brett Wilson]

Yes, the changes in xpfe/components/shistory are erroneous, sorry.

Comment 17

[Brian Ryner (not reading)]

(In reply to comment #14)
> Created an attachment (id=190416) [edit]
> Patch to expose onPageShow/onPageHide
> 
> New patch implementing bryner's suggestions.

I checked in this patch.

I'm also of the opinion that we might want to add an attribute that causes a
form reset to happen automatically on pageshow, i.e.

<form reset="true">

Comment 18

[Simon Fraser [no longer active]]

Could we use these onshowpage/onhidepage events to fix bug 277067?

Comment 19

[Brian Ryner (not reading)]

The events aren't fired when switching tabs, only when a page is put into or
restored from session history.

Comment 20

[Brian Ryner (not reading)]

The <form reset="true"> idea is actually a different problem, because it's
applicable whether or not fastback is enabled (see google.com search results,
which reset the form to workaround form-value saving, independently of bfcache).

So marking this fixed.

Comment 21

[Markus Hübner]

Well, but what about all sites out there where webmasters don't update their 
sites. There Mozilla will still show the wrong behaviour, or?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Webmasters would need to update their site for the reset="true" feature too.

Comment 23

[Phil Ringnalda (:philor)]

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