Closed Bug 346729 Opened 18 years ago Closed 18 years ago

Table cell selection is broken (copying & pasting gives two copies)

Categories

(Core :: DOM: Selection, defect, P1)

1.8 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla1.8.1beta2

People

(Reporter: Gavin, Assigned: brettw)

References

Details

(Keywords: regression, testcase, verified1.8.1)

Attachments

(2 files, 1 obsolete file)

Steps to reproduce:
1) Load http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=2006-07-31+12%3A37&maxdate=2006-07-31+12%3A37
2a) While holding down Ctrl, select cells from the table
2b) Notice that both the text itself and the cell are selected (border around the cell, highlight color on the text)
3) Ctrl+C, or Edit->Copy and paste into a text editor

Actual results: Two copies of the selected text are pasted
Expected results: The selected text is pasted

Backing out the patch to bug 345099 fixes this.
Flags: blocking1.8.1?
Weird. I won't get to this until after all the crashes are fixed.
*** Bug 346817 has been marked as a duplicate of this bug. ***
Flags: blocking1.8.1? → blocking1.8.1+
Summary: Table text selection is broken → Table cell selection is broken (copying & pasting gives two copies)
Attached patch Patch (obsolete) — Splinter Review
The problem is that the range is getting added twice for some reason. The old selection code would check table selections and not allow the same range to be added twice. I now added a general check for identical ranges, and silently succeed (since the range is already there).
Attachment #232838 - Flags: review?(bryner)
Comment on attachment 232838 [details] [diff] [review]
Patch

>+inline PRBool
>+RangeMatchesBeginPoint(nsIDOMRange* aRange, nsIDOMNode* aNode, PRInt32 aOffset)
>+{

>+inline PRBool
>+RangeMatchesEndPoint(nsIDOMRange* aRange, nsIDOMNode* aNode, PRInt32 aOffset)
>+{

These functions might be a little long for forced inlining, it's usually better to let the compiler decide anyway unless you know specifically that this is a win.

>+// nsTypedSelection::FindRangeGivenPoint
>+//
>+//    Searches for the range matching the exact given range points. We search
>+//    in the array of beginnings, and start from the given index. This index
>+//    should be the result of FindInsertionPoint, which will return any index
>+//    within a range of identical ones.
>+//
>+//    Therefore, this function searches backwards and forwards from that point
>+//    of all matching beginning points, and then compares the ending points to
>+//    find a match. Returns true if a match was found, false if not.
>+
>+PRBool
>+nsTypedSelection::FindRangeGivenPoint(
>+    nsIDOMNode* aBeginNode, PRInt32 aBeginOffset,
>+    nsIDOMNode* aEndNode, PRInt32 aEndOffset,
>+    PRInt32 aStartSearchingHere)
>+{

You might want to assert that 0 <= aStartSearchingHere <= mRanges.Length()

Looks good otherwise.
Attachment #232838 - Flags: review?(bryner) → review+
Attached patch Patch V2Splinter Review
Attachment #232838 - Attachment is obsolete: true
Attachment #232962 - Flags: superreview?(bryner)
Attachment #232962 - Flags: superreview?(bryner) → superreview+
Fixed on trunk, leaving open for branch checkin.
Attachment #232962 - Flags: approval1.8.1?
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.8.1beta2
Whiteboard: [baking until 08/12]
Bret - is there anything we can be doing to test around for regressions on this code?  Do you have any unit tests?
No. If you ask me, this situation is kind of bogus. This bug is caused by the calling code selecting the text twice. The semantics on the interface aren't clear, and I dutifully stored the two selections while the old version did not. This is not the type of problem that would be caught by a unit test anyway.

I think this bug's behavior duplicates the old version. If you have a selection with a range in it from A to B, and add another selection from A to B, it doesn't add the duplicate range (which makes sense). However, if you add a range from A to B+1, it will allow it, and everything from A to B will still be selected twice. Roc has talked about forcing the ranges to be non-overlapping, which would fix all this stupidity, but that's a different problem.
Comment on attachment 232962 [details] [diff] [review]
Patch V2

a=beltzner on behalf of drivers; brett, should we be filing a gecko 1.9 bug against the interface to make it less bogus in the future?
Attachment #232962 - Flags: approval1.8.1? → approval1.8.1+
Fixed on branch.

I'll check with roc about a bug for the overlapping ranges.
Status: NEW → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [baking until 08/12]
(In reply to comment #10)
> brett, should we be filing a gecko 1.9 bug
> against the interface to make it less bogus in the future?

Filed bug 348681.
Assignee: selection → brettw
Verified FIXED, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060902 BonEcho/2.0b2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060830 Minefield/3.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: