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
Fix browse mode in Firefox extension popups. #7809
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Firefox extensions can pop up documents to provide access to their functionality without creating a new browser tab. For example, both the LastPass and bitwarden password managers pop up a document allowing you to access your vault, add a site, etc. when you press their button on the toolbar. Previously, browse mode only worked in these documents when you were focused on the document itself. If you focused something inside the document (e.g. a link, a button or a text box), browse mode functionality ceased to work. This could happen even if you moved the browse mode cursor, so you would suddenly be "thrown out" of browse mode. This occurred because the Gecko vbuf code to handle combo box popups was being incorrectly used in this case. It only checked for Mozilla popup windows, but these popup documents are also popup windows. It walked to the nearest ancestor with a different HWND, but that meant that the document was skipped. Thus, anything inside the document was never considered to be part of the document. Now, the code to deal with combo box popups has been tightened so that it only matches those.
derekriemer
reviewed
Dec 1, 2017
return None | ||
if obj.role == controlTypes.ROLE_COMBOBOX: | ||
return obj | ||
return None |
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.
NIT: return None is just return
""" | ||
if not (obj.windowClassName.startswith('Mozilla') and obj.windowStyle & winUser.WS_POPUP): | ||
# This is not a Mozilla popup window, so it can't be an expanded combo box. | ||
return None |
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.
same NIT as below.
It's true that I can do just return instead of return None. However, I think it is clearer to be explicit here. The key point is that the caller expects and examines the return value, rather than there being no possibility of a return value (like a void return in C++). Think of it like return nullptr in C++, as opposed to just return (for a void).
|
michaelDCurran
approved these changes
Dec 4, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to issue number:
None.
Summary of the issue:
Firefox extensions can pop up documents to provide access to their functionality without creating a new browser tab. For example, both the LastPass and bitwarden password managers pop up a document allowing you to access your vault, add a site, etc. when you press their button on the toolbar. Currently, browse mode only works in these documents when you are focused on the document itself. If you focus something inside the document (e.g. a link, a button or a text box), browse mode functionality ceases to work. This can happen even if you move the browse mode cursor, so you are suddenly "thrown out" of browse mode.
Description of how this pull request fixes the issue:
This occurred because the Gecko vbuf code to handle combo box popups was being incorrectly used in this case. It only checked for Mozilla popup windows, but these popup documents are also popup windows. It walked to the nearest ancestor with a different HWND, but that meant that the document was skipped. Thus, anything inside the document was never considered to be part of the document.
This PR tightens the code to deal with combo box popups so that it only matches those.
Testing performed:
Known issues with pull request:
None.
Change log entry:
Bug fixes: