1524115 - When clicking and selecting source text on a collapsible message, the message expansion is toggled after releasing the mouse

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Brian Grinstead [:bgrins]]

STR:

• execute console.group("foo"); console.log("bar"); console.groupEnd("foo")
• select and highlight the source (in this case, "debugger eval code:1:1")
• let go of mouse

Expected:
"debugger eval code:1:1" is selected, group is still open

Actual:
"debugger eval code:1:1" is selected, but group is closed

If you select again then the group gets reopened. I'd expect the click and drag to not toggle the group as a normal click does.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

The Message toggling happens in devtools/client/webconsole/components/Message.js#111-118, and is fired on click.

We should do an early return if there's any text selected.
It could look something like:

const selection = e.target.ownerDocument.defaultView.getSelection();
if (selection.type === "Range") {
  return;
}

Then we could have a test similar to devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_selected_text.js

I'd be happy to mentor anyone who'd like to work on this 🙂

Comment 2

[SresthaSrivastava [:srestha]]

Hey, can I take this up?

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Hello srestha, thanks for wanting to help us.

You can set up the work environment by following this page https://docs.firefox-dev.tools/getting-started/
Also, feel free to ask any question, here or on our Slack

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: toggle-on-select.gif

Here's another example where this makes it hard to copy out a stack trace. Looks like the proposed fix will cover this case as well.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Hello srestha!
Is there anything blocking you on this bug?
No worries if you simply don't have the time to look into this :)

Comment 6

[SresthaSrivastava [:srestha]]

Hey, sorry for the late reply, I just got busy, I'll get it done in a day or two.

Comment 7

[SresthaSrivastava [:srestha]]

Hey, I don't quite understand the actual result, could someone upload a gif of it?

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Srestha, you can look at the gif in comment 4 🙂

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Hello again Srestha.
Do you still plan to work on this? Is there anything I can clarify?

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

clearing assignee

Comment 11

[Armando Ferreira]

Hi Nicolas, Can I also take this?

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Hello Armando, it's yours :)

Comment 13

[Armando Ferreira]

Attachment: reproduce-bug-armando.gif

Comment 14

[Armando Ferreira]

Attachment: testing-armando.gif

Comment 15

[Armando Ferreira]

Hi Nicholas, I'm able to confirm that your proposed fix works like a charm, but I'm struggling to reproduce it on a test.

I created this new function in head.js to be able to select multiple nodes.

function selectNodes(hud, nodes) {
  const outputContainer = hud.ui.outputNode.querySelector(".webconsole-output");

  // We must first blur the input or else we can't select anything.
  outputContainer.ownerDocument.activeElement.blur();

  const selection = outputContainer.ownerDocument.getSelection();
  selection.removeAllRanges();

  for (const node of nodes) {
    const range = document.createRange();
    range.selectNodeContents(node);
    selection.addRange(range)
  }

  return selection;
}

But when using it like this on a test:

add_task(async function () {

  const hud = await openNewTabAndConsole(TEST_URI);
  const label = "bat";

  const onLoggedMessage = waitForMessage(hud, label);

  await ContentTask.spawn(gBrowser.selectedBrowser, label, function (str) {
    content.wrappedJSObject.console.group(str, "collapsible-test");
    content.wrappedJSObject.console.log("joy");
    content.wrappedJSObject.console.log("can");
    content.wrappedJSObject.console.log("sip");
    content.wrappedJSObject.console.log("van");
    content.wrappedJSObject.console.groupEnd("collapsible-test");
  });
  const { node } = await onLoggedMessage;

  info(`Select all the text in collapsible message`);
  selectNodes(hud, node.parentNode.querySelectorAll(".message"));
  await waitFor(() => 1 === 0);
});

I got the result as the GIF I uploaded in comment #14

Any suggestions? On how I can reproduce this issue on a test?

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Hello Armando,

Despite my comment earlier, I don't think we can use the same test mechanism as things are a bit different.
Here, the group collapses because it register a click event at some point (because we mousedown, move the mouse, and mouseup, the down/up is what gives a click).
So what we should do is simulate this behavior.

EventUtils.sendMouseEvent({ type: "mousedown" }, node);
EventUtils.sendMouseEvent({ type: "mousemove", /* additional properties to actually move */ }, node);
EventUtils.sendMouseEvent({ type: "mouseup" }, node);

but I'm not sure this would work, as those events are simulated and might only impact the js, not the browser behavior
Maybe you can try to use selectNode between the mousedown and the mouseup.
Let me know if this work

Comment 17

[Armando Ferreira]

Thanks Nicolas! I will try to reproduce with the sendMouseEvent()

Comment 18

[Armando Ferreira]

Attachment: Bug 1524115 - When clicking and selecting source text on a collapsible message, the message expansion is toggled after releasing the mouse. r=nchevobbe

When clicking and selecting source text on a collapsible message, the
message expansion is toggled after releasing the mouse

Comment 19

[Armando Ferreira]

Hi Nicholas, I tried my best to reproduce the bug under a test but I was unable to do it. I tried sendMouseEvent with mousedown, mousemove, mouseup and using selectNode in between them but it was not being triggered. Also I tried dragEvents to see if this will trigger but it also didn't trigger the bug. I'm sending the fix to code review D37607.

Let me know if there is anything more I can do :)

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

I'll ask around and look if there's a way we can mimick the text selection, but otherwise we can drop it.
Were you waiting a bit between these operations? (like await new Promise(res => setTimeout(res, 10))), just to see if that would help?

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7ca971deb77
When clicking and selecting source text on a collapsible message, the message expansion is toggled after releasing the mouse. r=nchevobbe

Comment 22

[Andrei Ciure[:aciure]]

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