1905267 - Possible uncaught TypeError when detaching tab

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[tabmix.onemen]

When holding tab title and dragging the tab to detach it, we end up with this errors if the target element of the event was the #text

Uncaught TypeError: can't access property "contains", event.target.classList is undefined
    on_mouseover chrome://browser/content/tabbrowser/tab.js:348
    handleEvent chrome://global/content/customElements.js:459

    on_mouseover(event) {
      if (event.target.classList.contains("tab-close-button")) {
        this.mOverCloseButton = true;
      }

Comment 1

[Niklas Baumgardner [:niklas]]

Thanks for filing this bug!
We are unable to reproduce this.
Are you able to reproduce this bug in a new profile?

Comment 2

[tabmix.onemen]

Yes,

I tested a new profile on Firefox nightly from today.
make sure to drag the mouse by its title.
see in this image where to hold your mouse for dragging the tab. then drag the tab to detach it
image

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit BugBot documentation.

Comment 4

[Marco Bonardo [:mak]]

I bisected the problem to
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=23d502ed06280881577c5f9df617026ee30500f1&tochange=88c50740c90849f41b36a3ebdea5c1182d010b62
That points to Bug 1877815.
I don't know the details about that change, but it looks like it would change the event target in some cases, and that's apparently what happened here. I can confirm event.target and event.originalTarget are a #text node.
I'm not sure whether that change was expected, thus needinfo-ing Masayuki, before evaluating changes to the code here (we can surely check what the event target is).

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1877815

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Of source, no.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1905267 - part 1: Make `PresShell` manage event target frame and content with a struct r=smaug!,#dom-core

PresShell::mCurrentEventTargetContent may be update to non-element node
when PresShell::mCurrentEventTargetFrame is destroyed. Therefore, for
avoiding it, I'd like to add EventMessage to the group to consider whether
the content needs to be Element. However, adding new members for current
and stack would make PresShell members more messy. Therefore, I'd like to
group the data with the simple struct.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1905267 - part 2: Make `PresShell` not use non-element event target when target frame is destroyed and the event wants `Element` target r=smaug!,#dom-core

When event target frame is destroyed, PresShell::NotifyDestroyingFrame updates
current event target content to the frame content. However, event target of
some DOM events need to be Element. Therefore,
PresShell::mCurrentEventTarget needs to store at least the handling
EventMessage to consider whether the event target can be non-element content
node or only element node. So, we need to make PresShell::EventTargetInfo
store EventMessage. Then, we can make PresShell::NotifyDestroyingFrame
prefer inclusive ancestor element as new event target from the EventMessage.

Although I don't understand what's going on the reported case, but I found the
simple case when event target is changed to a Text with the assertion in
PresShell::DispatchEventToDOM. Therefore, this patch has a new test.

Note that if target frame is a frame for element, e.g., when event target is
a focused element, IsForbiddenDispatchingToNonElementContent result won't
change the behavior. Therefore, its implementation is not optimized for
non-user input events nor user input events without ref-point.

Depends on D217204

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/917da27ba36c
part 1: Make `PresShell` manage event target frame and content with a struct r=smaug
https://hg.mozilla.org/integration/autoland/rev/d4221479caa6
part 2: Make `PresShell` not use non-element event target when target frame is destroyed and the event wants `Element` target r=smaug

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/47347 for changes under testing/web-platform/tests

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

The patches fix a bug of considering event target at text frame destruction which has existed since 1999 at latest. Therefore, we should not uplift the patches except when this bug causes users unhappy in some important web apps in the wild.

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/917da27ba36c
https://hg.mozilla.org/mozilla-central/rev/d4221479caa6

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot