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

Simplify application context menu cache, rename to be clearer. #5932

Merged
merged 3 commits into from Feb 1, 2019

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 1, 2019

This function is still a bit of a hack, but a useful one at times. This tries to simplify it and make it a bit clearer.

@jasongrout jasongrout self-requested a review Feb 1, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

@ian-r-rose - it seems that composedPath is now pretty well supported. What do you think about using it? https://developer.mozilla.org/en-US/docs/Web/API/Event/composedPath

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 1, 2019

I haven't really looked into it. Does it return all the DOM nodes in the hierarchy, or just ones for which an event handler is registered?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 1, 2019

(I also don't view this PR as critical, just wanted to sneak it in since it's API breaking)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

From the examples, it appears to return all elements on which listeners could be invoked, i.e., all, not just ones that have listeners actually listening.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 1, 2019

Sure, I'll try that.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

diff --git a/packages/application/src/frontend.ts b/packages/application/src/frontend.ts
index c9bd3196c..637bab0f4 100644
--- a/packages/application/src/frontend.ts
+++ b/packages/application/src/frontend.ts
@@ -115,17 +115,7 @@ export abstract class JupyterFrontEnd<
     ) {
       return undefined;
     }
-    // This one-liner doesn't work, but should at some point in the future
-    // `return this._contextMenuEvent.composedPath() as HTMLElement[];`
-    // cf. (https://developer.mozilla.org/en-US/docs/Web/API/Event)
-    let node = this._contextMenuEvent.target as HTMLElement;
-    do {
-      if (test(node)) {
-        return node;
-      }
-      node = node.parentNode as HTMLElement;
-    } while (node.parentNode && node !== node.parentNode);
-    return undefined;
+    return this._contextMenuEvent.composedPath().find(test);
   }
 
   /**

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

Or maybe even simpler:

diff --git a/packages/application/src/frontend.ts b/packages/application/src/frontend.ts
index c9bd3196c..dda4d7aea 100644
--- a/packages/application/src/frontend.ts
+++ b/packages/application/src/frontend.ts
@@ -109,22 +109,9 @@ export abstract class JupyterFrontEnd<
   contextMenuHitTest(
     test: (node: HTMLElement) => boolean
   ): HTMLElement | undefined {
-    if (
-      !this._contextMenuEvent ||
-      !(this._contextMenuEvent.target instanceof HTMLElement)
-    ) {
-      return undefined;
+    if (this._contextMenuEvent) {
+      return this._contextMenuEvent.composedPath().find(test);
     }
-    // This one-liner doesn't work, but should at some point in the future
-    // `return this._contextMenuEvent.composedPath() as HTMLElement[];`
-    // cf. (https://developer.mozilla.org/en-US/docs/Web/API/Event)
-    let node = this._contextMenuEvent.target as HTMLElement;
-    do {
-      if (test(node)) {
-        return node;
-      }
-      node = node.parentNode as HTMLElement;
-    } while (node.parentNode && node !== node.parentNode);
     return undefined;
   }

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

Or perhaps we want to filter event targets to be just html elements? return this._contextMenuEvent.composedPath().filter(x => x instanceof HTMLElement).find(test); or something...

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 1, 2019

Yeah, I think we should filter.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

Okay. I'm afk for a bit, but I'll check here when I'm back.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 1, 2019

Okay, updated.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

Nice! Does it actually work too? (I didn't test it...)

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 1, 2019

Hmm, it seems to have some problems.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 1, 2019

composedPath() is returning an empty array some of the time, for reasons I don't know. I may back out that last commit, as nice as it looks.

@ian-r-rose ian-r-rose force-pushed the context-menu-cleanup branch from 97231c2 to a0a0686 Feb 1, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

Okay, that's fine. Your version was pretty nice too. I liked the way you simplified it down to a do-while.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

I updated the comment to be more accurate in the context of this new code.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 1, 2019

Sounds good, thanks @jasongrout

@jasongrout jasongrout merged commit a496aad into jupyterlab:master Feb 1, 2019
7 of 11 checks passed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants