Closed
Bug 329122
Opened 19 years ago
Closed 19 years ago
Event dispatching code in nsGenericDOMDataNode doesn't handle event retargeting
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(4 files, 2 obsolete files)
(deleted),
application/xhtml+xml
|
Details | |
(deleted),
application/xhtml+xml
|
Details | |
(deleted),
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
Event dispatching code in nsGenericDOMDataNode doesn't handle event retargeting.
Currently nsGenericDOMDataNode::HandleDOMEvent, after bug 234455 nsGenericDOMDataNode::PreHandleEvent .
Assignee | ||
Comment 1•19 years ago
|
||
Should the BindingParent handling of nsGenericDOMDataNode
changed too. Right now nsGenericDOMDataNode uses its parent's BindingParent.
That is wrong, right?
<binding>
<content>Some text</content>
</binding>
Binding parent should be the parent node of the text node, not the binding parent of the parent node.
Comment 2•19 years ago
|
||
Yeah, the problem is not wanting to add another pointer to textnodes...
Do we have a free bit on nsGenericDOMDataNode? If so, we could stick the binding parent in the property table in the rare case when it differ's from our parent's... Or I guess the only way they could differ is if the binding parent _is_ the parent, so we could flag that with the bit...
We should just stick mFlagsOrSlots on nsGenericDOMDataNode IMHO. Yes, it'll cost us a few bytes, but really, i doubt it'll be that bad.
And that'd let us move the member to nsIContent and inline a bunch of stuff...
Comment 4•19 years ago
|
||
Hmm... So that would be sizeof(void*) more bytes per textnode, right?
Yes, i think that is something that wouldn't make the sky fall.
Comment 6•19 years ago
|
||
Sticking it in the property hash doesn't really require another bit, just call CouldHaveProperties. It means an additional hash lookup if the node is a range boundary, has an event listener or has other properties (all of those switch the IS_IN_A_HASH bit). It would also add a hash lookup for looking up all those when the bindingparent != parent. Wouldn't make the sky fall either ;-).
Comment 7•19 years ago
|
||
I guess either way. Let's just do it one way or the other? ;)
Assignee | ||
Updated•19 years ago
|
Assignee: events → Olli.Pettay
Assignee | ||
Comment 8•19 years ago
|
||
Assignee | ||
Comment 9•19 years ago
|
||
Moved mFlagsOrSlots to nsIContent, nsDOMSlots to its own header file,
some generic implementations of nsIContent methods (used by nsGenericElement and nsGenericDOMDataNode, but not always by nsXULElement - some other generic nsIContent method implementations can be done later), event retargeting in nsGenericDOMDataNode.
What do you think about this, Jonas?
Attachment #220633 -
Flags: review?(bugmail)
Assignee | ||
Comment 10•19 years ago
|
||
Attachment #220633 -
Attachment is obsolete: true
Attachment #220645 -
Flags: review?(bugmail)
Attachment #220633 -
Flags: review?(bugmail)
Drats, so we have duplicated work. I have a patch 334075 that does a lot of the same thing. Mind if we checked that one in first?
Assignee | ||
Comment 12•19 years ago
|
||
(In reply to comment #11)
> Drats, so we have duplicated work. I have a patch 334075 that does a lot of the
> same thing. Mind if we checked that one in first?
>
Sounds good to me.
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•19 years ago
|
||
Comment on attachment 220645 [details] [diff] [review]
better mutation events handling
I'll do a new patch after bug 334075
Attachment #220645 -
Attachment is obsolete: true
Attachment #220645 -
Flags: review?(bugmail)
Assignee | ||
Comment 14•19 years ago
|
||
Assignee | ||
Comment 15•19 years ago
|
||
nsGenericDOMDataNode doesn't need all the members that
nsGenericElement::nsDOMSlots has, so I decided to do a simpler class
for nsGenericDOMDataNode, (not to move nsDOMSlots to nsIContent for example).
The patch fixes both testcases.
Attachment #222345 -
Flags: review?(bugmail)
Comment on attachment 222345 [details] [diff] [review]
nsDOMSlots for nsGenericDOMDataNode
>Index: src/nsGenericDOMDataNode.cpp
>+nsGenericDOMDataNode::nsDOMSlots::nsDOMSlots(PtrBits aFlags)
Please name the class something different than nsDOMSlots to avoid confusion with the one in nsGenericElement. nsDataSlots for example.
Ideally we should rename the nsGenericElement one to nsElemSlots or some such.
> nsGenericDOMDataNode::GetChildNodes(nsIDOMNodeList** aChildNodes)
...
>+ nsDOMSlots *slots = GetDOMSlots();
>+
>+ if (!slots) {
> return NS_ERROR_OUT_OF_MEMORY;
> }
I like NS_ENSURE_TRUE(slots, NS_ERROR_OUT_OF_MEMORY). Up to you. Same applies elsewhere in the patch.
Looks great otherwise! Please keep an eye on regressions though since i'm not sure if we have any code that'll start freaking out when we start support bindingParents on textnodes.
r=me
Attachment #222345 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 17•19 years ago
|
||
Attachment #222357 -
Flags: superreview?(bzbarsky)
Comment 18•19 years ago
|
||
Comment on attachment 222357 [details] [diff] [review]
nsDOMSlots -> nsDataSlots
>Index: src/nsGenericDOMDataNode.cpp
> nsGenericDOMDataNode::GetChildNodes(nsIDOMNodeList** aChildNodes)
Please set *aChildNodes to null up front, so it's null in the error cases?
>+ NS_PRECONDITION(!GetBindingParent() ||
>+ aBindingParent == GetBindingParent() ||
>+ (aParent &&
>+ aParent->GetBindingParent() == GetBindingParent()),
>+ "Already have a binding parent. Unbind first!");
So the third part of that is a little wrong. See the nsGenericElement version (or https://bugzilla.mozilla.org/show_bug.cgi?id=286000#c13).
> nsGenericDOMDataNode::UnbindFromTree(PRBool aDeep, PRBool aNullParent)
> {
>+ // Notify XBL- & nsIAnonymousContentCreator-generated
>+ // anonymous content that the document is changing.
Maybe comment that we need this so that the insertion point for |this| will get updated? Otherwise it looks weird, since generic DOM data nodes can't have XBL bindings, anon kids, etc.
sr=bzbarsky.
I sorta wonder whether it makes sense to have an nsIContent::Slots impl that would have binding parent and child nodes and just have generic element extend it. We might be able to have a single method (nsIContent::BindToTree ?) for BindToTree then -- the only difference between nsGenericElement and nsGenericDOMDataNode now is in walking the child nodes... Separate bug, though.
Attachment #222357 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 19•19 years ago
|
||
Checked in.
Still watching whether this made any changes to tp/tdhtml/etc..
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
I was thinking about putting mBindingParent in nsSlots too, but since in nsGenericElement it lives inside a union I don't think it's woth the hassle
You need to log in
before you can comment on or make changes to this bug.
Description
•