Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Highlight DOM Nodes onHover of Lexical DevTools Node #2728

Merged

Conversation

noi5e
Copy link
Contributor

@noi5e noi5e commented Jul 29, 2022

Highlight.DevTools.Nodes.mov

Developer Tools Web Extension Planning Issue: #2127

@vercel
Copy link

vercel bot commented Jul 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Sep 3, 2022 at 4:44AM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Sep 3, 2022 at 4:44AM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 29, 2022

// append highlight overlay <div> to document
chrome.devtools.inspectedWindow.eval(`
const highlightOverlay = document.createElement('div');
Copy link
Contributor Author

@noi5e noi5e Jul 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized a bit late that a lot of the stuff in devtools.js can be moved into content.js. Particularly the parts that don't depend on the editor object, but are just declaring functions and variables in the window scope.

This would be ideal since they would be linted as TS in content.ts, and they wouldn't be run in an eval.

I can either move it in this PR with another commit, or early in the next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized a bit late that a lot of the stuff in devtools.js can be moved into content.js.

Actually forget about it, I spent the last couple of days working on this, but I can't get around the fact that content.js cannot document.querySelector the editorDOMNode. This is because content scripts have limited access to the DOM.

I don't think I'll be able to get around having the highlight/dehighlighting code within devtools.

@thegreatercurve This is otherwise ready to merge!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I'll be able to get around having the highlight/dehighlighting code within devtools.

I've been experimenting with some of the approaches in this StackOverflow post.

Some of it seems promising. In particular, I think the way to go in the future will be to have the content script inject something via document.createElement('script'). React DevTools takes this approach while still making limited use of inspectedWindow.eval().

I tried to get this approach running yesterday, but for some reason it doesn't quite work yet. I think it has something to do with timing, like the script is being injected before the DOM finishes loading. I'll keep experimenting for now, but I think whatever changes I make should be the subject of a new PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try defer loading of the script by setting async to be true on the script, or add an event listener on the window for after the document has fully loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try defer loading of the script by setting async to be true on the script, or add an event listener on the window for after the document has fully loaded.

Nice, I tried some of these. I went with another workaround where the DevTools app sends a message, requesting to load the editorState the moment the DevTools panel is opened in the browser console. Seems to work fine.

The good news is that I got all of the code out of the eval() statement and into an injected script, and it works great. I'll bundle the changes into another PR right after this one is merged.

onMouseEnter={handleMouseEnter}
onMouseLeave={handleMouseLeave}
style={{paddingLeft: leftIndent}}>
<span style={{width: '1.2em'}}>&nbsp;</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should 1.2 be a constant somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it's using CSS variables now...

This is a little weird, not sure if this is best practice, but I'm using getComputedStyle here to read the CSS variable from the DOM in TreeView:

const monospaceWidth = getComputedStyle(
    document.documentElement,
  ).getPropertyValue('--monospace-character-width');

In order to pass it to TreeNode here:

const leftIndent = depth * parseFloat(monospaceWidth) + 'em';

@noi5e
Copy link
Contributor Author

noi5e commented Aug 3, 2022

This is merge ready if the fix here is okay, tests are mostly passing except for some unrelated stuff.

Once this is merged, I'll open a PR for the changes mentioned here.

@thegreatercurve thegreatercurve merged commit d44f6db into facebook:main Aug 3, 2022
@noi5e noi5e deleted the highlight-DOM-nodes-onHover branch August 3, 2022 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants