Closed Bug 58333 Opened 24 years ago Closed 24 years ago

Middle mouse button paste of URL no longer works

Categories

(Core :: XUL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9

People

(Reporter: fosterd, Assigned: bugzilla)

Details

(Keywords: regression)

Attachments

(13 files)

Build: trunk checked out at 108010 Using the middle mouse button to paste a URL into the browser area doesn't do anything. This is a recent regression (sometime this morning). I've checked to see if the default prefs or my prefs have changed, but looks like they haven't.
OK, I'm now seeing that using button2 to open a link in a new window no longer works either.
Keywords: regression
Dang! ->pavlov, rtm/M18
Assignee: trudelle → pavlov
Keywords: rtm
Target Milestone: --- → M18
Was this before or after my modifier+click checkin? I just touched lots of stuff related to this, so reassigning to me. But please see if this is in the branch, I can't imagine that it is. In any case, why would this be an rtm stop- ship?
Assignee: pavlov → blakeross
Any console errors?
OK, I see what's wrong. My patch assumed that browserHandleMiddleClick() was only necessary if you're on a link -- I forgot that it needs to take into account pasting also. Attaching a fix...
Status: NEW → ASSIGNED
Here's what I see on the console: JavaScript error: line 0: linkClick is not defined I had the previous revision of navigator.js when this first broke, so this may not be you.
attached a patch for review / approval. comments welcome, but no additional whitespace changes please (shaver already has a patch to whack this file).
Keywords: rtmapproval, patch, review
OS: Linux → All
Priority: P3 → P1
Hardware: PC → All
Target Milestone: M18 → mozilla0.9
decklin just explained to me how unix paste should work -- pasting should be allowed anywhere except on a link. Right now my patch just allows it for textfields, which is how I thought it was supposed to work (oh, and input type="textfield" is obviously a mistake in my patch). I'll attach a new patch soon.
Attached patch new patch (deleted) — Splinter Review
attached a new patch that works, per decklin's testing. but warning: this code is u-g-l-y with all the |break|s and stuff. I can't think of the most efficient way to paste the URL if anything but a link is clicked on. I don't want to iterate over nodes to see if it's a link every time the user clicks anywhere, so I can't just put the check at the beginning. And if I put the check at the end, I'll return too early unless I do this working but ugly set of breaks. I'm hoping Brendan/someone can help me out with a better program flow here...
I suppose we could just always return true. I suspect that will be the most- wanted case, since returning false means the click is just canceled. But I'd like to keep that option open... (I have a new patch that also makes sure you're trying to load the URL if the scrollbar is clicked on, and does s/handleMiddleMousePaste ()/middleMouseButtonPaste(), but I'll attach it after we figure out this other stuff)
This patch does indeed fix my problems. Blake: if you're worried about the code, maybe you could look at older revisions of navigator.js? I don't remember the exact way it worked previously, but it was somewhat simpler.
Just to confirm -- in the _branch_, linux build, middle-mouse on a link loads that link in a new window, middle-mouse in content area pastes/loads that url/text into the content area, middle-mouse in urlbar pastes that url/text into the url bar but does not load it, and middle-mouse on scrollbar moves the thumb to that location but does not paste/load the text/url. > I have a new patch that also makes sure you're trying to load the URL if > the scrollbar is clicked on, You mean "you're *not* trying to load the URL ...", right?
Right, my bad.
Attached patch patch - please review (deleted) — Splinter Review
+ case "a": + linkNode = handleLinkClick(event, event.target); + break; Oops: this case statement should be + case "a": + linkNode = event.target; + break; Also, use a switch on event.button rather than an if/else-if in handleLinkClick. How about a new patch? /be
Attached patch patch with suggestions (deleted) — Splinter Review
Brendan, how's this latest patch? doh..the left-button comment is off by a line handleLinkClick(). I'll fix that.
Do you really want to fall through from the first if's then part into the if (event.shiftKey) then save-page logic? When transforming the code to use a switch (which is faster than if-else for small, dense int-valued case labels) the returns you had before should have remained, I think. + case 1: + if (event.metaKey || event.ctrlKey) { // and meta or ctrl are down + openNewWindowWith(node.href); // open link in new window + event.preventBubble(); + } + if (event.shiftKey) // if shift is down + return savePage(node.href); // save the link + if (event.altKey) // if alt is down + ; // select text within link (not implemented) + break;
What? I didn't remove any returns.
Attached patch er...i apparently did. (deleted) — Splinter Review
Shave a few more JS bytecode cycles (tens or hundreds of machine cycles): + var linkNode = null; + var rv = true; are needlessly initialized. linkNode, if not set, defaults to undefined, which converts to false. rv is set in all cases before it is used, later on in this function. Otherwise, looks good. Show me a final patch and I'll sr=. /be
Attached patch patch #3962 (deleted) — Splinter Review
Sorry I didn't point this out sooner: the "#text" and "img" case statements are identical here: + case "#text": + linkNode = findParentNode(target, "a"); + break; + case "a": + linkNode = event.target; + break; + case "img": + linkNode = findParentNode(target, "a"); + break; so fuse them: + case "#text": + case "img": + linkNode = findParentNode(target, "a"); + break; + case "a": + linkNode = event.target; + break; Do that, and sr=brendan@mozilla.org. /be
Attached patch patch #3963 (for reference) (deleted) — Splinter Review
Now that I think about it, shouldn't the default: case subsume #text and img, so that we have: + switch (target.localName.toLowerCase()) { + case "a": + linkNode = event.target; + break; + case "area": + if (event.target.href) + linkNode = event.target; + break; + default: + linkNode = findParentNode(target, "a"); + break; + } Otherwise I'm concerned that we won't cover all cases that could be wrapped by an <a href=> </a> tag. One final patch, please. /be
*** Bug 58700 has been marked as a duplicate of this bug. ***
But then what happens when someone adds the img and #test cases later on? Just readd the findParentNode() call to each case?
Also, in what cases would we miss a link? What else can be linked besides text and images (and links and <area>'s)? And do we really want to look for an <a> parent every time the user clicks on anything that isn't a link or an <area>?
Why would anyone add the #text and img cases later, when they're already subsumed by the default: case? /be
How about <br>, is that #text? <hr>? I'm not sure of other elements, and being exhaustive in the switch may be better, but if the ancestor line is generally not too long, then the cost of a default: case that searches it for an "a" tag may be worth the code consolidation and generality. Someone school me on what elements can be in an <a> tag, per the standards. /be
Because someone might need to distinguish between clicking on text and clicking on an image... I'll make this change if you really want, but I'm trying to make this function as general and scalable as possible should it be used to handle many more nodes in the future.
>Because someone might need to distinguish between clicking on text and clicking >on an image... If we go with the default: case, then "someone" has to do the right thing, and split cases out of it later. Cross that bridge when you come to it, I say. >I'll make this change if you really want, but I'm trying to make this function >as general The default approach is more general. Can you try an <hr> in an <a> tag? How about a <br>, does that result in a clickable region? Hmm. >and scalable as possible should it be used to handle many more nodes >in the future. Scalability usually refers to performance, so the ancestor line search could cost more in a deep doc, if you click on something not wrapped in an "a". That is a downside, and a reason to do with your patch #3963. I'm happy to sr= that if we can get an authoritative reading of the standards (and not from me!) about what elements are valid inside <a href=> </a>. /be
I wrote "and a reason to do with your patch #3963" but meant "to go with ...". /be
Attached patch patch #50000! <sob> (deleted) — Splinter Review
Does it make you feel better to know I will *LOVE* having the middle-click paste back? :)
Attached patch patch - HELP ME! (deleted) — Splinter Review
> Someone school me on what elements can be in an <a> tag, per the standards. Per spec, any inline element can be inside an <A HREF></A>, which is pretty much anything that is not a block level element. http://www.w3.org/TR/html401/struct/links.html#edef-A <!ELEMENT A - - (%inline;)* -(A) -- anchor --> (In practice (the real world), anything.) > How about <br>, is that #text? <hr>? No, those are elements in their own right, not #text.
Blake, dude, what editor do you use? It seems to make it too easy to delete lines unintentionally. Last time it was returns, now it's a crucial break: + case "area": + if (event.target.href) + linkNode = event.target; + default: + linkNode = findParentNode(target, "a"); + break; Right before the default:, there should be a break; -- fix that and don't change another thing, and I'll sr=, I swear! /be
sr=brendan@mozilla.org. Does ben's r= stand? I think so, although he missed some stuff! /be
YES!!! YES!! A SUPERREVIEW!! /me does the superreview happy dance WHEEEEEEEEEE ok. so. ben, please r= one more time. thanks. (needless to say, if you have a problem with the patch, you - er - won't quite be fully intact tomorrow.)
Fix checked in...back to civilization I go.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
This broke forms, all that needs to be changed is @@ -1598,7 +1598,7 @@ return middleMousePaste(event); } - return false; + return true; } function handleLinkClick(event, node) Otherwise it is fixed.
Yes, I just realized this typo in my patch, and it has been fixed.
Would now be a good time to mention XLinks and the HTML <link> element?
What?
Blake, thank you for your efforts with this patch. :) It pleases me greatly. Good luck with the sanity. :) :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: