-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Improve firefox support #2476
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
Improve firefox support #2476
Conversation
This reverts commit d00345f. This is not (and will not be) supported by Firefox.
This does NOT fix wrapping, only catches errors
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.
Hi, @mrmr1993. Could you check my two small comments, please. Then I'll merge.
content_scripts/mode_find.coffee
Outdated
try | ||
result = window.find(query, options.caseSensitive, options.backwards, true, false, true, false) | ||
catch # Failed searches throw on Firefox. | ||
options.postFindFocus?.focus() |
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 the indentation correct here? Looks like XXX.focus()
should happen only if the exception is raised.
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.
Yes. window.find
in Firefox focuses the window that we call it on, so this gives us an opportunity to refocus the HUD window.
Should I add a newline between the try ... catch
and this line, to clearly separate them?
pages/content_script_loader.coffee
Outdated
@@ -0,0 +1,28 @@ | |||
injectContentScripts = -> |
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 this file actually being used?
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.
No, sorry. The revert commit reintroduced it and I didn't notice. I'll remove it
@smblott-github I've pushed some commits to address your comments. |
This merges all of the code mentioned in my comment on #2425.
As listed there:
Also, there are still issues with focus in the find mode HUD.
(Branch hud-focus seems to improve the situation, by using separate persistent elements for normal HUD and the HUD find input. Not sure if this is a direction we want to take.)