67288 - Target = "_self" in GoLive 4 built-in script ignored

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Ruud]

Please visit the URL above and go to the BBC Sessions page (from the popup in
the upper right corner). On this page, I have included another popup menu
(autocreated by Adobe GoLive 4) that allows you to quickly jump to an anchor on
the same BBC page. When you scrutinize the HTML, you will notice the Target =
"_self" used by the Java script. This is ignored by Mozilla (and Netscape 6);
instead the page is incorrectly opened in a new window.

Netscape Communicator 4.7 and Internet Explorer 5.0 do process the page correctly.

I will include a sample page as an attachment.

Comment 1

[Ruud]

Attachment: Frame page

Comment 2

[Ruud]

Attachment: Top navigator

Comment 3

[Ruud]

Attachment: Bottom navigator

Comment 4

[Ruud]

Attachment: Page containing the GoLive created popup menu/Java script that does not behave correctly in Mozilla latest built.

Comment 5

[Fabian Guisset]

Go Live is known not to create standards-compliant html and js. Are you sure
target="_self" is a valid and standards thingie?

Comment 6

[Ruud]

You are right that GoLive 4 uses non-standard html and javascript. I'm not 
an expert who can judge if this javascript is properly written, but this 
built-in GoLive popupmenu has always behaved as it should under older 
versions of Communicator and also under all versions of Explorer.

Comment 7

[Fabian Guisset]

Yes but older Netscape and IE use proprietary features that Mozilla will not
support, so it is not a good test.. However it would seem that target="_self" is
a w3c standard. See http://www.w3.org/TR/WD-layout.html under "Reserved TARGET
names".
"
target: _self 
     Loads the linked document into the same frame as the anchor that was
clicked on. This is useful for overriding an inherited target value. "

Probably something else is wrong with the code. Need to figure out what. Sending
to layout.

Comment 8

[Christian Mattar]

Confirming bug.
Changing URL since it's easier to see there (just wait 5 secs).
Changing to all/all.

Comment 9

[Brandon Hume]

Another URL which I believe displays this same problem is http://www.ncix.com/

The scrollboxes at the top fire off via JavaScript on select.  Under NN4 and
IE, the new page loads in the existing frame; under Mozilla a whole new browser
window is created instead.

Comment 10

[Phil Schwartau]

Setting default owner and QA Contact - 

Comment 11

[HJ]

This might be related to that patch for bug 56296 ??
I will try with a version without that patch, just to be sure!

Comment 12

[HJ]

Build 2001012820 work while build starting from 2001012920 doesn't work properly
any more! So it a regression for sure but I'm not 100% sure, for now, if that
patch has anything to do with this!

Comment 13

[HJ]

This must be related to the recent changes to nsDosShell.cpp (build 2001012908
Mtrunk also works correct).

Comment 14

[karnaze (gone)]

Adam, can you take a look based on the recent comments. 

Comment 15

[Adam Lock]

Attachment: Fix for _self targets

Comment 16

[Adam Lock]

A glance with LXR shows we don't handle _self anywhere. So I've written a small 
patch to the window watcher code that ensures that window.open with a "_self" 
target returns the current docshell. We may also need code to handle link clicks 
HTML tags with a "_self" target and this could involve changes to docshell.

Investigating further.

Comment 17

[Adam Lock]

Attachment: Sample html to test behaviour of anchors with "_self" targets and JS window.open(..., "_self")

Comment 18

[Johnny Stenback (:jst)]

Patch looks good to me, but I suggest you run this by Dan (who wrote this code)
before checking in. r=jst

Comment 19

[Dan M]

  Hmm. Theoretically this check would more properly go alongside the checks for 
"", "_blank" and "_content". The "_top" check in WindowWatcher started life as an 
anomaly. But come to think of it, this is a better place and I should clean up 
those other three.
  OK. Given that, I have a small correction. You'll be sending a null pointer to 
GetWindowTreeItem if aParent is null, and that would be bad. aParent is maybe 
misnamed; it's more like aCurrentWindowContextThatCouldBeTheParentIfYouLike. 
_self is nonsensical without a window context anyway, so the patch should go

 if (aParent) {
+  if (name.EqualsIgnoreCase("_self"))
+    GetWindowTreeItem(aParent, getter_AddRefs(newDocShellTreeItem));
   else if (name.EqualsIgnoreCase("_top")) {

Comment 20

[Dan M]

oh yeah. odd that we aren't checking for _self. i wonder when that was removed. 
r=me on my version of the patch!

Comment 21

[Adam Lock]

Thanks Johnny & Dan, I'll update the patch to follow the latest suggestions 
(i.e. move the code inside of the aParent test) before checking in.

Comment 22

[Adam Lock]

Attachment: Fix as it was checked in

Comment 23

[Adam Lock]

Fix checked in

Comment 24

[Chris Petersen]

Marking verified in the March 23rd build (2001032108).