Last Comment Bug 380639 - dynamic removal of feeds/<link> elements (DOMLinkRemoved) don't cause updates to the feed discovery menu (live bookmarks)
: dynamic removal of feeds/<link> elements (DOMLinkRemoved) don't cause updates...
Status: NEW
:
Product: Firefox
Classification: Client Software
Component: RSS Discovery and Preview (show other bugs)
: unspecified
: All All
: -- normal with 6 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 451738 471535 478552 (view as bug list)
Depends on:
Blocks: 396056
  Show dependency treegraph
 
Reported: 2007-05-14 08:36 PDT by Mark Streatfield
Modified: 2015-04-22 14:13 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
a potential patch for the issue described in this ticket (4.37 KB, patch)
2007-05-14 08:44 PDT, Mark Streatfield
no flags Details | Diff | Splinter Review
dynamic-feed-removal-test-case.html (305 bytes, text/html)
2015-04-22 14:13 PDT, Brandon Frohs
no flags Details

Description Mark Streatfield 2007-05-14 08:36:44 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

this applies to the live bookmarking icon in the right hand corner of the address bar in firefox

using javascript (and AJAX) it is possible to add live bookmarks after a page has finished loading by adding <link> tags to the DOM.  in browser.js there is a handler (onLinkAdded) on the event (DOMLinkAdded) to capture this.

however, if links are removed dynamically, the live bookmarking option in the address bar does not update to show the new links - essentially there is not a onLinkRemoved handler for the DOMLinkRemoved event to capture this.

this is a missing function in mozilla/browser/base/content/browser.js

Reproducible: Always

Steps to Reproduce:
on any page run some javascript to add two links:

var headId = top.document.getElementsByTagName("head")[0];
var newLink = document.createElement('link');
newLink.rel = 'alternate';
newLink.type = 'application/rss+xml';
newLink.title = "link a";
newLink.href = "http://linka.com/feed.xml";
headId.appendChild(newLink);

var newLink = document.createElement('link');
newLink.rel = 'alternate';
newLink.type = 'application/rss+xml';
newLink.title = "link b";
newLink.href = "http://linkb.com/feed.xml";
headId.appendChild(newLink);

and then remove a link:

headId.removeChild(headId.firstChild);
Actual Results:  
live bookmarking shows "link a" and "link b"

Expected Results:  
live bookmarking to show just one link option
Comment 1 Mark Streatfield 2007-05-14 08:44:22 PDT
Created attachment 264751 [details] [diff] [review]
a potential patch for the issue described in this ticket
Comment 2 Aubrey Alexander 2007-10-03 07:07:27 PDT
This is exactly what I am looking for!
Comment 3 Nickolay_Ponomarev 2007-11-10 16:17:36 PST
For instructions on moving forward with the patch, see: http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree
Comment 4 Aubrey Alexander 2007-11-14 13:40:16 PST
Can somebody else submit Mark's fix, or does it have to be Mark himself?
Comment 5 Nickolay_Ponomarev 2007-11-14 15:46:53 PST
Doesn't matter, but fwiw, I don't think that patch would be accepted as is, because it introduces quite a bit of duplicated code.
Comment 6 Phil Ringnalda (:philor) 2007-11-14 16:15:59 PST
More significant than the duplicated code is that it just doesn't work: Array.pop() takes off the last element of the array, it doesn't take arguments and search for them. An actual working patch would need to do that searching, would need to decide what constitutes a match if it isn't exactly "all three match" (if you add a feed with one title, change the title, then remove the link element, does it remove the feed with a matching href and type, or not?), and now that we have a single listener for DOMLinkAdded for feeds, icons, and search, would probably need to handle removing all three, and would need automated tests along the lines of the tests for adding in http://mxr.mozilla.org/seamonkey/source/browser/base/content/test/
Comment 7 Aubrey Alexander 2007-11-14 16:21:03 PST
Thanks for the info.  I've never submitted anything myself before, but it gives me some direction (and some hint of what I might be up against) if I wanted to try to do a patch myself.  
Comment 8 m8r-00wwvr 2008-02-21 19:32:28 PST
Is anyone actively working on the patch? If not I may give it a shot.
Comment 9 Justin Wood (:Callek) (Away until Aug 29) 2008-02-21 21:58:19 PST
William, by all means feel free to work on it (it does not appear anyone is currently)...

Do realize that currently Firefox 3 is nearing its "Beta 4" milestone which means that any code changes are strictly looked at, so no-one can guarantee that your change will actually be in Firefox 3 (yet).  But if its done fast enough, and gets needed reviews, hopefully it can make it!
Comment 10 Phil Ringnalda (:philor) 2008-08-22 14:43:49 PDT
*** Bug 451738 has been marked as a duplicate of this bug. ***
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-01-08 12:31:56 PST
*** Bug 471535 has been marked as a duplicate of this bug. ***
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-01-08 12:36:25 PST
See bug 471535 comment 6 and subsequent comments for some thoughts about how to fix this.

Claudio and Matteo also mention an issue with DOMLinkRemoved in that bug, but there aren't really any details about the problem - Claudio, can you elaborate?
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-01-08 12:40:00 PST
Alternatively, we could just update the menu each time it's opened (which would also fix bug 396056).
Comment 14 Claudio Procida 2009-01-08 12:53:48 PST
The issue with DOMLinkRemoved is that this event is never fired. The workaround by Matteo fires a DOMLinkRemoved event when the DOMNodeRemoved event has a LINK element as original target. That is the purpose of the _FixDOMEvents function added as event handler to catch DOMNodeRemoved events right after the DOMContentLoaded event.
Comment 15 Justin Wood (:Callek) (Away until Aug 29) 2009-01-08 13:23:43 PST
I would rather fix this in Gecko with DOMLinkRemoved correctly than have us incur a perf penalty with the DOMNodeRemoved listener[s] on the document.

Is there any case where DOMLinkRemoved *is* fired? I thought there was a few at least!
Comment 16 Phil Ringnalda (:philor) 2009-02-14 07:30:21 PST
*** Bug 478552 has been marked as a duplicate of this bug. ***
Comment 17 Nicolas Hoizey 2014-05-26 06:33:58 PDT
I saw this issue today, trying to develop a userscript that would reduce the number of feeds in http://500px.com/ pages and leave only the ones that are relative to current page.

I confirm links removed using JavaScript are still listed when opening the feeds list with the RSS icon button.
Comment 18 Anthony Ricaud (:rik) 2014-05-26 06:58:09 PDT
Is there anyone on this bug who would be kind enough to mentor it?
Comment 19 Matthew N. [:MattN] 2014-05-30 19:17:15 PDT
(In reply to Anthony Ricaud (:rik) (out until June 2nd) from comment #18)
> Is there anyone on this bug who would be kind enough to mentor it?

Sure, although solving this with the approach in comment 13 is trickier now with multiple processes which makes me think we should get DOMLinkRemoved working and just listen for that. I just tested and it seems to work for some removals while not others but I haven't investigated why.

To start, I would recommend making a test case showing that DOMLinkRemoved doesn't always work and attaching it in a Core dependency bug. With that bug fixed, this should be straightforward.

The relevant code has moved a bit due to E10S:
* https://mxr.mozilla.org/mozilla-central/search?string=Link%3AAddFeed
* https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-feeds.js
Comment 20 Brandon Frohs 2015-04-22 14:13:37 PDT
Created attachment 8596180 [details]
dynamic-feed-removal-test-case.html

Minimal test case. The link named "Static (removed)" is removed and the link named "Dynamic (added)" is added immediately with JavaScript. Both show up in the Subscribe menu while only "Dynamic (added)" should show up.

Note You need to log in before you can comment on or make changes to this bug.