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)
Firefox
General
Tracking
()
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)
636 bytes,
text/html
|
Details | |
6.17 KB,
patch
|
smaug
:
review-
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 2•20 years ago
|
||
(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.
Comment 3•20 years ago
|
||
*** 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.
Comment 5•20 years ago
|
||
*** Bug 267222 has been marked as a duplicate of this bug. ***
Comment 6•20 years ago
|
||
*** Bug 267222 has been marked as a duplicate of this bug. ***
*** Bug 270778 has been marked as a duplicate of this bug. ***
Comment 8•19 years ago
|
||
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
Comment 9•19 years ago
|
||
<shot in dark> Could this be the bug that "broke" the Linky extension? http://gemal.dk/mozilla/linky.html
Comment 10•19 years ago
|
||
since nothing's been written or checked in yet, I'd say no...
Comment 11•19 years ago
|
||
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."
Updated•19 years ago
|
Flags: blocking-aviary2+
Updated•18 years ago
|
Priority: -- → P3
Target Milestone: Firefox1.5 → Firefox 2 beta1
Comment 12•18 years ago
|
||
load-balancing tabbrowser stuff to sspitzer
Assignee: mconnor → sspitzer
Status: ASSIGNED → NEW
Comment 14•18 years ago
|
||
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.
Comment 15•18 years ago
|
||
not going to block at this now
Flags: blocking-firefox2+ → blocking-firefox2-
Target Milestone: Firefox 2 beta1 → ---
Comment 16•17 years ago
|
||
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
Comment 17•17 years ago
|
||
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
Updated•17 years ago
|
Flags: wanted-firefox3+
Comment 18•17 years ago
|
||
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 19•17 years ago
|
||
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 20•17 years ago
|
||
Comment on attachment 290773 [details] [diff] [review] eat click modifiers a=beltzner for 1.9
Attachment #290773 -
Flags: approval1.9? → approval1.9+
Comment 21•17 years ago
|
||
fixed, long overdue
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 22•17 years ago
|
||
I think changing the behavior for data: URLs was the wrong thing to do.
Comment 23•17 years ago
|
||
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
Comment 24•17 years ago
|
||
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).
Comment 26•17 years ago
|
||
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.
Comment 27•17 years ago
|
||
Adding a test file to simplify things and because the referenced pages change over time.
Comment 28•17 years ago
|
||
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]
Updated•17 years ago
|
Comment 29•17 years ago
|
||
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 )
Updated•16 years ago
|
Flags: blocking-firefox3?
Whiteboard: [SWAG: 2d] [parity-safari] [parity-IE] → [SWAG: 2d] [parity-safari] [parity-IE] [has wrong patch]
Comment 30•16 years ago
|
||
This does not block the final release of Firefox 3.
Flags: blocking-firefox3? → blocking-firefox3-
Comment 31•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #302005 -
Flags: review?(gavin.sharp) → review+
Updated•16 years ago
|
Attachment #302005 -
Flags: approval1.9?
Comment 33•16 years ago
|
||
Dao why do we want to backout?
Comment 34•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #302005 -
Flags: approval1.9? → approval1.9+
Updated•16 years ago
|
Keywords: checkin-needed
Updated•16 years ago
|
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
Comment 35•16 years ago
|
||
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
Updated•16 years ago
|
Whiteboard: [SWAG: 2d] [parity-safari] [parity-IE] [has wrong patch] → [not needed for 1.9][SWAG: 2d][parity-safari][parity-IE][has wrong patch]
Updated•16 years ago
|
Assignee: mconnor → nobody
Status: REOPENED → NEW
Comment 37•15 years ago
|
||
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)
Updated•15 years ago
|
Whiteboard: [not needed for 1.9][SWAG: 2d][parity-safari][parity-IE][has wrong patch] → [SWAG: 2d][parity-safari][parity-IE]
Comment 38•15 years ago
|
||
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-
Comment 39•15 years ago
|
||
(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.
Comment 40•15 years ago
|
||
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.
Comment 41•15 years ago
|
||
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 42•14 years ago
|
||
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-
Updated•14 years ago
|
Attachment #406043 -
Flags: review?(gavin.sharp)
Updated•14 years ago
|
Assignee: dao → nobody
Component: Tabbed Browser → General
QA Contact: tabbed.browser → general
Comment 43•14 years ago
|
||
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
Comment 44•11 years ago
|
||
I don't think this is tracking anything useful separate from bug 55696 now.
Status: NEW → RESOLVED
Closed: 17 years ago → 11 years ago
Resolution: --- → DUPLICATE
Comment 45•11 years ago
|
||
(and bug 251137)
Comment 46•11 years ago
|
||
(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.
Description
•