274486 - 'Bookmark This Link...' fails to gather link text from nested elements for bookmark title

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect)

Description

[Fergus Incoronato]

123456789_123456789_123456789_123456789_123456789_123456789_
Usually when in a page there is a string, in this case, 
"Traffic Shaping with Linux", that is a link and I try to 
use "Bookmark This Link..." on it, the string suggested as 
a "Name:" is the same as the one of the link.

But when I try to use "Bookmark This Link..." on the 
"Traffic Shaping with Linux" link in the page I reported 
(it's about the twentyfifth link in the page, at the end of 
the first message), the string suggested as a "Name:" is 
different from "Traffic Shaping with Linux", namely just 
"Traffic".

Comment 1

[Phil Ringnalda (:philor)]

Confirmed in 1.0 and 20041209 trunk: we stop when we get the first text node
that's in a grandchild of the <a>, rather than just in a child.

Comment 2

[Phil Ringnalda (:philor)]

Attachment: testcase

Bookmark This Link gives "mozilla dot" rather than "mozilla dot org" as a
suggested name.

Comment 3

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

Comment 4

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

Comment 5

[Phil Ringnalda (:philor)]

Comment on attachment 254397 [details] [diff] [review]
Fix v.1

Mmm, no. Image alt attributes ought to be handled better, concatenating them with the rest of the text, not completely forgotten.

Comment 6

[Phil Ringnalda (:philor)]

Attachment: Fix v.2

Well, if we're going to do anything smarter than this, we need to find a smart owner for the bug.

Comment 8

[Phil Ringnalda (:philor)]

Actually, not smarter, just more persistent. Using a documentencoder's fairly easy, something like

+  const kEncoderCIDbase = "@mozilla.org/layout/documentEncoder;1?type=";
+  var doc = root.ownerDocument;
+
+  if ((kEncoderCIDbase + doc.contentType) in Components.classes) {
+    const nsIDocumentEncoder = Components.interfaces.nsIDocumentEncoder;
+    var encoder = Components.classes[kEncoderCIDbase + doc.contentType]
+                            .createInstance(nsIDocumentEncoder);
+    var flags = encoder.OutputNoScriptContent | encoder.OutputNoFramesContent;
+    encoder.init(doc, "text/plain", flags);
+    encoder.setNode(root);
+    return encoder.encodeToString().replace(/^\s+|\s+$/g, "")
+                                   .replace(/\s+/g, " ");

(with an else, and maybe a try, too), but since that involves using two essentially untested things, documentencoder and plaintextserializer, someone's going to be in for a whole lot of test writing to land it.

Comment 9

[Nickolay_Ponomarev]

The testcase here works fine for me (fixed in bug 710878?), I'll move comment 8 into the remaining filed bug about this (bug 306937).