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 1 commit into from Dec 20, 2017

Conversation

Projects
None yet
4 participants
@jcsteh
Contributor

jcsteh commented Dec 1, 2017

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:

  1. With a select size="1" in single process Firefox, expanded with alt+downArrow, then tested that collapsing with alt+upArrow switches back to browse mode.
    • This doesn't work in multi process Firefox either before or after this change. That's due to Mozilla bug 1422176.
  2. Tested navigating in browse mode with the bitwarden and LastPass popups. Before this change, whenever the cursor hit a focusable element, browse mode would stop working. With the change, it doesn't.

Known issues with pull request:

None.

Change log entry:

Bug fixes:

- In Mozilla Firefox, browse mode now works correctly in pop-ups created by extensions such as LastPass and bitwarden. (#7809)
Fix browse mode in Firefox extension popups.
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.

@jcsteh jcsteh requested a review from michaelDCurran Dec 1, 2017

return None
if obj.role == controlTypes.ROLE_COMBOBOX:
return obj
return None

This comment has been minimized.

@derekriemer

derekriemer Dec 1, 2017

Collaborator

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

This comment has been minimized.

@derekriemer

derekriemer Dec 1, 2017

Collaborator

same NIT as below.

@jcsteh

This comment has been minimized.

Contributor

jcsteh commented Dec 1, 2017

michaelDCurran added a commit that referenced this pull request Dec 4, 2017

@michaelDCurran michaelDCurran merged commit 359fba3 into nvaccess:master Dec 20, 2017

@nvaccessAuto nvaccessAuto added this to the 2018.1 milestone Dec 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment