Closed Bug 1740281 Opened 3 years ago Closed 3 years ago

Toggling off node picker with keyboard shortcut does not hide the highlighter

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox-esr91 wontfix, firefox94 wontfix, firefox95 wontfix, firefox96 verified)

Status:
VERIFIED FIXED
Milestone:
96 Branch
Tracking Flags:
Tracking Status
firefox-esr91 --- wontfix
firefox94 --- wontfix
firefox95 --- wontfix
firefox96 --- verified

People

(Reporter: nchevobbe, Assigned: jdescottes)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Bug 1740281 - [devtools] Cancel node picker correctly when using client side shortcuts
48 bytes, text/x-phabricator-request
Details | Review

Steps to reproduce

  1. Open a tab with data:text/html,<meta charset=utf8><button>First button</button><button>Second one</button>
  2. Ctrl+Shift+C (or Cmd+Shift+C on OSX) and hover First button
  3. Hit Ctrl+Shift+C (or Cmd+Shift+C on OSX) again

Expected results

The picker stops, the <body> node is still selected, and the highlighter is hidden

Actual results

The picker stops and the <body> node is still selected, but the highlighter is not hidden. It will only go away when hovering another node in the markup view (or starting the picker again)

It was working fine in 78 but is broken in 91 (maybe before).

Severity: -- → S3
Keywords: regressionwindow-wanted
Priority: -- → P3
QA Whiteboard: [qa-regression-triage]

Hello I have managed to find a regression for this issue on Ubuntu 20.
Here is the pushlog:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fb2b08c6bc8c320127586fcedb2626e496bf3dcf&tochange=ee89deeb4fb6d12456c551c5ab4aaae151ceedae

Have a nice day!

Has Regression Range: --- → yes
QA Whiteboard: [qa-regression-triage]
status-firefox94: --- → affected
status-firefox95: --- → affected
status-firefox96: --- → affected
Keywords: regressionwindow-wanted

Thank you Negritas!

Perhaps this one could be the culprit?
Bug 1607756 - Extract node picker functionality from HighlighterActor

Honza

:sergiu.negritas, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Flags: needinfo?(sergiu.negritas)

As per Honza's suggestion marking the regressed by field with the 1607756 bug. If it's not the right bug please feel free to change it with another bug.

Thank you!

Flags: needinfo?(sergiu.negritas)
Regressed by: 1607756

Julian, do you think that Bug 1607756 caused the regression?

Flags: needinfo?(jdescottes)

Probably, the issue comes from two conflicting events emitted by the client-side node-picker:

  • picker-stopped
  • picker-node-canceled

picker-stopped is a purely client-side event which is always emitted when the picker is stopped, whatever the reason.
picker-node-canceled comes from the server's node-picker. Since there is no NodePicker actor, the event is transmitted through the WalkerActor/Front, but that's just a technical detail. Note that even if the picker is actually "canceled" here in the STRs (at least semantically), since this is purely handled by the DevTools client, the picker-node-canceled event is not involved.

When the client node-picker receives a picker-node-canceled, it will both emit a picker-stopped event and will re-emit picker-node-canceled. But the opposite is not true. If a consumer explicitly calls the node-picker stop method, only the picker-stopped event will be emitted. So if a client side consumer relies on picker-node-canceled, they will only be triggered when the picker is canceled from the server.

And this is what happens for the highlighting here. The inspector is listening for picker-node-canceled and will unhighlight at https://searchfox.org/mozilla-central/rev/674b6582bafedc11fba6dc537769c88e9ed921d3/devtools/client/inspector/inspector.js#1953 . Instead it should most likely rely on picker-stopped.

The same goes for the markup-view, which listens for node-picker-canceled to reset the scrollposition of the markup view at https://searchfox.org/mozilla-central/rev/674b6582bafedc11fba6dc537769c88e9ed921d3/devtools/client/inspector/markup/markup.js#461. It should also switch to picker-stopped.

There is one last consumer from this event in toolbox.js which will use it to focus back the toolbox. It's not really clear here if it makes sense to only perform this for a server initiated cancel.

In general, I would suggest to only rely on picket-stopped on the client, potentially passing an additional "reason" flag if some consumers need to distinguish between a regular stop and a cancel.

Flags: needinfo?(jdescottes)

After quickly testing, we might still need the distinction between stop and cancel. When we cancel the picker we want to immediately hide the highlighter, whereas when we "pick" the highlighter stays for one second.
So it's between three events:

  • picker-stopped
  • picker-node-canceled
  • picker-node-picked

In the end, it might be easier to simply find a way to emit picker-node-canceled from codepaths which cancel the picker from the client side (eg https://searchfox.org/mozilla-central/rev/674b6582bafedc11fba6dc537769c88e9ed921d3/devtools/client/definitions.js#158)

One last note, to reproduce it in a mochitest, make sure you are focusing the toolbox before sending the CtrlShift+C shortcut, otherwise this will fail.

Oh well, spent too much time investigating the mochitest weirdness, let's fix it.

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

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

status-firefox-esr91: --- → affected

Depends on D131174

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3785a75145e1
[devtools] Cancel node picker correctly when using client side shortcuts r=nchevobbe

Thank you for working on this, Julian!

https://hg.mozilla.org/mozilla-central/rev/3785a75145e1

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox96: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch

The patch landed in nightly and beta is affected.
:jdescottes, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(jdescottes)

This one can ride the trains IMO. Been broken for quite some time, and it's not really breaking any user flow, it's more of an inconvenience.
Thanks

status-firefox94: affected → wontfix
status-firefox95: affected → wontfix
status-firefox-esr91: affected → wontfix
Flags: needinfo?(jdescottes)

Hello! I have managed to reproduce the issue with firefox 96.0a1(2021-11-01) on Ubuntu 20. I can confirm that the issue is fixed with firefox 96.0a1(2021-11-22) on Ubuntu 20, Windows 10 and MacOS 10.15.

Updating the flags and marking this issue as VERIFIED->FIXED.

Thank you!

Status: RESOLVED → VERIFIED
status-firefox96: fixed → verified
Blocks: 1742436

Comment on attachment 9250990 [details]
Bug 1740281 - [devtools] Use private fields in client node-picker

Revision D131270 was moved to bug 1742436. Setting attachment 9250990 [details] to obsolete.

Attachment #9250990 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: