301466 - Unable to copy text with adjacent HTML

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Jerry Baker]

20050715 trunk

If you double-click a word that is surrounded by HTML such as <b> or <span>, and
then right-click, the context menu does not have a "copy" option even though the
word is selected.

Comment 1

[Jerry Baker]

Attachment: Test case

Double-click on either the word "bold" or "span" and then right-click. Witness
the wrong context menu.

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

worksforme with build 2005-07-19-06, SeaMonkey trunk.  Are you using Firefox?

Comment 3

[Jerry Baker]

Yes. I am surprised that the context menu switching is different between Mozilla
and Firefox.

Comment 4

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

Attachment: patch

The check for an empty selection was slightly flawed, so this makes sure that
the empty text fragment is in fact the only item in the selection.

Comment 5

[Jerry Baker]

Forgive my ignorance, but why is nodeValue returning a null? Is the HTML tag
considered childNode[0] and so it has no text to return? If so, I would say that
this bug is directly related to the discussion in bug 157065 about Firefox
selecting things from the source when they are not visible in the document. In
this case the <b> or <span> tags.

Comment 6

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

(In reply to comment #5)
> Forgive my ignorance, but why is nodeValue returning a null? Is the HTML tag
> considered childNode[0] and so it has no text to return? If so, I would say that
> this bug is directly related to the discussion in bug 157065 about Firefox
> selecting things from the source when they are not visible in the document. In
> this case the <b> or <span> tags.

nodeValue isn't returning null. The goal of this code is to determine whether
there is a selection. It does this by retrieving the rangeFragment, which is an
array of nodes that are selected. When there is no selection, the rangeFragment
contains only an empty text node. When you double-click select the bolded text,
rangeFragment contains an empty text node, a "bold" node, and a text node who's
value is " " (in that order). To determine whether there is a selection, the
code currently just checks that the first node is an empty text node, and
doesn't verify that it is the only node in the selection. My patch corrects
this. This has nothing to do with bug 157065.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 189919 [details] [diff] [review]
patch

good catch guys, thanks for picking this one up.

Comment 8

[Jerry Baker]

I see what you are saying. How is it possible to end up with a null selection (a
text node with zero characters selected)? Just curious how to get into that
condition in a test case.

Comment 9

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

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.458; previous revision: 1.457
done

Comment 10

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

(In reply to comment #8)
> I see what you are saying. How is it possible to end up with a null selection (a
> text node with zero characters selected)? Just curious how to get into that
> condition in a test case.

It isn't possible to select "nothing". The selection simply returns an empty
text node to indicate "no selection". As to why it does that, you'd have to ask
someone else :).

Comment 11

[Martijn Wargers (dead)]

Uhm, bug 301357 is basically the same bug. I posted a patch there, and I think
it might be a better patch, since it removes the whole cloneContents() entirely,
since I don't think it is necessary in this case.
It fixes also the testcase in this bug.

Comment 12

[Worcester12345]

(In reply to comment #11)
> Uhm, bug 301357 is basically the same bug. I posted a patch there, and I think
> it might be a better patch, since it removes the whole cloneContents() entirely,
> since I don't think it is necessary in this case.
> It fixes also the testcase in this bug.

Cool! I am assuming that that patch isn't the one which got checked in to fix
this, right?

Comment 13

[Worcester12345]

Nevermind that last post. Saw the comments in the other bug. Sorry, and thanks.

Comment 14

[Greg K.]

Verified fixed using Win FF 1.5.