58333 - Middle mouse button paste of URL no longer works

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Decklin Foster]

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.

Comment 1

[Decklin Foster]

OK, I'm now seeing that using button2 to open a link in a new window no longer
works either.

Comment 2

[Peter Trudelle]

Dang!  ->pavlov, rtm/M18

Comment 3

[Blake Ross]

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?

Comment 4

[Blake Ross]

Any console errors?

Comment 5

[Blake Ross]

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...

Comment 6

[Decklin Foster]

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.

Comment 7

[Blake Ross]

Attachment: patch, also fixes some other things, and general cleanup

Comment 8

[Blake Ross]

attached a patch for review / approval.  comments welcome, but no additional 
whitespace changes please (shaver already has a patch to whack this file).

Comment 9

[Blake Ross]

Attachment: patch #2 - oops. textfield should use originalTarget in case of anonymous content

Comment 10

[Blake Ross]

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.

Comment 11

[Blake Ross]

Attachment: new patch

Comment 12

[Blake Ross]

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...

Comment 13

[Blake Ross]

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)

Comment 14

[Decklin Foster]

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

[John Morrison]

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?

Comment 16

[Blake Ross]

Right, my bad.

Comment 17

[Blake Ross]

Attachment: patch - please review

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

this patch seems fine. 
r=[e-mail]

Comment 19

[Brendan Eich [:brendan]]

+      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

Comment 20

[Blake Ross]

Attachment: patch with suggestions

Comment 21

[Blake Ross]

Brendan, how's this latest patch?

doh..the left-button comment is off by a line handleLinkClick().  I'll fix that.

Comment 22

[Brendan Eich [:brendan]]

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;

Comment 23

[Blake Ross]

What?  I didn't remove any returns.

Comment 24

[Blake Ross]

Attachment: er...i apparently did.

Comment 25

[Brendan Eich [:brendan]]

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

Comment 26

[Blake Ross]

Attachment: patch #3962

Comment 27

[Brendan Eich [:brendan]]

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=[e-mail].

/be

Comment 28

[Blake Ross]

Attachment: patch #3963 (for reference)

Comment 29

[Brendan Eich [:brendan]]

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

[sairuh (rarely reading bugmail)]

*** Bug 58700 has been marked as a duplicate of this bug. ***

Comment 31

[Blake Ross]

But then what happens when someone adds the img and #test cases later on?  Just 
readd the findParentNode() call to each case?

Comment 32

[Blake Ross]

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

[Brendan Eich [:brendan]]

Why would anyone add the #text and img cases later, when they're already 
subsumed by the default: case?

/be

Comment 34

[Brendan Eich [:brendan]]

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

Comment 35

[Blake Ross]

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

[Brendan Eich [:brendan]]

>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

[Brendan Eich [:brendan]]

I wrote "and a reason to do with your patch #3963" but meant "to go with ...".

/be

Comment 38

[Blake Ross]

Attachment: patch #50000! <sob>

Comment 39

[Blake Ross]

Attachment: patch 50000 - if I never see this again, I will be a happy man

Comment 40

[sarnold]

Does it make you feel better to know I will *LOVE* having the middle-click paste
back? :)

Comment 41

[Blake Ross]

Attachment: patch - HELP ME!

Comment 42

[John Morrison]

> 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

[Brendan Eich [:brendan]]

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

Comment 44

[Blake Ross]

Attachment: Day 14: I have been reduced to the broken shell of a patch-making machine.

Comment 45

[Blake Ross]

Attachment: Day 30: today i drank tea with a pleasant little monkey who identified himself as `billy'

Comment 46

[Brendan Eich [:brendan]]

sr=[e-mail].

Does ben's r= stand?  I think so, although he missed some stuff!

/be

Comment 47

[Blake Ross]

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.)

Comment 48

[Blake Ross]

Fix checked in...back to civilization I go.

Comment 49

[Decklin Foster]

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.

Comment 50

[Blake Ross]

Yes, I just realized this typo in my patch, and it has been fixed.

Comment 51

[Hixie (not reading bugmail)]

Would now be a good time to mention XLinks and the HTML <link> element?

Comment 52

[Blake Ross]

What?

Comment 53

[sarnold]

Blake, thank you for your efforts with this patch. :) It pleases me greatly.
Good luck with the sanity. :)

:)