Using createRange() and then adding that range to the selection with addRange throws exception

RESOLVED FIXED

Status

()

Core
Selection
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Martijn Wargers (dead), Unassigned)

Tracking

({testcase})

Trunk
x86
Windows XP
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

12 years ago
See discussion at url, especially at the bottom.

When creating a range with createRange() and then immediately trying to add it to the selection with addRange(), you get an exception.
This should not happen, since the initial range created with createRange() is a valid selection in the document.

Last comment from Boris in the discussion on the mailing list:
"
Martijn wrote:
> You're right, it doesn't really makes sense to add an empty range to
> the selection, but should it really throw? I suspect not.

I think I agree.  As best I can tell, it throws because the code that tries to
determine whether the range is a table cell selection will throw if the
startContainer is not an nsIContent.  That's probably bogus.  Just switching
that code (in nsTypedSelection::GetTableSelectionType) to nsINode (which
documents do QI to) will help, since then the QI on document would succeed and
the IsNodeOfType(nsINode::eELEMENT) call would return false and things would be
happy.
"
(Reporter)

Updated

12 years ago
Assignee: traversal-range → selection
Component: DOM: Traversal-Range → Selection
QA Contact: ian
(Reporter)

Comment 1

12 years ago
Created attachment 226657 [details]
testcase
(Reporter)

Comment 2

12 years ago
Created attachment 226663 [details] [diff] [review]
patch
Attachment #226663 - Flags: review?(bzbarsky)
(Reporter)

Comment 3

12 years ago
I presume you meant something like this, Boris?
Comment on attachment 226663 [details] [diff] [review]
patch

>Index: layout/generic/nsSelection.cpp
>+  if (!node->IsNodeOfType(nsINode::eELEMENT))
>     return NS_OK; //got to be a text node, definitely not a table row/cell

Take the "got to be a text node, " part out of the comment, and looks good.

>+  nsCOMPtr<nsIContent> content = do_QueryInterface(startNode);

Assert after this that |content| is not null?
Attachment #226663 - Flags: superreview+
Attachment #226663 - Flags: review?(bzbarsky)
Attachment #226663 - Flags: review+
(Reporter)

Comment 5

12 years ago
Created attachment 226670 [details] [diff] [review]
patch for checkin
(Reporter)

Comment 6

12 years ago
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.244; previous revision: 3.243
done

Checked in on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.