380639 - dynamic removal of feeds/<link> elements (DOMLinkRemoved) don't cause updates to the feed discovery menu (live bookmarks)

Status: RESOLVED INACTIVE

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

Description

[Mark Streatfield]

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]

Attachment: a potential patch for the issue described in this ticket

Comment 2

[Aubrey Alexander]

This is exactly what I am looking for!

Comment 3

[Nickolay_Ponomarev]

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]

Can somebody else submit Mark's fix, or does it have to be Mark himself?

Comment 5

[Nickolay_Ponomarev]

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)]

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]

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]

Is anyone actively working on the patch? If not I may give it a shot.

Comment 9

[Justin Wood (:Callek)]

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 12

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

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: [e-mail]]]

Alternatively, we could just update the menu each time it's opened (which would also fix bug 396056).

Comment 14

[Claudio Procida]

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)]

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 17

[Nicolas Hoizey]

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)]

Is there anyone on this bug who would be kind enough to mentor it?

Comment 19

[Matthew N. [:MattN]]

(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]

Attachment: 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.

Comment 21

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.