Closed
Bug 72197
Opened 24 years ago
Closed 23 years ago
setting location= in javascript: link replaces current page in session history
Categories
(Core :: DOM: Navigation, defect, P3)
Core
DOM: Navigation
Tracking
()
VERIFIED
FIXED
mozilla0.9.1
People
(Reporter: blizzard, Assigned: radha)
Details
(Keywords: testcase)
Attachments
(4 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Here's a bit of JS that I use to load bugs. It's in my bookmarks as the
property of the bookmark. When the resulting url is loaded the back button
isn't highlighted and it hasn't been added to the history as near as I can tell.
You have to do this with a newly opened browser window.
<DT><A HREF="javascript:id=document.getSelection();if(!id){void(id=promp
t('Show Mozilla bug no.',''))}if(id)location.href='http://bugzilla.mozilla.org/s
how_bug.cgi?id='+escape(id)+' '" ADD_DATE="943845841">Show Bug...</A>
Comment 1•24 years ago
|
||
session history --> radha. and this is either a dupe or a recent regression.
Assignee: alecf → radha
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.1
Assignee | ||
Comment 2•24 years ago
|
||
Does this JS code in a frameset page or create a frameset page? There's already
a bug that says, if the first page loaded on a browser window is a frameset
page, back button fails to work. This bug is assigned to mscott.
No, this happens with any javascript: URL, where ``result'' means ``URL load
from the setting of document.location during the execution of a javascript: URL
that doesn't return a non-void result''.
The bookmarklet on the front page of bugzilla is a great example, and this bug
is insanely annoying, for the record.
Assignee | ||
Comment 4•24 years ago
|
||
This bug is caused by the checkin for bug 39938 in js/dom/src/nsLocation.cpp.
When the location.href is processed, docshell is still busy with the alert
dialog, which makes the new url to replace the existing one, instead of a normal
load.
Comment 5•24 years ago
|
||
If a javascript: bookmark (bookmarklet) or javascript: link loads a URL with
[document.]location[.href], the new URL replaces the current page in session
history. This happens even if the link/bookmarklet doesn't call alert().
Additional URLs with bookmarklets:
http://www.google.com/options/netscape6.html (previously linked to on Google's
front page)
http://www.squarefree.com/bookmarklets/pagedata.html#validate
Severity: minor → normal
OS: Linux → All
Hardware: PC → All
Summary: url that is the result of a javascript: url doesn't end up in history → setting location= with javascript replaces current page in session history
Comment 6•24 years ago
|
||
Updated•24 years ago
|
Keywords: testcase
Summary: setting location= with javascript replaces current page in session history → setting location= in javascript: link replaces current page in session history
Assignee | ||
Comment 7•24 years ago
|
||
Assignee | ||
Comment 8•24 years ago
|
||
Assignee | ||
Comment 9•24 years ago
|
||
The attached patch takes care of bookmarklets, onLoadHandlers and simple script
tags that have a location.href in them.
Comment 10•24 years ago
|
||
sr=alecf
Reporter | ||
Comment 11•24 years ago
|
||
Radha, who needs to review this?
Assignee | ||
Comment 12•24 years ago
|
||
I would like rpotts to take a look at this. I will talk to him today.
Comment 13•24 years ago
|
||
Hi Radha,
I talked with jst about this one and he suggested that you add a
IsExecutingEventHandler(PRBool *aResult) to nsIScriptContext.
It turns out that event handlers are executed by calling CallEventHandler(...)
while other scripts are executed by calling EvaluateString(...)
So, inside on nsLocation you can check
nsIScriptContext::IsExecutingEventHandler(...) to determine the replacement
policy.
This seems to limit the number of dependencies and keeps knowledge of script
evaluation out of the DocShell (and nsDocument) :-)
How does this sound?
-- rick
Assignee | ||
Comment 14•24 years ago
|
||
That does sound better. As we discussed in the morning, I was trying to use
nsDocument for maintaining the script execution status and it was more messier
than the patch I have attached here. I will try jst's suggestion.
Comment 15•24 years ago
|
||
one nit - please use GetIsExecutingScript() rather than IsExecutingScript() so
that we can use IDL attributes to describe this. (readonly attribute
isExecutingScript;)
Comment 16•24 years ago
|
||
Since nsIScriptContext will never be a [scriptable] interface is there any
advantage to using a 'readonly attribute isExecutingScript' rather than 'boolean
IsExecutingScript' method?
It seems like GetIsExecutingScript() just makes it harder to understand the
code...
Comment 17•24 years ago
|
||
oh, I didn't realize it would never be scriptable...
Assignee | ||
Comment 18•24 years ago
|
||
I tried out the suggestion that jst made. But it doesn't work in all situations.
The original test case posted to this bug by blizzard on 3/16 for the
bookmarklet fails. In this case, the location.href is executed by
EvaluateString(), even though clicking OK in the alert dialog triggers the
location.href. Also, I have to make a special case checking for onLoad handlers,
since we want to do LoadReplace if a onLoadHandler had a location.href in it.
Assignee | ||
Comment 19•24 years ago
|
||
Assignee | ||
Comment 20•24 years ago
|
||
The latest patch is a combination of first and the sugegstion jst made. Th idea
is to isolate the <script> tag from the rest and set a flag in nsIScriptContext
whenever we are in the middle of processing a script tag. This flag is used
bynsLocation to distinguish a regular load from a replace load. So, all
location.href will lead to a normal load except for the ones inside a script
tag. We do not use nsDocument or nsDocShell to pass flags around.
Comment 21•24 years ago
|
||
Hi Radha,
This looks good to me (r=rpotts)
The only comment I have is to change my mind about what I said earlier :-) In
this instance, I think that it would be better to have an attribute called
'processingScriptTag' rather than the IsProcessingScriptTag and
SetProcessingScriptTag methods... So, for consistancy the methods should be
called GetProcessingScriptTag and SetProcessingScriptTag
I admit that I was wrong about the names - and now it's come back to bite me :-)
Reporter | ||
Comment 22•24 years ago
|
||
sr=blizzard
Comment 23•23 years ago
|
||
Whoa, what's up with the new argument to EvaluateScript() in the HTML content
sink? Is the idea that external vs. inline scripts should be treated differently
here, that seams really really wrong to me. If EvaluateScript() is called in the
sink, you know if's because a <script> tag was seen in the HTML that's loaded,
is that not all you're interested in? Also, the attached patch only fixes the
problem for HTML, not for XHTML, you need similar changes in the XML content
sink, having the two work differently doesn't seem like a good idea to me.
Also, to add to the fun, Vidur is rewriting the script loading code and taking
it out of the content sinks all together, so depending one who's checking in
first, changes will be needed in both these diffs, and in Vidur's diffs.
Cc:ing Vidur, please don't check this in yet until we really know what we wanto
do here.
Comment 24•23 years ago
|
||
Hmm, I see this was checked in already, please don't close this before dealing
with the differences between inline vs. external scripts, which I guess is
something Vidur will haveto fix now. Oh well...
Assignee | ||
Comment 25•23 years ago
|
||
jst, the addition argument to EvaluateScript() got added because, in the
original patch, it was getting called with a TRUE flag by ProcessScriptTag() and
with a FALSE flag by OnStreamComplete(). Just minutes before I checked in, I saw
your reply to the email from rpotts that it needs to be TRUE for both the cases.
So, now, like you said, since both the callers pass a TRUE flag, the additional
argument to EvaluateScript() is not needed. I can take that out and make similar
changes to nsXMLContentSink.
Vidur, What is the status of your rewrite? Will you be checking in RSN? Thanks,
Assignee | ||
Comment 26•23 years ago
|
||
vidur's changes has the patch attached to this bug. Markign fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 27•22 years ago
|
||
mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.
if you think this particular bug is not fixed, please make sure of the following
before reopening:
a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).
thanks!
[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]
Status: RESOLVED → VERIFIED
Comment 28•20 years ago
|
||
This checkin caused bug 178729 -- broken history when the script is running in
one window and setting location in a _different_ window.
Component: History: Session → Document Navigation
QA Contact: claudius → docshell
You need to log in
before you can comment on or make changes to this bug.
Description
•