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)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla0.9
People
(Reporter: fosterd, Assigned: bugzilla)
Details
(Keywords: regression)
Attachments
(13 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•24 years ago
|
||
OK, I'm now seeing that using button2 to open a link in a new window no longer
works either.
Keywords: regression
Comment 2•24 years ago
|
||
Dang! ->pavlov, rtm/M18
Assignee | ||
Comment 3•24 years ago
|
||
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
Assignee | ||
Comment 4•24 years ago
|
||
Any console errors?
Assignee | ||
Comment 5•24 years ago
|
||
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
Reporter | ||
Comment 6•24 years ago
|
||
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.
Assignee | ||
Comment 7•24 years ago
|
||
Assignee | ||
Comment 8•24 years ago
|
||
attached a patch for review / approval. comments welcome, but no additional
whitespace changes please (shaver already has a patch to whack this file).
Assignee | ||
Comment 9•24 years ago
|
||
Assignee | ||
Comment 10•24 years ago
|
||
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.
Assignee | ||
Comment 11•24 years ago
|
||
Assignee | ||
Comment 12•24 years ago
|
||
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...
Assignee | ||
Comment 13•24 years ago
|
||
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)
Reporter | ||
Comment 14•24 years ago
|
||
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.
Comment 15•24 years ago
|
||
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?
Assignee | ||
Comment 16•24 years ago
|
||
Right, my bad.
Assignee | ||
Comment 17•24 years ago
|
||
Comment 18•24 years ago
|
||
this patch seems fine.
r=ben@netscape.com
Comment 19•24 years ago
|
||
+ 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
Assignee | ||
Comment 20•24 years ago
|
||
Assignee | ||
Comment 21•24 years ago
|
||
Brendan, how's this latest patch?
doh..the left-button comment is off by a line handleLinkClick(). I'll fix that.
Comment 22•24 years ago
|
||
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;
Assignee | ||
Comment 23•24 years ago
|
||
What? I didn't remove any returns.
Assignee | ||
Comment 24•24 years ago
|
||
Comment 25•24 years ago
|
||
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
Assignee | ||
Comment 26•24 years ago
|
||
Comment 27•24 years ago
|
||
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
Assignee | ||
Comment 28•24 years ago
|
||
Comment 29•24 years ago
|
||
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
Comment 30•24 years ago
|
||
*** Bug 58700 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 31•24 years ago
|
||
But then what happens when someone adds the img and #test cases later on? Just
readd the findParentNode() call to each case?
Assignee | ||
Comment 32•24 years ago
|
||
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>?
Comment 33•24 years ago
|
||
Why would anyone add the #text and img cases later, when they're already
subsumed by the default: case?
/be
Comment 34•24 years ago
|
||
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
Assignee | ||
Comment 35•24 years ago
|
||
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.
Comment 36•24 years ago
|
||
>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
Comment 37•24 years ago
|
||
I wrote "and a reason to do with your patch #3963" but meant "to go with ...".
/be
Assignee | ||
Comment 38•24 years ago
|
||
Assignee | ||
Comment 39•24 years ago
|
||
Comment 40•24 years ago
|
||
Does it make you feel better to know I will *LOVE* having the middle-click paste
back? :)
Assignee | ||
Comment 41•24 years ago
|
||
Comment 42•24 years ago
|
||
> 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.
Comment 43•24 years ago
|
||
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
Assignee | ||
Comment 44•24 years ago
|
||
Assignee | ||
Comment 45•24 years ago
|
||
Comment 46•24 years ago
|
||
sr=brendan@mozilla.org.
Does ben's r= stand? I think so, although he missed some stuff!
/be
Assignee | ||
Comment 47•24 years ago
|
||
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.)
Assignee | ||
Comment 48•24 years ago
|
||
Fix checked in...back to civilization I go.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 49•24 years ago
|
||
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.
Assignee | ||
Comment 50•24 years ago
|
||
Yes, I just realized this typo in my patch, and it has been fixed.
Comment 51•24 years ago
|
||
Would now be a good time to mention XLinks and the HTML <link> element?
Assignee | ||
Comment 52•24 years ago
|
||
What?
Comment 53•24 years ago
|
||
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.
Description
•