1815472 - Debugger tooltip displays wrong value when there is line wrapping

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Oriol Brufau [:Oriol]]

Attachment: testcase

1. In about:config, set devtools.debugger.ui.editor-wrapping = true.
2. Open the attached testcase with the debugger open
3. Find this line: isNaN(cs1, cs2);. The debugger should show some information:

   cs1: CSS2Properties(354) { "accent-color" → "auto", "align-content" → "normal", "align-items" → "normal", … } cs2: CSS2Properties(354) { "accent-color" → "auto", "align-content" → "normal", "align-items" → "normal", … }
   

4. Ensure that information appears on the line below isNaN(cs1, cs2);. If you have a wide screen and it fits on the same line, shrink your browser window or resize the debugger sidebars.
5. Hover hoverMe

Expected: you see NaN
Actual: you see

prop: 0
<prototype>: Object { … }

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: Screenshot

Comment 2

[Hubert Boma Manilla (:bomsy)]

Attachment: Screenshot 2023-02-15 at 08.32.00.png

Comment 3

[Hubert Boma Manilla (:bomsy)]

Thanks for reporting i can reproduce the issue.

Also for me myObject is NaN when it shouldn't be. See the second screenshot attached.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

FYI this is an annoying behavior when the window of the debugger is too small, which is exactly the case for the default window size. Most of the lines are wrapped in my case when I'm trying to debug code from /remote/.

Comment 5

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1815472 - [devtools] Cleanup and use utility file for tokens r=#devtools-reviewers

Changes in this patch

• Rename the token-events.js => token.js
• Unify all the token realted utils into token.js
• Remove other token util related files
• Remove not really useful jest test get-token-location.spec.js as this is already covered by preview related mochitests

Comment 6

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1815472 - [devtools] Disable inline-previews when editor wrapping is r=#devtools-reviewers

Depends on D181960

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Bomsy, do you have an update for us? Is it something you could fix or is it blocked by some dependency not yet listed on this bug? Thanks.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

There's an issue in a CodeMirror method which doesn't look trivial to fix (see https://phabricator.services.mozilla.com/D181962#6054336)
I did attempt to fix this but it failed through the cracks, I'll try to resume that work

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1815472 - [devtools] Fix variable tooltip when location is impacted by inline preview or wrapping. r=#devtools-reviewers.

In getTokenLocation, the call to coordsChar needed an extra parameter so the
coordinates are relative to the top left corner fo the currently visible window.
We were passing the center of the bounding client rect to getTokenLocation, but
a token could wrap and have it's bounding rect center not being on the token.
To fix this we're passing the center of the first box quads of the token.

The inline preview widgets were causing some troubles on CodeMirror internal
state too. Since we're using React to populate the bookmark we set on the line,
and given that React.render is asynchronous, we were setting an empty element
as a bookmark on the editor, which React populates later. But CodeMirror probably
update its internal state when the bookmark is added, so that was causing wrong
computation later.
To avoid that, we only set the bookmark once React actually rendered the element
in the DOM.

We add a test to cover this, and also modify test helpers so we would query
the DOM instead of relying on CodeMirror methods.

Depends on D181960

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/930888998f68
[devtools] Cleanup and use utility file for tokens r=devtools-reviewers,nchevobbe

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04e0ba20153f
[devtools] Fix variable tooltip when location is impacted by inline preview or wrapping. r=devtools-reviewers,bomsy.

Comment 12

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/930888998f68

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/04e0ba20153f

Comment 14

[Narcis Beleuzu [:NarcisB]]

Backed out for dt failure on browser_dbg-preview.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/507c2d152cee16515182154ff49e3622581a7b40
Log link: https://treeherder.mozilla.org/logviewer?job_id=432475866&repo=autoland&lineNumber=19574

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/c85ad51385ad
Backed out changeset 04e0ba20153f for dt failure on browser_dbg-preview.js

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

I fixed the failing tests. Let's see if TRY is happy: https://treeherder.mozilla.org/jobs?repo=try&revision=045153cd121aa63af5ff31149d688769cc11d779

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64122f08423f
[devtools] Fix variable tooltip when location is impacted by inline preview or wrapping. r=devtools-reviewers,bomsy.

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/64122f08423f

Comment 19

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:bomsy, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval. Also, don't forget to request an uplift for the patches in the regression caused by this fix.
• If no, please set status-firefox120 to wontfix.

For more information, please visit BugBot documentation.

Comment 20

[Hubert Boma Manilla (:bomsy)]

Comment on attachment 9357027 [details]
Bug 1815472 - [devtools] Fix variable tooltip when location is impacted by inline preview or wrapping. r=#devtools-reviewers.

Beta/Release Uplift Approval Request

• User impact if declined: Developer debugging webpages will see wrong values in the preview tooltip, when inline-preview is enabled and the line is wrapped.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Devtools JS code fix mainly related to the tooltip display, should be low risk
• String changes made/needed:
• Is Android affected?: No

Comment 21

[Dianna Smith [:diannaS]]

Comment on attachment 9340915 [details]
Bug 1815472 - [devtools] Cleanup and use utility file for tokens r=#devtools-reviewers

This patch already landed in fx120 during the nightly cycle

Comment 22

[Dianna Smith [:diannaS]]

Comment on attachment 9357027 [details]
Bug 1815472 - [devtools] Fix variable tooltip when location is impacted by inline preview or wrapping. r=#devtools-reviewers.

Waiting on issue with bug 1860639 before uplifting

Comment 23

[Dianna Smith [:diannaS]]

Comment on attachment 9357027 [details]
Bug 1815472 - [devtools] Fix variable tooltip when location is impacted by inline preview or wrapping. r=#devtools-reviewers.

Approved for 120.0b3

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/67fbd0a1b997