1907490 - `Draggable.startDragging` uses `Element.setPointerCapture` with `undefined`

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://searchfox.org/mozilla-central/rev/f7b41fc41c5505db507d076c16961a8da67dd318/devtools/client/shared/components/splitter/Draggable.js#58

  startDragging(ev) {
    const xDiff = Math.abs(this.mouseX - ev.clientX);
    const yDiff = Math.abs(this.mouseY - ev.clientY);

    // This allows for double-click.
    if (this.props.onDoubleClick && xDiff + yDiff <= 1) {
      return;
    }
    this.mouseX = ev.clientX;
    this.mouseY = ev.clientY;

    if (this.isDragging) {
      return;
    }
    this.isDragging = true;
    ev.preventDefault();

    this.draggableEl.current.addEventListener("mousemove", this.onMove);
    this.draggableEl.current.setPointerCapture(ev.pointerId);

This is a mousedown event listener. So, ev is a MouseEvent interface, but MouseEvent does not have pointerId.
https://searchfox.org/mozilla-central/rev/f7b41fc41c5505db507d076c16961a8da67dd318/dom/webidl/MouseEvent.webidl

So, the value must be undefined which can be treated as 0. 0 is used for mouse events and first touch of active touches. Therefore, this could work as expected for mouse inputs, but this may cause unexpected exception if the event is the compatibility mouse events of Touch Events. Additionally, this pointerId value is set by different rules from the other browsers. Therefore, the value could be changed for solving web compat issues.

I think that it should listen to pointerdown, pointerup and pointercancel instead, and it should handle it only when the pointerType is mouse.

Comment 1

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

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

:nchevobbe, since you are the author of the regressor, bug 1682754, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1907490 - Make `Draggable` listen to `pointerdown` to capture the pointer

MouseEvent.pointerId is undefined so that setPointerCapture(undefined)
means setPointerCapture(0) and it captures both click of mouse and first
touch of touch input devices. It needs to listen to pointerdown event to
get the correct pointerId value. Then, it can use only pointerdown and
pointerup because they are fired before mousedown and mouseup.
Additionally, it handles a drag while a mouse button is pressed. Therefore,
it can ignore pointer events which are caused by other devices.

Although it's not required explicitly to release the capture when the pointer
is up, it needs to release capturing the pointer if Escape is pressed.
Therefore, this patch keeps the explicit release call.

Depends on D217216

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df598ad0ab53
Make `Draggable` listen to `pointerdown` to capture the pointer r=nchevobbe,devtools-reviewers

Comment 4

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/df598ad0ab53