1709832 - dragenter event incorrectly fires on Text nodes (should be Element nodes only)

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P2)

Description

[u587334]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.93 Safari/537.36

Steps to reproduce:

I was writing a drag event handler on a <table>.

I needed to obtain the parent <tr> element while dragging over various nested elements, such as a <div> containing text inside <td>.

I was having trouble getting consistent results between browsers with Node.nodeName. So I decided to use Element.closest().

Here's a quick HTML example of my markup nesting (it's not all that complicated):

<tr>
    <td>
      Test 123<div>Test 123</div>
    </td>
</tr>

Actual results:

It seemed to work, but I sometimes got this error: Uncaught TypeError: event.target.closest is not a function

Upon investigation, it only happens if event.target has a nodeType of TEXT_NODE. This appears to be a "Text Interface" according to MDN: https://developer.mozilla.org/en-US/docs/Web/API/Text.

It would seem that Element.closest() can only handle type ELEMENT_NODE.

Expected results:

The text node itself has a parentElement property that Element.closest() should be able to use. This bug does not occur in Chrome.

Comment 1

[u587334]

Maybe this is related: https://github.com/whatwg/dom/issues/161

Comment 2

[BugBot [:suhaib / :marco]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

[u587334]

This appears to be a workaround that makes my code work as it should (adapted from whatwg issue):

if (!Text.prototype.closest) Text.prototype.closest = function(s) {return this.parentNode.closest(s); }

Existing polyfills don't seem to solve this particular issue because they still only work with Element.

Comment 4

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

This bug does not occur in Chrome.

Chrome also limits closest() to Element so it should be same. Could you share the full page for us to debug the issue? Possibly https://codepen.io or such?

The text node itself has a parentElement property that Element.closest() should be able to use.

Sounds like a valid argument to me. Anne, do you recall why closest() is limited to Elements?

Comment 5

[u587334]

Here's a test case: https://codepen.io/bryc/pen/MWpWzYv

To reproduce the issue:

1. Click and hold on "Test" text.
2. Drag to the right towards "123" text, careful not to cross over the dotted red line.

(Don't let go just yet)

Result: Firefox should give the error, Chrome will log "a1" to the console.

3. Continue dragging from the black "123" text, over the dotted red line, down to the red "123" text below it.

Result: Firefox will log "a1", and then give a second error. Chrome will log a second "a1".

---

Hope that helps highlight the issue.

Comment 6

[u587334]

https://developer.mozilla.org/en-US/docs/Web/API/Node/contains

This method works somewhat similarly, it checks if a node is contained in another node. Might be something to contrast against, because it could often be used together and be assumed to work similarly. Firefox doesn't have this issue here.

Comment 7

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Hmm, it seems Firefox emits dragenter with text nodes while Chrome emits with the parent element. The spec says the target must be "Immediate user selection or the body element" where the former is also an element, so the target should always be an element.

Anne, could you double check also for this one?

Comment 8

[Anne (:annevk)]

Agreed that the specification says that dragenter events must only fire on elements.

Comment 11

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Attachment: Bug 1709832 - Part 1: Do not fire dragenter/leave at text nodes r=edgar

Comment 12

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Attachment: Bug 1709832 - Part 2: Mark drag event dispatchers as MOZ_CAN_RUN_SCRIPT r=edgar

Because EventDispatcher::Dispatch can run script (but not marked as such, which is bug 1539884).

Depends on D121006

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/763aa9ea82f0
Part 1: Do not fire dragenter/leave at text nodes r=edgar
https://hg.mozilla.org/integration/autoland/rev/1db9d4f36b1e
Part 2: Mark drag event dispatchers as MOZ_CAN_RUN_SCRIPT r=edgar

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/763aa9ea82f0
https://hg.mozilla.org/mozilla-central/rev/1db9d4f36b1e