Allow selection in the a11y tree and sync the selection to terminal #4742
Conversation
|
Hi @Tyriar, can you take a look at this. A few notes:
|
|
Hi @Tyriar, can you take a look at this? Or can you let me know how I can help to get this merge quicker? |
Previously, for string 'ab好', outColumns=[0, 1, 2]. Now it becomes [0, 1, 2, 4]. This allows the user to know where does the last character ends.
There was a problem hiding this comment.
Seems to work 👍
For tests, yes it is a little tricky to get them set up for this as it's quite entangled with the main Terminal object (cyclic dependency). If you want to do a follow up for tests which would be greatly appreciated, you could just turn this into an exported function and test that, instead of trying to test the method on AccessibilityManager.
|
FYI @meganrogge, this should allow screen readers to select text in the underlying textarea. I haven't tested it much so we'll want to keep an eye out in Insiders. |
| if (outColumns) { | ||
| for (let i = 0; i < chars.length; ++i) { | ||
| outColumns.push(startCol); | ||
| } | ||
| } |
There was a problem hiding this comment.
Just stumbled over this code change and blamed it back to here.
This looks like a serious perf smell to me - note that this function is hot-path for handlers grabbing line contents over and over.
Ideally hot-path functions are as branching/inner-loop free as possible, and never change the inner works/touched variables to not get de-optimized from JIT. This change adds all of that.
Note that I have not perf tested it yet, so its just a "looks-alike" concern from my side atm.
@Tyriar FYI
There was a problem hiding this comment.
I played with it a bit and this did not seem to be called much during normal rendering. It is called quite a lot by the SearchAddon (when you are using search) though, but it is unclear to me whether it actually affect performance.
There was a problem hiding this comment.
Yeah that method is not called during normal data ingestion & rendering (basic operations). But, as you already found, it gets used by several 2nd-line operations, which do buffer scraping alot, like search, serialize, in general things operating on the buffer API to grab the text repr. While those actions are rarely triggered, they most likely run over bigger bufferlines batches creating some runtime pressure for a short amount of time (potentially leading to hiccups).
Will see if I find the time to perf it - tricky part here prolly is, that the de-opt might only show up, if calls alternate between outColumns being set and unset. Not sure yet.
Fixes #4656