304458 - Feed icon/menu doesn't respect key modifiers / middle mouse button (was: middle-clicking the feed icon or an item from the drop-down feed menu should open the feed in a new tab)

Status: VERIFIED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Grant Wohl]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050812 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050812 Firefox/1.0+

Middle-clicking the feed icon or an item from the drop-down feed menu should
open the feed in a new tab (or a new window, depending on the setting of
"browser.tabs.opentabfor.middleclick").

Reproducible: Always

Steps to Reproduce:
1.Go to a page with one or more feeds.
2.If the page has only one feed, middle-click the feed icon in the location bar.
3.If the page has more than one feed, click the feed icon in the location bar
and then middle-click one of the feeds.

Actual Results:  
If the page had only one feed, it selected the text in the location bar. If the
page had more than one feed, it didn't do anything.

Expected Results:  
Firefox should have opened the feed in a new tab (or a new window, depending on
the setting of "browser.tabs.opentabfor.middleclick").

Comment 1

[Peter van der Woude [:Peter6]]

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

Comment 2

[Asaf Romano (gone)]

Also need to respect ctrl+click, ctrl+shift+click and shift+click

Comment 3

[Jesse Ruderman]

Using openUILink() would make all the link modifiers work as expected.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

(This should block bug 303848)

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Feedview has been backed out, and the button now creates Live Bookmarks. Marking
INVALID.

Comment 6

[Grant Wohl]

(In reply to comment #5)
> Feedview has been backed out, and the button now creates Live Bookmarks. Marking
> INVALID.

Feedview may have been backed out of the branch, but it is still in the trunk.
Reopening and changing version to trunk.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> Feedview may have been backed out of the branch, but it is still in the trunk.
> Reopening and changing version to trunk.

Feedview was backed out branch and trunk.

Comment 8

[Nickolay_Ponomarev]

(Feedview is back for Firefox 2, reopened.)

Comment 9

[Nickolay_Ponomarev]

Attachment: patch

Comment 10

[Nickolay_Ponomarev]

Attachment: patch

Duh, forgot to check the event phase in the feed button's click handler.

Comment 11

[Phil Ringnalda (:philor)]

With attachment 227852 [details] [diff] [review], with a single discovered feed, either left-click or cmd+click on the addressbar icon gives me 

Warning: reference to undefined property event.phase
Source File: chrome://browser/content/browser.js
Line: 5824"

Comment 12

[Nickolay_Ponomarev]

Attachment: patch

Gah. Yes, it's supposed to be event.eventPhase, but somehow this didn't get into the patch :/ Thanks.

Comment 13

[Phil Ringnalda (:philor)]

Trunk/places, if you have no top-level bookmarks directly in the bookmarks menu then middle-clicking the Subscribe menuitem opens the feed preview, but if you have at least one bookmark it does Open in Tabs on whatever's directly in the Bookmarks Menu folder. 

Comment 14

[Nickolay_Ponomarev]

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

Thanks for testing! I'll post an updated patch later.

Comment 15

[Nickolay_Ponomarev]

Attachment: another attempt

Phil confirmed that the issue in comment 13 is in fact a places bug 331856. The new patch fixes one other issue, clicking on "Subscribed to this page" when it's disabled.

Comment 16

[Asaf Romano (gone)]

Comment on attachment 227987 [details] [diff] [review]
another attempt

>Index: browser/base/content/browser-menubar.inc
>===================================================================

> function checkForMiddleClick(node, event)
> {
>-  if (event.button == 1) {
>+  if (event.button == 1 && !node.hasAttribute("disabled")) {

Why is this additional check and why wasn't it/isn't it necessary for other callers of checkForMiddleClick?

Also, unless you pass a node which doesn't extend chrome://global/content/bindings/general.xml#basecontrol, the right check is !this.disabled.

Comment 17

[Nickolay_Ponomarev]

(In reply to comment #16)
> (From update of attachment 227987 [details] [diff] [review] [edit])
> >Index: browser/base/content/browser-menubar.inc
> >===================================================================
> 
> > function checkForMiddleClick(node, event)
> > {
> >-  if (event.button == 1) {
> >+  if (event.button == 1 && !node.hasAttribute("disabled")) {
> 
> Why is this additional check and why wasn't it/isn't it necessary for other
> callers of checkForMiddleClick?
> 
There are few in-tree callers of checkForMiddleClick. Of them a few are never disabled (release notes, context menu's view image), and the back button fires an exception when middle clicked in disabled state.

> Also, unless you pass a node which doesn't extend
> chrome://global/content/bindings/general.xml#basecontrol, the right check is
> !this.disabled.
> 
I know, but since there are no restrictions on the kind of nodes that can be passed to this function, I figured I would rely on the attribute instead. I don't remember any widgets one might want to pass to checkForMiddleClick that don't inherit from basecontrol, so it might make sense to use the property. What do you say?

Comment 18

[Asaf Romano (gone)]

Comment on attachment 227987 [details] [diff] [review]
another attempt

Sorry for latency. Nickolay , can you update this patch (to either branch or trunk tip) so i can test it? Thanks.

Comment 19

[Nickolay_Ponomarev]

Attachment: updated patch

There you go.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 231922 [details] [diff] [review]
updated patch


>   
>   /**
>    * Subscribe to a given feed.  Called when
>    *   1. Page has a single feed and user clicks feed icon in location bar
>    *   2. Page has a single feed and user selects Subscribe menu item
>    *   3. Page has multiple feeds and user selects from feed icon popup
>    *   4. Page has multiple feeds and user selects from Subscribe submenu
>    * @param   href
>-   *          The feed to subscribe to
>+   *          The feed to subscribe to. May be null, in which case the
>+   *          event target's feed attribute is examined.
>+   * @param   event
>+   *          The event this method is handling. Used to decide where 
>+   *          to open the preview UI. (Optional)
>    */
>-  subscribeToFeed: function(href) {
>-#ifdef MOZ_PLACES
>+  subscribeToFeed: function(href, event) {

I would rather make the event parameter optional (note openUILink does take the event as an optional parameter). The feed attribute is an implementaion detial of the in-tree callers, not this method by itself.

Looks pretty good otherwise.

Comment 21

[Nickolay_Ponomarev]

(In reply to comment #20)
> >+   * @param   event
> >+   *          The event this method is handling. Used to decide where 
> >+   *          to open the preview UI. (Optional)
> >    */
> >-  subscribeToFeed: function(href) {
> >-#ifdef MOZ_PLACES
> >+  subscribeToFeed: function(href, event) {
> 
> I would rather make the event parameter optional (note openUILink does take 
> event as an optional parameter). The feed attribute is an implementaion detial
> of the in-tree callers, not this method by itself.
> 
Thanks for the review, but I don't understand what should be changed. The event parameter is already optional, it's just that you can supply the event instead of the href parameter. It is then used to decide both where the feed should be opened and what is the feed's URL.

Comment 22

[Asaf Romano (gone)]

The href parameter shouldn't be optional IMO. It's pretty bad to have two optioanl parameters when only one of them can be optional when calling.

Comment 23

[Nickolay_Ponomarev]

Doesn't feel wrong to me (maybe just a bit)... Are you suggesting to write a wrapper for subscribeToFeed which gets the feed's URL from an event and calls subscribeToFeed? I can do that, just not sure it is much better.

Or simply pass event.target.getAttribute("feed") wherever we call subscribeToFeed? It seemed rather awkward, which is why I implemented subscribeToFeed the way I did.

Comment 24

[Asaf Romano (gone)]

Comment on attachment 231922 [details] [diff] [review]
updated patch

Not that it's really optimal, but r=mano if you update the documentation to reflect that both parameters are optional (but only one each time).

Comment 25

[Nickolay_Ponomarev]

Attachment: patch

The documentation was almost correct, so I just replaced "Optional" with "Optional, unless href is null" for the event param. Is this what you meant?

Comment 26

[Asaf Romano (gone)]

Yeah, thanks.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Attachment: unbitrotted

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Attachment: unbitrotted, pass event to loadFeed

Comment 29

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/base/content/utilityOverlay.js 	1.40
mozilla/browser/base/content/browser.xul 	1.315
mozilla/browser/base/content/browser.js 	1.683
mozilla/browser/base/content/browser-menubar.inc 	1.101

Comment 30

[Mike Connor [:mconnor]]

Not going to block on this, but would take a safe patch.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 234379 [details] [diff] [review]
unbitrotted, pass event to loadFeed

This has been on the trunk since Friday the 18th.

Comment 32

[Mike Schroepfer]

Comment on attachment 234379 [details] [diff] [review]
unbitrotted, pass event to loadFeed

a=schrep for drivers - approving all [181approval pending] bugs now that the tree is open.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/base/content/browser-menubar.inc 	1.57.2.40
mozilla/browser/base/content/browser.js 	1.479.2.192
mozilla/browser/base/content/browser.xul 	1.268.2.59
mozilla/browser/base/content/utilityOverlay.js 	1.32.2.10

Comment 34

[Asaf Romano (gone)]

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

Comment 35

[Tanner M. Young [:tmyoung]]

Verified Fixed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090901 Namoroka/3.6a2pre (.NET CLR 3.5.30729) ID:20090901050855

However, shift+click doesn't work as requested in comment #2.  Rather than reopen this bug, I have filed Bug 514308 for it.

Comment 36

[Tanner M. Young [:tmyoung]]

Resetting QA Contact to default.