627065 - "Copy Link Location" doesn't work on links on pages open in the sidebar

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Honza Bambas (:mayhemer)]

The link address is not copied to clipboard.

No idea of a regression range but I first noted that a long ago, just didn't get to report it.

Comment 1

[aravindm]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre

Able to reproduce. Copy link location does not work when loaded the web pages in windows sidebar. 
Works fine in Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.14pre) Gecko/20110119 Namoroka/3.6.14pre

Comment 2

[aravindm]

works: 
Mozilla/5.0 (Windows NT 6.1; rv:2.0b4pre) Gecko/20100809 Minefield/4.0b4pre

broken: 
Mozilla/5.0 (Windows NT 6.1; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre

Pushlog : 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fb0c72c4bfb3
&tochange=905529619219

Comment 3

[Honza Bambas (:mayhemer)]

Aravind, if this is simple to fix, we should get it to Fx4 as it is a regression from 3.6.

Comment 4

[Dietrich Ayala (:dietrich)]

Regression -> softblocker.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Proposal patch

I'm not sure whether this is best fix but works fine for me.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=ebc8dbbe674b

Comment 6

[Dão Gottwald [:dao]]

What regressed this?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, the previous push includes another patch.
http://tbpl.mozilla.org/?tree=MozillaTry&rev=fc17e9e87493

(In reply to comment #6)
> What regressed this?

I can see this error.
> Error: An error occurred executing the cmd_copyLink command: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIController.isCommandEnabled]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 95"  data: no]
> Source File: chrome://global/content/globalOverlay.js
> Line: 100

And only the copy link location uses goDoCommand() rather than gContextMenu.

Comment 8

[Dão Gottwald [:dao]]

Right, but which changeset caused this regression?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#3290
> 3290             node = pm->GetLastTriggerPopupNode(rootDoc);

this result is null, this is the cause of NS_ERROR_FAILURE.

I suspect http://hg.mozilla.org/mozilla-central/rev/ed8906789d08

Comment 10

[[e-mail]]

[Seems to work OK in SeaMonkey's sidebar. Don't know what would be different.]

Comment 11

[Neil Deakin]

Comment on attachment 509440 [details] [diff] [review]
Proposal patch

We should fix the real probelem here.

Comment 12

[Neil Deakin]

The sidebar is in a child frame and has its own context menu implementation. The document viewer commands assume that the context menu is defined in the toplevel window.

Note that this works in Seamonkey's sidebar as it just uses an overlay and one context menu.

Comment 13

[Neil Deakin]

Attachment: fix

I don't really like setting popupNode, but this should be ok for now.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm, once you set popupNode it stops returning the last opened popup till you clear it. I expect this change to break most of Places context menu items.

Comment 15

[Neil Deakin]

If the context menu has a non-null triggerNode, popupNode will be cleared when the context menu is closed.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

And what happens in the other case? We rely on popupNode being the last open popup, if this change hits any code path that doesn't clear it, we have broken a bunch of menuitems to save one.
I think we should not rely on setting popupNode and fight to make it read-only asap (at a maximum allow to set it to null), it's really easy to break stuff by setting it.

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> And what happens in the other case?

What other cases?

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #17)
> What other cases?

I was mostly worried about add-ons preventing popupshowing or adding something that could throw during the event propagation. In those cases the menu will be hosed in the sidebar, that was looking like a larger cost than the benefit.
I still think we should make popupNode a readonly attribute, as is triggerNode.

Comment 19

[Dão Gottwald [:dao]]

(In reply to comment #18)
> (In reply to comment #17)
> > What other cases?
> 
> I was mostly worried about add-ons preventing popupshowing

I guess the patch should reset document.popupNode to null for gContextMenu.shouldDisplay == false.

Comment 20

[[e-mail]]

Comment on attachment 509769 [details] [diff] [review]
fix

Shouldn't you set the popupNode after you've determined that it's the popup itself that's showing and not one of its submenus?

Comment 21

[Neil Deakin]

(In reply to comment #19)
> I guess the patch should reset document.popupNode to null for
> gContextMenu.shouldDisplay == false.

I'll look into this.


(In reply to comment #20)
> Shouldn't you set the popupNode after you've determined that it's the popup
> itself that's showing and not one of its submenus?

That shouldn't affect the outcome but I'll make that change any way.

Comment 22

[Neil Deakin]

Attachment: patch

Like so. Also includes test.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 510329 [details] [diff] [review]
patch

>+  EventUtils.synthesizeMouse(link, 5, 5, { type: "contextmenu", button: 2 }, browser.contentWindow);

Use synthesizeMouseAtCenter?

Comment 24

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/8e716006f397

Comment 25

[Teodosia Pop]

Verified using Mozilla/5.0 (Windows NT 6.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12.