Closed
Bug 304458
Opened 19 years ago
Closed 18 years ago
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)
Categories
(Firefox Graveyard :: RSS Discovery and Preview, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 2
People
(Reporter: grantwohl+mozbug, Assigned: asqueella)
References
(Blocks 1 open bug, )
Details
(Keywords: fixed1.8.1)
Attachments
(1 file, 7 obsolete files)
14.06 KB,
patch
|
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
*** Bug 304501 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Status: UNCONFIRMED → NEW
Component: General → Location Bar and Autocomplete
Ever confirmed: true
OS: Windows XP → All
Comment 2•19 years ago
|
||
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•19 years ago
|
||
Using openUILink() would make all the link modifiers work as expected.
Comment 5•19 years ago
|
||
Feedview has been backed out, and the button now creates Live Bookmarks. Marking INVALID.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → INVALID
Reporter | ||
Comment 6•19 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
Comment 7•19 years ago
|
||
(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
Closed: 19 years ago → 19 years ago
Resolution: --- → INVALID
Assignee | ||
Updated•19 years ago
|
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Target Milestone: Firefox1.5 → ---
Assignee | ||
Comment 8•19 years ago
|
||
(Feedview is back for Firefox 2, reopened.)
Assignee | ||
Comment 9•19 years ago
|
||
Assignee: bugs.mano → asqueella
Status: REOPENED → ASSIGNED
Attachment #227850 -
Flags: review?(bugs.mano)
Assignee | ||
Updated•19 years ago
|
Priority: P3 → --
Target Milestone: --- → Firefox 2 beta2
Assignee | ||
Comment 10•19 years ago
|
||
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)
Comment 11•19 years ago
|
||
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•19 years ago
|
||
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)
Comment 13•19 years ago
|
||
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•19 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•19 years ago
|
Attachment #227878 -
Attachment is obsolete: true
Assignee | ||
Comment 15•19 years ago
|
||
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 16•19 years ago
|
||
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•18 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 18•18 years ago
|
||
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•18 years ago
|
||
There you go.
Attachment #227987 -
Attachment is obsolete: true
Attachment #231922 -
Flags: review?(bugs.mano)
Comment 20•18 years ago
|
||
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•18 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.
Comment 22•18 years ago
|
||
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•18 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 24•18 years ago
|
||
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•18 years ago
|
||
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
Comment 27•18 years ago
|
||
Attachment #232407 -
Attachment is obsolete: true
Comment 28•18 years ago
|
||
Attachment #234378 -
Attachment is obsolete: true
Comment 29•18 years ago
|
||
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
Closed: 19 years ago → 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed] → [baking]
Version: Trunk → 2.0 Branch
Comment 30•18 years ago
|
||
Not going to block on this, but would take a safe patch.
Flags: blocking-firefox2? → blocking-firefox2-
Updated•18 years ago
|
Target Milestone: Firefox 2 beta2 → Firefox 2
Comment 31•18 years ago
|
||
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?
Updated•18 years ago
|
Whiteboard: [baking] → [baking][181approval pending]
Updated•18 years ago
|
Whiteboard: [baking][181approval pending] → [181approval pending][baking]
Comment 32•18 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+
Updated•18 years ago
|
Whiteboard: [181approval pending][baking] → [checkin needed (1.8 branch)]
Comment 33•18 years ago
|
||
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)]
Comment 34•18 years ago
|
||
*** Bug 304843 has been marked as a duplicate of this bug. ***
Comment 35•15 years ago
|
||
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
Updated•15 years ago
|
Flags: in-litmus?
Updated•15 years ago
|
Flags: in-litmus? → in-testsuite?
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•