All users were logged out of Bugzilla on October 13th, 2018

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)

VERIFIED FIXED in Firefox 2

Status

()

VERIFIED FIXED
13 years ago
9 years ago

People

(Reporter: grantwohl+mozbug, Assigned: asqueella)

Tracking

(Blocks: 1 bug, {fixed1.8.1})

2.0 Branch
Firefox 2
fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

13 years ago
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").
*** Bug 304501 has been marked as a duplicate of this bug. ***
Status: UNCONFIRMED → NEW
Component: General → Location Bar and Autocomplete
Ever confirmed: true
OS: Windows XP → All
Also need to respect ctrl+click, ctrl+shift+click and shift+click
Assignee: nobody → bugs.mano
Severity: minor → normal
Component: Location Bar and Autocomplete → RSS Discovery and Preview
Priority: -- → P3
Hardware: PC → All
Summary: middle-clicking the feed icon or an item from the drop-down feed menu should open the feed in a new tab → 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)
Target Milestone: --- → Firefox1.1

Comment 3

13 years ago
Using openUILink() would make all the link modifiers work as expected.
(This should block bug 303848)
Blocks: 303848
Feedview has been backed out, and the button now creates Live Bookmarks. Marking
INVALID.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → INVALID
(Reporter)

Comment 6

13 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Version: unspecified → Trunk
(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.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → INVALID
(Assignee)

Updated

13 years ago
Blocks: 70501
No longer blocks: 303848
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Target Milestone: Firefox1.5 → ---
(Assignee)

Comment 8

13 years ago
(Feedview is back for Firefox 2, reopened.)
(Assignee)

Comment 9

13 years ago
Created attachment 227850 [details] [diff] [review]
patch
Assignee: bugs.mano → asqueella
Status: REOPENED → ASSIGNED
Attachment #227850 - Flags: review?(bugs.mano)
(Assignee)

Updated

13 years ago
Priority: P3 → --
Target Milestone: --- → Firefox 2 beta2
(Assignee)

Comment 10

13 years ago
Created attachment 227852 [details] [diff] [review]
patch

Duh, forgot to check the event phase in the feed button's click handler.
Attachment #227850 - Attachment is obsolete: true
Attachment #227852 - Flags: review?(bugs.mano)
Attachment #227850 - Flags: review?(bugs.mano)
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"
(Assignee)

Comment 12

13 years ago
Created attachment 227878 [details] [diff] [review]
patch

Gah. Yes, it's supposed to be event.eventPhase, but somehow this didn't get into the patch :/ Thanks.
Attachment #227852 - Attachment is obsolete: true
Attachment #227878 - Flags: review?(bugs.mano)
Attachment #227852 - Flags: review?(bugs.mano)
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. 
(Assignee)

Comment 14

13 years ago
Comment on attachment 227878 [details] [diff] [review]
patch

Thanks for testing! I'll post an updated patch later.
Attachment #227878 - Flags: review?(bugs.mano)
(Assignee)

Updated

13 years ago
Attachment #227878 - Attachment is obsolete: true
(Assignee)

Comment 15

13 years ago
Created attachment 227987 [details] [diff] [review]
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.
Attachment #227987 - Flags: review?(bugs.mano)
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.
(Assignee)

Comment 17

13 years ago
(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 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.
Attachment #227987 - Flags: review?(bugs.mano)
(Assignee)

Comment 19

12 years ago
Created attachment 231922 [details] [diff] [review]
updated patch

There you go.
Attachment #227987 - Attachment is obsolete: true
Attachment #231922 - Flags: review?(bugs.mano)
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.
Attachment #231922 - Flags: review?(bugs.mano) → review-
(Assignee)

Comment 21

12 years ago
(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.
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.
(Assignee)

Comment 23

12 years ago
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 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).
Attachment #231922 - Flags: review- → review+
(Assignee)

Comment 25

12 years ago
Created attachment 232407 [details] [diff] [review]
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?
Attachment #231922 - Attachment is obsolete: true
Yeah, thanks.
Flags: blocking-firefox2?
Whiteboard: [checkin needed]
Created attachment 234378 [details] [diff] [review]
unbitrotted
Attachment #232407 - Attachment is obsolete: true
Created attachment 234379 [details] [diff] [review]
unbitrotted, pass event to loadFeed
Attachment #234378 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed] → [baking]
Version: Trunk → 2.0 Branch
Not going to block on this, but would take a safe patch.
Flags: blocking-firefox2? → blocking-firefox2-
Target Milestone: Firefox 2 beta2 → Firefox 2
Comment on attachment 234379 [details] [diff] [review]
unbitrotted, pass event to loadFeed

This has been on the trunk since Friday the 18th.
Attachment #234379 - Flags: approval1.8.1?
Whiteboard: [baking] → [baking][181approval pending]
Whiteboard: [baking][181approval pending] → [181approval pending][baking]

Comment 32

12 years ago
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.
Attachment #234379 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [181approval pending][baking] → [checkin needed (1.8 branch)]
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
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
*** Bug 304843 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Blocks: 355761
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Resetting QA Contact to default.
QA Contact: general → rss.preview
Flags: in-litmus? → in-testsuite?
You need to log in before you can comment on or make changes to this bug.