Fix math not being read in Chromium-based browsers (#17421) #17548
Conversation
…ange that causes a call to (correctly) return E_NOTIMPL. This simple fix catches the exception and moves on. The behavior should be the same as before the Chromium change. Only E_NOTIMPL is caught, other COMErrors are re-raised. Fixes nvaccess#17421
WalkthroughThe pull request introduces an enhancement to error handling in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
source/NVDAObjects/IAccessible/ia2Web.py (2)
338-344: LGTM: Error handling implementation addresses the Chromium issue.The implementation correctly handles the E_NOTIMPL error while preserving other error cases, which solves the math reading issue in Chromium-based browsers.
Consider adding debug logging for the E_NOTIMPL case for consistency:
try: attr = node.attributesForNames(1, attrNames, namespaceIds) except COMError as e: if e.hresult != E_NOTIMPL: log.debugWarning(f"MathML getting attr error: {e}") raise + log.debug("MathML attributesForNames not implemented, falling back to default behavior") attr = None
338-344: Add explanatory comment for future maintainers.Consider adding a comment explaining why E_NOTIMPL is specifically handled, referencing the Chromium change that necessitated this fix.
+# Handle case where Chromium-based browsers return E_NOTIMPL for attributesForNames +# This was introduced in Chromium version X and affects math content reading try: attr = node.attributesForNames(1, attrNames, namespaceIds)user_docs/en/changes.md (2)
5-6: Improve clarity of patch release descriptionThe description should be more specific about what is being fixed. Consider:
-This is a patch release to fix bugs with braille devices and reading math in Chromium. +This is a patch release to fix bugs with braille devices and to fix math content not being read in Chromium-based browsers.
9-9: Fix formatting of bug fix entryThe bullet point for the math fix should be more descriptive and consistent with other entries:
-* Fixed bug with with reading math in Chromium Browsers (Chrome, Edge). (#17421, @NSoiffer) +* Fixed an issue where math content was not being read in Chromium-based browsers (Chrome, Edge). (#17421, @NSoiffer)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
source/NVDAObjects/IAccessible/ia2Web.py(2 hunks)user_docs/en/changes.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
source/NVDAObjects/IAccessible/ia2Web.py (2)
Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>
Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.
user_docs/en/changes.md (3)
Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>
Pattern **/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.
Pattern user_docs/en/changes.md: Ensure each change log entry references an issue or pull request number. Change log entries can also include a reference to a GitHub author. Examples of valid change log entries: * Item with sub-items (#123, @username): * sub-item * bar (#342) * Item with. Multiple lines. (#143)
🔇 Additional comments (1)
source/NVDAObjects/IAccessible/ia2Web.py (1)
15-15: LGTM: Import added for new error handling.
The addition of E_NOTIMPL import is appropriate for the new error handling in the Math class.
|
@seanbudd: thanks for doing that. For some reason, I always end up having trouble with forks. Sorry for all the bother. |
Note:: this is the same as #17528 but done on a non-master branch in my fork. According to @seanbudd, permissions to push in the fork don't work properly when coming from master, so here's a second attempt in case it makes life easier.
Fix math not being read in Chromium-based browsers. Chromium made a change that causes a call to (correctly) return E_NOTIMPL.
This simple fix catches the exception and moves on. The NVDA behavior should be the same as before the Chromium change. Only E_NOTIMPL is caught, other COMErrors are re-raised.
Fixes #17421.
This fixes a very serious user-issue. I recommend that a 2024.4.2 build happen with this fix if possible.
Link to issue number:
Fixes #17421
Summary of the issue:
Chromium-browsers were changed to return E_NOTIMPL and broke speech and braille of all math equations (i.e., no speech or braille for math) in those browsers because NVDA passes along a 'not implemented' COM exception.
Description of user facing changes
This restores the behavior of NVDA (i.e., math reads) as before the change.
Description of development approach
A try/except block is added around the call. If the error is E_NOTIMPL, the code moves on as before. Otherwise, the error is re-raised as before.
Testing strategy:
Make sure that some math addon is present. I tested with MathCAT, but any math addon will do. Go to any page with math (e.g., https://en.wikipedia.org/wiki/Quadratic_equation). Navigate to a math expression. Without the change, you will not hear any math. With the change, you will hear the math expression being read.
Known issues with pull request:
None.
Code Review Checklist:
I did not make a change log entry because I don't know what version this change will go into. I think a simple entry like
Summary by CodeRabbit
New Features
Bug Fixes