301357 - Simplify context menu selection code

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement)

Description

[Brian Polidoro]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050719 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050719 Firefox/1.0+

This can be reproduced using the 'Vote for this bug' link.  The bug seems to
require text before the link.  For example the 'Votes' link functions properly.  

Reproducible: Always

Steps to Reproduce:
1.Select the text of a link using the mouse from the left edge of the link.  
2.Right click on the selected text.


Actual Results:  
No copy option is present on the context menu.

Expected Results:  
A copy menu item should be present.

This also occurs when selecting a link with caret browsing with the same
requirements.

Comment 1

[Brian Polidoro]

The regression time frame for this seems to be: 
6/13 OK 
6/14 broken
From the build thread for 6/14:
http://forums.mozillazine.org/viewtopic.php?t=279117

I'd point to this bug fix:
bug 293758 [Firefox]-Context menu screw-up with data and javascript protocol
images [All]

Comment 2

[Martijn Wargers (dead)]

Ah! I get the same regression period, independantly. However, I think the fix
for bug 280284 is more likely the cause of this bug.

Comment 3

[Ben Basson]

Confirmed, Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4)
Gecko/20050718 Firefox/1.0+

Comment 4

[Martijn Wargers (dead)]

Ok,
this part of the patch in bug 280284 returns true:
 if (node.nodeType == node.TEXT_NODE && node.nodeValue.length == 0) {

So when selecting from the left to the right directly before a link, you begin
with an empty textnode. That doesn't seem logical in itself, but the code of
that patch is also not good.

Probably the collapsed property of a range object is best used here.

Comment 5

[Martijn Wargers (dead)]

Attachment: patch

Like this. This fixes it for me.

Comment 6

[Brian Polidoro]

Is this possibly a dupe of bug 301466 which was just checked in?  The patch is
different but changes the same area.  

Comment 7

[Martijn Wargers (dead)]

You're right, Brian, it is the same bug. However, I think my patch is better
(imho). I'll ask in that bug.

Comment 8

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

Yes, this patch is much simpler and works better than the kludge in the other
patch. Martijn, do you want to update your patch now that the other one has been
checked in?

Comment 9

[Martijn Wargers (dead)]

Attachment: patch2, updated

I've diffed this locally, and again tested the patch.

Comment 10

[Martijn Wargers (dead)]

Attachment: patch3

This is even a bit simpler (based on some sharp suggestions by Gavin).
I've also tested this patch a bit, for not breaking anything (and fixing the
issue).

Comment 11

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

Changing summary to reflect new intent of bug.

Comment 12

[Jesse Ruderman]

+   return ((selection.rangeCount >= 1) && !selection.getRangeAt(0).collapsed)

Shouldn't this be true if *any* range in the selection is uncollapsed?

Comment 13

[Martijn Wargers (dead)]

Comment on attachment 189944 [details] [diff] [review]
patch3

Yes, you're right.
Probably I can use the isCollapsed property on the selection object to simplify
this code even more:
http://mozref.com/reference/objects/Selection#isCollapsed

Comment 14

[Martijn Wargers (dead)]

Attachment: patch4

I've tested that this works correctly.
There is a potential issue where you have multiple ranges and all of the
instances of the ranges are collapsed, then you get the "View Selection
Source", when you probably shouldn't, but I think that is a situation that
virtually never happens.

Comment 15

[Chris]

WFM 

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050806
Firefox/1.0+ ID:2005080619

Context menu:
Open Link in New Window
Open Link in New Tab
----------------
Bookmark This Link...
Save Link As...
Send Link...
Copy Link Location
Copy
----------------
Search Web for ""
View Selection Source
Properties

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 191399 [details] [diff] [review]
patch4

Please get this in on the trunk, and then request branch approval after a
couple of days.

Comment 17

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

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

Comment 18

[Tracy Walker [:tracy]]

works well on Linux 2005-08-29-06-trunk

Comment 19

[Jay Patel [:jay]]

v.fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050829 Firefox/1.6a1. I see the same context menu as in comment #15.  We
should get this onto the branch.

Comment 20

[Asaf Romano (gone)]

1.8 Branch:
Checking in browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.479.2.20; previous revision: 1.479.2.19
done

Comment 21

[Tracy Walker [:tracy]]

verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07