-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Fixed keydown event handling problem with shadow DOM. #12405
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
Fixed keydown event handling problem with shadow DOM. #12405
Conversation
When does this actually happen, since as-is there's not really enough information provided to make it possible to understand in exactly what situations these changes are needed. Please provide additional information, and preferably a runnable example such that it's possible to actually test/review these changes! Furthermore, given that the default viewer was originally written for usage in Firefox, it's somewhat difficult to even understand why these any changes are necessary here. For example, why can't these changes simply be included in a custom implementation of the viewer; please note http://mozilla.github.io/pdf.js/getting_started/#introduction (emphasis mine):
|
Sorry for the information missing. I am developing a translator extension for multiple browsers and I used pdf.js as a built-in pdf viewer in my extension. When I am trying injecting a shadow DOM with an input box into the pdf viewer, I found that I could not input space in the input box. When I clicked the space key, the keydown event was catched by pdf.js and resulted in page scrolling instead of inputting a space in the input box. After digging into the code I found that although pdf.js had considered the input case, the original solution could not handle DOMs with shadow DOM element. I also found that the problem with text editors has been discussed in #7156 and #11253 provided the incomplete solution. So I thought that maybe you need a patch for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an easy way to verify for us that this works? The code itself looks good with the comments addressed.
web/ui_utils.js
Outdated
/** | ||
* Get the active or focused element in current DOM. | ||
* | ||
* Recursively search for the truly active or focused element incase there are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incase
-> in case
web/ui_utils.js
Outdated
* Recursively search for the truly active or focused element incase there are | ||
* shadow DOMs. | ||
* | ||
* @author Chen Jinlong <[email protected]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never use the @author
tag in the codebase, so this should be removed.
web/ui_utils.js
Outdated
* | ||
* @author Chen Jinlong <[email protected]> | ||
* | ||
* @param {DocumentOrShadowRoot} root the root element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param {DocumentOrShadowRoot} [root] - The root element.
root
should be in brackets to indicate that it's an optional argument.
We can also leave the entire root
argument out since no code currently calls this function with an argument?
web/ui_utils.js
Outdated
* @author Chen Jinlong <[email protected]> | ||
* | ||
* @param {DocumentOrShadowRoot} root the root element. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This newline can be removed.
Sorry for the late replying. I'll fix the comment problems and make a package for you to verify the patch. |
Editable elements in shadow DOMs can not be detected in old version.
07a34f0
to
252e258
Compare
I made 2 packages for you to verify the patch. One is built from the fixed branch and the other is built from the original branch. Download the packages and extract them to somewhere in you computer respectively. Follow the following steps to run the packages. Run original:
Run fixed: Steps 1-4 is the same (replace Here are the packages: fixed: fixed.zip original: original.zip |
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/202ae21a71a27c7/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/202ae21a71a27c7/output.txt Total script time: 3.58 mins Published |
Seems to work just fine. Thanks! |
Editable elements in shadow DOMs can not be detected in old version.