383519 - control-click on bookmark bar folder item is handled differently than right-mouse-click

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect, P4)

Description

[John Daggett (:jtd)]

control-click on bookmark bar folder item is handled differently than right-mouse-click

Steps:

1. Add folder to the bookmark bar
2. Drag a URL into that folder
3. Click on the folder on the bookmark bar; popup menu appears
4. Control-click on the one item in the menu
==> handled as a click event, URL is loaded
5. Click on the folder again
6. Right-mouse-click on the item
==> nothing happens!

With FF 2.0, both are handled as click events.  Safari 2.0.4 has the same behavior as FF 2.0.  Also, right-clicking -or- control-clicking the home button on the latest trunk shows a contextual menu, so the problem is specific to code handling events in the bookmark bar.

Comment 1

[Steve Won (stevewon on irc)]

Attachment: Patch

Patch.

Comment 2

[Steve Won (stevewon on irc)]

Comment on attachment 271749 [details] [diff] [review]
Patch

Found a problem in the Patch. Sorry.

Comment 3

[Steve Won (stevewon on irc)]

Attachment: Patch

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This patch has bitrotted a long time. Anyone who can review the latest patch? 

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 271878 [details] [diff] [review]
Patch

since it's bitrotted i'm marking as obsolete and clearing review...

right mouse click here opens the context menu for the item, i don't think it should load anything... Am i loosing something?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marco the context menu only opens for the items which are directly located at the bookmarks toolbar. For sub items the context menu doesn't open. There is nothing happen on latest trunk. Fx2 opens the clicked bookmark within the current tab. 

Comment 7

[Marco Bonardo [:mak]]

Attachment: un-bitrotted fixed patch

i suppose this is for mac os x only... this is un bitrotted, but i cannot test functionality since i don't have a mac, all i can say is that is not causing problems in Win

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 292642 [details] [diff] [review]
un-bitrotted fixed patch

r=me, thanks marco

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I tested this patch and now the bookmark is opened within the current tab. But I get an js error:

Error: popupNode is null
Source File: chrome://browser/content/places/toolbar.xml
Line: 1038

There is still another glitch. If I open the context menu of the top folder e.g. "Latest Headlines" first, this patch doesn't work anymore for each saved bookmark. I have to restart Firefox to be able to use the right click to open the website.

Comment 10

[Marco Bonardo [:mak]]

Attachment: make it only for menu and menuitem

henrik could you try this for both problems?

try also the bookmark menu since it should be served by the same code and should not break... i cannot do much more without a Mac to test on...

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marco, it doesn't change anything and I cannot see a difference in behavior. Using right click opens the bookmarks URL inside the same tab. But it still stops when you open the context menu of a root element in the bookmarks toolbar. Menu items inside the bookmarks menu aren't affected by these steps.

Root elements have to show the context menu so we can't disable them. No idea why there is no reaction afterward. I could run some tests to probably find the issue why it happens.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I tried to identify the issue step by step and I think we are a bit closer. The cause why a URL isn't opened inside the current tab after the context menu of a toolbar button was opened is that this._view.selectedURINode is null.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/places/content/controller.js&rev=1.190&mark=639&#638

Mano, do you have an idea why that happens?

Comment 13

[Marco Bonardo [:mak]]

i'm unassigning from me since i can't debug here...

Comment 14

[Marco Bonardo [:mak]]

Attachment: remove context attribute and don't handle modifiers

henrik, could you try this?

hwv this._view.selectedURINode should be != null only if you click on a URI node, not a folder, so that should not be the problem...

Comment 15

[Marco Bonardo [:mak]]

Attachment: oops. bad patch

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #14)
> Created an attachment (id=296687) [details]
> remove context attribute and don't handle modifiers
> 
> henrik, could you try this?

It doesn't work. The right click is handled now but...

> hwv this._view.selectedURINode should be != null only if you click on a URI
> node, not a folder, so that should not be the problem...
 
Sure? this._view.selectedURINode isn't null when I left click an URI. But doing a right click, it's null. So we have to explicitly select the clicked node?

At the moment there are some bugs open which handle left/middle/right clicking of bookmarks. Perhaps we should run a clean up?

Simple search: http://tinyurl.com/2ka67m

Comment 17

[Marco Bonardo [:mak]]

ok so you have that null when right clicking an URI node? i'll check if the node is correctly selected before trying to open it

Comment 18

[Marco Bonardo [:mak]]

are you still getting the popupNode error?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #17)
> ok so you have that null when right clicking an URI node?

Yes. Right clicking any bookmark results in null. Further the mentioned error doesn't appear anymore but is replaced with a new one:

Error: aChild is null
Source File: chrome://browser/content/places/toolbar.xml
Line: 966

      <method name="_isChevronChild">
        <parameter name="aChild"/>
        <body><![CDATA[
          if (!this._chevron.firstChild.hasAttribute("type"))
            return false;

 ==>      var parent = aChild.parentNode;
          while (parent != this) {
            if (parent == this._chevron)
              return true;

            parent = parent.parentNode;
          }
          return false;
        ]]></body>

      </method>
 

Comment 20

[Mike Connor [:mconnor]]

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

Comment 21

[Roger Peters]

Firefox seems to have quite a few bugs on Mac's relating  right-click support. Right clicking the mouse button (which I haven't tried since I don't have a mouse), ctrl+clicking, and right-clicking by tapping 2 fingers on the trackpad all seem to have different effects.

The following bug's may be related:
435003
486315
355947
435160

Comment 22

[Roger Peters]

Sorry, I thought the bugs would be auto-linked. Here are the links:
https://bugzilla.mozilla.org/show_bug.cgi?id=435003
https://bugzilla.mozilla.org/show_bug.cgi?id=486315
https://bugzilla.mozilla.org/show_bug.cgi?id=355947
https://bugzilla.mozilla.org/show_bug.cgi?id=435160

Comment 23

[Reed Loden [:reed]]

(In reply to comment #22)
> Sorry, I thought the bugs would be auto-linked.

You need to say "bug xxx" for them to be auto-linked.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #22)
> Sorry, I thought the bugs would be auto-linked. Here are the links:
> https://bugzilla.mozilla.org/show_bug.cgi?id=435003
> https://bugzilla.mozilla.org/show_bug.cgi?id=486315
> https://bugzilla.mozilla.org/show_bug.cgi?id=355947
> https://bugzilla.mozilla.org/show_bug.cgi?id=435160

None of those bugs are related to this bug. I triaged all of them.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This bug is WFM now with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8) Gecko/2009032608 Firefox/3.0.8 ID:2009032608

Marco, any idea which patch could have fixed it? Unless we know that I mark this bug as WFM.

Comment 26

[Marco Bonardo [:mak]]

no idea which change did exactly fix this, related changes to that code area are:
Bug 419549 - click bookmark item does not work.
Bug 423124 - CMD/CTRL+clicking a bookmark creates two new tabs. 
Bug 402558 - urls from bookmarks folder in sidebar don't open in tabs on middle-click.

Comment 27

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv