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)
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)
780 bytes,
text/html
|
Details | |
6.84 KB,
patch
|
bryner
:
superreview+
beltzner
:
approval1.8.1+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•18 years ago
|
Flags: blocking1.8.1?
Reporter | ||
Updated•18 years ago
|
Keywords: regression
Assignee | ||
Comment 1•18 years ago
|
||
Weird. I won't get to this until after all the crashes are fixed.
Comment 2•18 years ago
|
||
*** Bug 346817 has been marked as a duplicate of this bug. ***
Comment 3•18 years ago
|
||
Updated•18 years ago
|
Flags: blocking1.8.1? → blocking1.8.1+
Assignee | ||
Updated•18 years ago
|
Summary: Table text selection is broken → Table cell selection is broken (copying & pasting gives two copies)
Assignee | ||
Comment 4•18 years ago
|
||
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 5•18 years ago
|
||
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+
Assignee | ||
Comment 6•18 years ago
|
||
Attachment #232838 -
Attachment is obsolete: true
Attachment #232962 -
Flags: superreview?(bryner)
Updated•18 years ago
|
Attachment #232962 -
Flags: superreview?(bryner) → superreview+
Assignee | ||
Comment 7•18 years ago
|
||
Fixed on trunk, leaving open for branch checkin.
Assignee | ||
Updated•18 years ago
|
Attachment #232962 -
Flags: approval1.8.1?
Assignee | ||
Updated•18 years ago
|
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.8.1beta2
Updated•18 years ago
|
Whiteboard: [baking until 08/12]
Comment 8•18 years ago
|
||
Bret - is there anything we can be doing to test around for regressions on this code? Do you have any unit tests?
Assignee | ||
Comment 9•18 years ago
|
||
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 10•18 years ago
|
||
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+
Assignee | ||
Comment 11•18 years ago
|
||
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]
Assignee | ||
Comment 12•18 years ago
|
||
(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.
Reporter | ||
Updated•18 years ago
|
Assignee: selection → brettw
Reporter | ||
Comment 13•18 years ago
|
||
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
Keywords: fixed1.8.1 → verified1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•