-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
Say all stops every 10th line in editors with NVDA #41423
Comments
This affects all Monaco editors, not just VSCode.
Bonus: if this issue is fixed (such that |
Background on this: #26730 Relevant lines in the code: textAreaHandler.ts:226 and textAreaState.ts:230 The Chromium performance issue which motivated introducing text paging in the first place seems to still exist. Details here (marked fixed, but I can still reproduce on latest Chromium) and here (submitted by @alexandrudima). I removed the paging limit on a local VSCode build, but this makes the editor unresponsive even with relatively small files (~1000 lines). Until the underlying Chromium issue is fixed, I don't see any way around this. |
I've (re-)reported the underlying issue to the Chromium bug tracker: https://bugs.chromium.org/p/chromium/issues/detail?id=868830 |
Thanks @dranov. Based on the Chromium ticket, it should hopefully be fixed soon. |
Update from the Chromium ticket:
To recap: we've prevented a performance regression from making it into Chrome stable, but the issue which affected VSCode is still there. I've reported steps to reproduce the underlying issue (which I've also reproduced on Chromium 61.0.3163.0, the version which VSCode uses internally), so hopefully it will get some attention now. |
Good news! The text metrics performance issue is now fixed in Chromium. The relevant commit is here. It might take a few months until the fix makes it into Chromium stable and then Electron. @alexandrudima, is there some way to run VSCode with a particular version of Chromium, so we can make sure this fixed the issue and text paging can now be removed? |
@dranov There is no easy way to run VSCode, as that requires that there exists an Electron version containing that Chromium version. The closest way to try this would be to run the monaco editor from sources (same source as VS Code) |
@alexandrudima, I've just tried it on a local build with text paging removed, and the Chromium fix definitely seems to improve things for Monaco as well. On Chromium 61.0.3163.0 (currently used by VS Code), the editor becomes visibly slow on files ~1k lines long. By contrast, Chromium 70.0.3530.0 starts showing similar slowness at ~10k lines. It does handle larger files (e.g. BigHTML in the Monaco tests, 24k lines), but at that point it's annoying to use. It's not ideal, but it might be good enough. |
@dranov Thank you for testing this! Once we update to a Chromium version >= 70.0.3530.0, we can raise the limits by an order of magnitude. I should also test if the current limits (10 lines per page, 500 chars before or after the selection) still make sense. Those were put it back when we used an older version, and perhaps we can raise them even on Chromium 61. Would you be willing to look into this and submit a PR to raise the limits? |
Sure, I'll take a closer look at this and submit a PR 😃 Also, considering the issue here ("reading through long files always stops on every 10th line"), besides increasing the current limits, it might be worth it to change the text paging strategy as well. The problem with the current strategy is that, even if you increase the limit to e.g. 100, if you are on line 100 in a larger file and ask NVDA to read until the end, NVDA will just read the current line and stop. Centring the page around the currently selected line might be better. @Neurrone, thoughts? I'll also try to understand how the Chromium accessibility stuff works -- it seems fundamentally slow, even when text metrics is turned off. |
@dranov Last time I tried with NVDA on Windows, centering the page around the current selection does not work, as it confuses screen readers terribly. For example, on every arrow down you end up removing the first line of text and adding a new line at the bottom. having an arrow key result in a text change causes terrible confusion to the screen reader which then fails to read out the line. |
I haven't had the chance to explore this in more depth yet (e.g. understand what's the root cause of the slowness in Chromium or figure out whether it's worth to hack NVDA to understand paging), but I'll dedicate some time for it next week and try to submit a PR. |
CC @jcsteh could you chime in on how NVDA implements paging for editables? |
We are currently using Chrome 69. Our progress on electron 6.0 adoption can be found here #75802 |
@isidorn Here is the code that determines what contents should be placed in the textarea. vscode/src/vs/editor/browser/controller/textAreaHandler.ts Lines 203 to 233 in 496d065
But we should really test this, because even if Chromium has improved in terms of textarea performance by one magnitude, today they run fine with ~10 lines, maybe they will run fine with ~100 lines... In other words, we should test with super duper large files before removing the limits completely. |
@alexandrudima thanks for the pointer. Yes, we can test it thouroughly once we adopt Electron 6.0.. |
We did not yet upgrade to Electron 6.0, thus pushing to next milestone. |
We will probably ship electron 6 in October, thus I will look into this in November once I am back from vacation. |
TLDR: set I just verified that the latest Chrome that we use still has the issue. And this is expected since https://bugs.chromium.org/p/chromium/issues/detail?id=580955 is still open. As a workaround we have introduced the following setting So anyone hitting this issue is advised to increase that setting to a larger number (default is 10). For example 1000 works just fine on my machine. @alexandrudima and me will measure if we can change the default value, to no longer be 10 but 100 for example. I am also thinking of having a heuristic to dynamically increase this setting to 100 if we detect a screen reader. Feedback on the setting name and anything else is very welcome. |
The setting name sounds fine, and I feel that 100 should be a safe default. This should be documented in the accessibility documentation for users with machines that can take a higher value, for example 1000. |
Thanks for feedback. I have added it to our Accessibility docs so users are aware of this. |
Please note that we have changed the This should provide a better out of the box experience. And due to that I plan to remove the tip from our accessibility docs that users need to configure this setting. |
This is the actual upstream issue https://bugs.chromium.org/p/chromium/issues/detail?id=1001591&q=10000%20chars&can=2 |
@isidorn, on that chromium issue page someone has commented that it is allegedly fixed now. Would this imply that when new version of chromium is included into vscode, we'd be able to set editor.accessibilityPageSize to infinity? |
@mltony in theory yes. In practice no since Chrome still has performance issues with large textArea. Once that get's fixed we will deprecate pageSize setting and always put the whole content of the editor. |
The performance issue is filed here: https://bugs.chromium.org/p/chromium/issues/detail?id=1076525 In comment 1 put where the problem/crazy-lag/timeout is happening. But we're going to need someone with AXPosition smarts to fix it. (My hopes that this was a simple problem were dashed.) |
This should work much better in the vscode insiders starting from Friday. So let me know if you see performance degradation starting from Friday. Thanks |
@isidorn Unfortunately, since this change was made, I am seeing a lot of lag when moving around the editor with arrow keys in large files (several hundred lines) while running the latest version of Jaws. We're talking about a good quarter second before Jaws speaks the active character. Setting editor.accessibilityPageSize to 100 resolves the issue. I was also able to reproduce this issue in NVDA, though it is a shorter lag. Interestingly, I don't notice the lag when navigating near the top of the file; it seems to get worse the further one moves down the file. |
@cannona thanks for letting us know. Let me reduce that to 500. Hopefully that works out better in practice... |
Extensions (4)
Steps to reproduce:
Expected behaviour
This should cause NVDA to start reading downwards from the current position to the end of the document.
Actual behaviour
NVDA does start reading from the current position, but stops once it reads line 10 when you start reading from lines <= 10.
I remember reading that VS Code's worked with screen readers based on "paging the text". I'm not sure what that means, could this be a side effect?
Impact
Reading through long files always stops on every 10th line, which is most noticeable when proofreading long files e.g .md or .tex files.
The text was updated successfully, but these errors were encountered: