Closed Bug 251137 Opened 20 years ago Closed 11 years ago

[Ctrl|Shift]+Click on JavaScript links open empty window, not requested page

Categories

(Firefox :: General, defect, P3)

defect

Tracking

()

RESOLVED DUPLICATE of bug 55696

People

(Reporter: ali, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: testcase, ue, Whiteboard: [SWAG: 2d][parity-safari][parity-IE])

Attachments

(2 files, 3 obsolete files)

On http://news.bbc.co.uk/ , left-click the image that says "OPEN MORE
AUDIO/VIDEO NEWS". You can see it opens up a requested popup window.

But what if you middle-click or ctrl+click on it? A new tab is opened, but it is
empty. If you shift+click, a new window is opened, but it is empty. We should
fix this.

I've been told that Safari handles this case well, we should try and get parity.
See Bug 126862 and Bug 138198. Dupe of the latter one?
(In reply to comment #1)
> See Bug 126862 and Bug 138198. Dupe of the latter one?

This are both browser (core) bugs. I'm wondering if we actually fork this. I'm
not sure if this falls strictly under the browser product.

Since mconnor asked for this to be assigned to him when I file it, I'll leave
the decision up to him as to whether or not we dupe this against browser bugs.
*** Bug 252353 has been marked as a duplicate of this bug. ***
It's certainly the same code problem in the Suite (bug 138198) and in Firefox
(this bug). I suggest we leave both bug reports open in the hopes that between
them fewer duplicates will be filed.
Depends on: 138198
*** Bug 267222 has been marked as a duplicate of this bug. ***
*** Bug 267222 has been marked as a duplicate of this bug. ***
*** Bug 270778 has been marked as a duplicate of this bug. ***
Not hard to fix, its a trade of one expected result for another, but since this
actually stands a chance of something useful happening, I'll take the tradeoff.
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox1.1
Version: unspecified → Trunk
<shot in dark> Could this be the bug that "broke" the Linky extension?

http://gemal.dk/mozilla/linky.html
since nothing's been written or checked in yet, I'd say no...
It's been a long time, but anyway, I've found an extension that does the job. Maybe you could take a look : http://dmextension.mozdev.org/misc.html

"By default, middle clicking on javascript links in Firefox opens a blank tab instead of following the link. This extension fixes that problem and lets middle clicks open simple javascript links in new tabs. It is not very sophisticated, so some javascript links may not work, but most will. Why this simple functionality is not built into Firefox by default, I don’t know."
Flags: blocking-aviary2+
Priority: -- → P3
Target Milestone: Firefox1.5 → Firefox 2 beta1
load-balancing tabbrowser stuff to sspitzer
Assignee: mconnor → sspitzer
Status: ASSIGNED → NEW
gah.  s/.org/.com/!
Assignee: sspitzer → sspitzer
there is an old core bug on this, see bug #55696

jesse and ian agreed that comment 23 in bug #55696 was the way to go about doing this.

I think the fix will be to contentAreaClick (as it is with the previously mentioned "Smart Middle Click" extension, see http://lxr.mozilla.org/mozilla1.8/source/browser/base/content/browser.js#5258

but note, from contentAreaClick():

5258          // javascript links should be executed in the current browser
5259          if (wrapper.href.substr(0, 11) === "javascript:")
5260            return true;

that change comes from a security fix from 1.0.2, see bug #284627

so giving myself two days to do this right and not open any security holes.
Status: NEW → ASSIGNED
Depends on: 55696
Whiteboard: [SWAG: 2d]
not going to block at this now
Flags: blocking-firefox2+ → blocking-firefox2-
Target Milestone: Firefox 2 beta1 → ---
sorry for the bug spam, re-assigning bugs back to default owner if I'm not working actively on them.
Assignee: sspitzer → nobody
Status: ASSIGNED → NEW
Attached patch eat click modifiers (obsolete) — Splinter Review
this seems to do the trick, and basically moves the sec fix for sidebar/search pane XSS to be a general policy for clicks on data: and javascript:

tested and seems to work quite sanely, though its perhaps not ideal, it should be good enough to ship with
Assignee: nobody → mconnor
Status: NEW → ASSIGNED
Attachment #290773 - Flags: review?(gavin.sharp)
Flags: wanted-firefox3+
Comment on attachment 290773 [details] [diff] [review]
eat click modifiers

Is there a reason we're not using nsIURI.scheme for this? Performance? I suppose it doesn't matter too much...
Attachment #290773 - Flags: review?(gavin.sharp) → review+
Comment on attachment 290773 [details] [diff] [review]
eat click modifiers

I don't think its perf, I think this is just old code that should be converted at some point, I'll file a followup.
Attachment #290773 - Flags: approval1.9?
Comment on attachment 290773 [details] [diff] [review]
eat click modifiers

a=beltzner for 1.9
Attachment #290773 - Flags: approval1.9? → approval1.9+
fixed, long overdue
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
I think changing the behavior for data: URLs was the wrong thing to do.
Hmmm... The expected behavior is opening the javascript window in a new tab, right ? Because right now, on the page I tested it, it just does nothing.

Try the links there: http://stifu.free.fr/en/emu-ngpcroms.php
Another example, just for the hell of it: http://www.samuraispirits.net/cgi-bin/ikonboard/ikonboard.cgi

Middle clicking on "Help" at the top does nothing.
I filed bug 409062 suggesting reverting the change for data: URLs (I agree with comment 22).
I'd hardly call this fixed. True, we no longer open the empty tab mentioned in the first half of the summary, but we are not opening the requested page from the second half of the summary. We do nothing at all.

The IE7 and Safari both go ahead and run the javascript in the current page. Opera opens a blank tab as we did before this fix.

I also agree data: is a different beast and should not have been changed.
Attached file test links
Adding a test file to simplify things and because the referenced pages change over time.
Depends on: 409062
It wouldn't really make sense to file a new bug for having stopped executing the script, because this bug's summary already covers this (open requested page vs. do nothing), so reopening based on comment 26.
Status: RESOLVED → REOPENED
Keywords: ue
Resolution: FIXED → ---
Whiteboard: [SWAG: 2d] → [SWAG: 2d] [parity-safari] [parity-IE]
No longer depends on: 55696, 138198
Gavin asked about removing http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsGenericElement.cpp&rev=3.617&mark=4349-4352#4347
so that fixing this could be easier.
I went through click-event-on-link-history until bug 12056 where that code 
was added. As far as I see, the code did make sense at that time, when
stack based event target chain / event dispatch was used and default event handling for links happened during bubble phase on default event group.
To be able to handle clicks in chrome, default event handling had to be
disabled because default handling would have occurred before chrome handling.

But on 1.9 default handling for events is different. It is always
on at_target/bubble phase of system event group, when PostHandleEvent is called.
So if there is an event listener added to bubble phase of default group,
it gets called before default handling (unless someone calls 
.stopPropagation(), not sure what we want to do in that case).
So as far as I see, the code could be removed.


(Currently event handling goes like this:
  - if event was dispatched via presshell,
    nsIEventStateManager::PreHandleEvent
  - nsEventDispatcher::Dispatch
    - create event target chain by calling PreHandleEvent
    - dispatch event to the chain
    - handle event in capture phase / default group
    - handle event at target / default group
    - handle event in bubble phase / default group
    - if event was dispatched via PresShell, possibly nsIFrame::HandleEvent
    - handle event in capture phase / system group
    - handle event at target / system group
    - handle event at target, default handling / PostHandleEvent
    - handle event in bubble phase / system group    
    - handle event in bubble phase, default handling / PostHandleEvent
  - if event was dispatched via presshell, 
     nsIEventStateManager::PostHandleEvent
)

Flags: blocking-firefox3?
Whiteboard: [SWAG: 2d] [parity-safari] [parity-IE] → [SWAG: 2d] [parity-safari] [parity-IE] [has wrong patch]
Keywords: testcase
This does not block the final release of Firefox 3.
Flags: blocking-firefox3? → blocking-firefox3-
Attached patch backout (obsolete) — Splinter Review
Attachment #302005 - Flags: review?(gavin.sharp)
Note that the changes described in bug 335963 should fix this so things behave "as expected" or make it easy to do so.
Attachment #302005 - Flags: review?(gavin.sharp) → review+
Attachment #302005 - Flags: approval1.9?
Depends on: 335963
Dao why do we want to backout?
The patch didn't do what it was supposed to do (it makes [Ctrl|Shift]+Click on Javascript links do nothing rather than open the requested page) and it caused bug 409062.
Attachment #302005 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Summary: [Ctrl|Shift]+Click on Javascript links open empty window, not requested page → [Ctrl|Shift]+Click on JavaScript links open empty window, not requested page
Backed out.

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.961; previous revision: 1.960
done
Keywords: checkin-needed
Whiteboard: [SWAG: 2d] [parity-safari] [parity-IE] [has wrong patch] → [not needed for 1.9][SWAG: 2d][parity-safari][parity-IE][has wrong patch]
Assignee: mconnor → nobody
Status: REOPENED → NEW
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Attachment #290773 - Attachment is obsolete: true
Attachment #302005 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #405795 - Flags: review?(gavin.sharp)
Attachment #405795 - Flags: review?(Olli.Pettay)
Whiteboard: [not needed for 1.9][SWAG: 2d][parity-safari][parity-IE][has wrong patch] → [SWAG: 2d][parity-safari][parity-IE]
Comment on attachment 405795 [details] [diff] [review]
patch

>-    if (NS_IS_MOUSE_LEFT_CLICK(aVisitor.mEvent)) {
>-      nsInputEvent* inputEvent = static_cast<nsInputEvent*>(aVisitor.mEvent);
>-      if (inputEvent->isControl || inputEvent->isMeta ||
>-          inputEvent->isAlt ||inputEvent->isShift) {
>-        break;
>-      }
>-
So how does the patch work now. Do all the left click events on a link
cause DOMActivate?


>+        aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault) {
Why do you need this check here?
CheckHandleEventForLinksPrecondition should do it already.


The patch needs tests.
Attachment #405795 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #38)
> So how does the patch work now. Do all the left click events on a link
> cause DOMActivate?

Only those that haven't been consumed already.

> >+        aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault) {
> Why do you need this check here?

Because that code processed events even if contentAreaClick had already done so.

> CheckHandleEventForLinksPrecondition should do it already.

Well, see above.
It's quite possible that the first version I tested didn't have these fixes:

-       handleLinkClick(event, wrapper.href, linkNode);
+       return !handleLinkClick(event, wrapper.href, linkNode);

That would probably explain what I was seeing.
Attached patch patch v2Splinter Review
removed redundant aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault
Attachment #405795 - Attachment is obsolete: true
Attachment #406043 - Flags: review?(gavin.sharp)
Attachment #406043 - Flags: review?(Olli.Pettay)
Attachment #405795 - Flags: review?(gavin.sharp)
Comment on attachment 406043 [details] [diff] [review]
patch v2

Hmm, this breaks, or at least changes, link handling in embedding.
Attachment #406043 - Flags: review?(Olli.Pettay) → review-
Attachment #406043 - Flags: review?(gavin.sharp)
Assignee: dao → nobody
Component: Tabbed Browser → General
QA Contact: tabbed.browser → general
Blocks: 565621
This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.
Status: ASSIGNED → NEW
I don't think this is tracking anything useful separate from bug 55696 now.
Status: NEW → RESOLVED
Closed: 17 years ago11 years ago
Resolution: --- → DUPLICATE
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #45)
> (and bug 251137)

I meant bug 672618.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: