Closed
Bug 297887
Opened 19 years ago
Closed 19 years ago
Form values are not correct with bfcache enabled
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla1.8beta4
People
(Reporter: markushuebner, Assigned: brettw)
References
()
Details
(Keywords: regression, Whiteboard: [no l10n impact])
Attachments
(3 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bryner
:
review+
benjamin
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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".
Reporter | ||
Updated•19 years ago
|
Component: General → History: Session
Product: Mozilla Application Suite → Core
Reporter | ||
Updated•19 years ago
|
Assignee: general → nobody
QA Contact: general → history.session
Comment 1•19 years ago
|
||
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•19 years ago
|
||
bryner has dibs on bfcache bugs, and can probably get content fixed here if that
is necessary.
/be
Assignee: nobody → bryner
Comment 3•19 years ago
|
||
fwiw, Safari and Opera behave the same way (neither of them fire the onload
handler to the page when going back)
Comment 4•19 years ago
|
||
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•19 years ago
|
||
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.
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.8b3?
OS: Windows XP → All
Hardware: PC → All
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•19 years ago
|
||
sicking: Doesn't autocomplete="off" handle that case?
It should, but it does other things too which are not desiarable (i.e. it
disables autocomplete :) )
Updated•19 years ago
|
Flags: blocking-aviary1.1?
Updated•19 years ago
|
Flags: blocking1.8b4+
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Flags: blocking-aviary1.1?
Updated•19 years ago
|
Flags: blocking1.8b4?
Updated•19 years ago
|
Whiteboard: [no l10n impact]
Comment 9•19 years ago
|
||
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.
Target Milestone: --- → mozilla1.8beta4
Assignee | ||
Updated•19 years ago
|
Assignee: bryner → brettw
Assignee | ||
Comment 10•19 years ago
|
||
Attached a testcase for a future patch that will implement onPageShow for the
body element to allow web page designers to detect this event.
Updated•19 years ago
|
Flags: blocking1.8b4?
Assignee | ||
Comment 11•19 years ago
|
||
Assignee | ||
Comment 12•19 years ago
|
||
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•19 years ago
|
||
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.
Attachment #190175 -
Flags: review+
Assignee | ||
Comment 14•19 years ago
|
||
New patch implementing bryner's suggestions.
Attachment #190175 -
Attachment is obsolete: true
Comment 15•19 years ago
|
||
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.
Attachment #190416 -
Flags: review+
Attachment #190416 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #190416 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 16•19 years ago
|
||
Yes, the changes in xpfe/components/shistory are erroneous, sorry.
Comment 17•19 years ago
|
||
(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•19 years ago
|
||
Could we use these onshowpage/onhidepage events to fix bug 277067?
Comment 19•19 years ago
|
||
The events aren't fired when switching tabs, only when a page is put into or
restored from session history.
Comment 20•19 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 21•19 years ago
|
||
Well, but what about all sites out there where webmasters don't update their
sites. There Mozilla will still show the wrong behaviour, or?
Webmasters would need to update their site for the reset="true" feature too.
Comment 23•18 years ago
|
||
*** Bug 361052 has been marked as a duplicate of this bug. ***
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
You need to log in
before you can comment on or make changes to this bug.
Description
•