Add iframe search with debounce #125856

rzhao271 · 2021-06-09T15:30:31Z

This PR fixes #96307 by adding an iframe search prototype. It uses electron/electron#28274 to search in the iframe, which results in a loss of focus during the search. Therefore, I added an event to re-focus the inputbox after find results are returned, and also added a debounce so that the focus-changing behaviour of the find call is not too obvious.

Preview:

Issues:

The jumping search problem remains (Support search in iframe based webview #96307 (comment)). This issue can be fixed by using a BrowserView, in which case, the debounce can be taken off as well (Support search in iframe based webview #96307 (comment)).

rzhao271 · 2021-06-09T15:56:35Z

An issue with the CI seems to be that it's not taking in the proper Electron.
Even locally, I get the same compile errors, but with the right Electron, the code still runs properly.

rzhao271 · 2021-06-09T21:09:21Z

Verified that in the case of an IME (e.g. for Mandarin), the search must be made explicit, e.g. by pressing enter, otherwise focus is lost from the IME. CC @rebornix

Meanwhile when using NVDA, the readout is very different than in stable, or in the Monaco editor. In the editor, NVDA starts reading the document from the active match. In stable, NVDA only reads what is being typed in the find box. With this PR, when searching in an iframe, NVDA sometimes says "virtual document" and starts reading the iframe from the beginning, due to focus being lost from the inputbox for long enough to trigger the screenreader. Other times, NVDA only reads what is typed. @isidorn I'm wondering how to make the search behaviour more accessible here?

mjbvz

Code changes look good overall. Nice work! Just a few minor comments

src/vs/platform/webview/electron-main/webviewMainService.ts

src/vs/workbench/contrib/webview/electron-sandbox/iframeWebviewElement.ts

src/vs/platform/webview/electron-main/webviewMainService.ts

bpasero

👏 👏 👏

Gave some feedback inline. Also found one potential issue: I cannot hit Cmd+X to cut/clear the input of the find input box. Also Cmd+C and Cmd+V do not seem to work there, maybe something that is still missing?

product.json

src/vs/platform/webview/electron-main/webviewMainService.ts

src/vs/workbench/contrib/webview/electron-sandbox/iframeWebviewElement.ts

isidorn · 2021-06-10T07:27:25Z

@rzhao271 thanks for the ping. I see there are already quite some discussions here so for accessibility I think the best would be that once we merge this in you create an issue for accessibility and we look into that together. Once this is merged it will also be easier for me to try it out.
Let me know if that works for you? Thanks

rzhao271 · 2021-06-10T17:16:55Z

@bpasero I can confirm that the cut, copy, and paste commands don't work, but I'm not sure why, yet. I notice there's this event handler that is run when I type Ctrl+X, but I'm not sure if it interferes in any way https://github.com/microsoft/vscode/blob/main/src/vs/base/browser/ui/findinput/findInput.ts#L215. @mjbvz do you have any insights on this?

Also, with the IME issue, if we switch webviews to iframes on Insiders, searching will fail for anyone using an IME (Chinese, Japanese, Korean languages require these, for example). Therefore, I think we can merge this to Insiders, but I wouldn't set iframes to be used by default for now.

@isidorn I can create an issue for accessibility after this PR gets merged. I think the IME issue is another blocker, though.

Edit: Fixed the IME issue:

Edit again: The active match should be highlighted orange. I'm not sure why the match colour seems to alternate between yellow and orange above.

rzhao271 · 2021-06-10T22:07:27Z

Another issue: OSS seems to crash when I do a search in the preview view, but then change the content in the source markdown file.

rzhao271 · 2021-06-10T23:05:56Z

The match colour alternating between yellow and orange is an artifact of the find-in-frame implementation:

The crash described in #125856 (comment) is irregular, though, and requires I make a crashdump.

bpasero

🚢 it

bpasero · 2021-06-11T06:15:12Z

Another issue: OSS seems to crash when I do a search in the preview view, but then change the content in the source markdown file.

What are the steps to get to this crash? When you say OSS you mean running with our internal builds?

rzhao271 · 2021-06-18T19:23:33Z

I confirmed the iframe search crash occurs around the time the html method of the base webview is called, which posts the following message:

[8916:0618/120706.592:INFO:CONSOLE(73)] "posting message", source: file:///C:/Users/raymondzhao/work/vscode-11/out/vs/workbench/contrib/webview/electron-sandbox/iframeWebviewElement.js (73)
[8916:0618/120706.592:INFO:CONSOLE(74)] "channel: content", source: file:///C:/Users/raymondzhao/work/vscode-11/out/vs/workbench/contrib/webview/electron-sandbox/iframeWebviewElement.js (74)
[8916:0618/120706.592:INFO:CONSOLE(75)] "data: {"contents":"<!DOCTYPE html>\n\t\t\t<html style=\"--markdown-font-family: -apple-system, BlinkMacSystemFont, 'Segoe WPC', 'Segoe UI', system-ui, 'Ubuntu', 'Droid Sans', sans-serif; --markdown-font-size: 14px; --markdown-line-height: 1.6;\">\n\t\t\t<head>\n\t\t\t\t<meta http-equiv=\"Content-type\" content=\"text/html;charset=UTF-8\">\n\t\t\t\t<meta http-equiv=\"Content-Security-Policy\" content=\"default-src 'none'; img-src 'self' https://*.vscode-webview.net https: data:; media-src 'self' https://*.vscode-webview.net https: data:; script-src 'nonce-1624043226590590'; style-src 'self' https://*.vscode-webview.net 'unsafe-inline' https: data:; font-src 'self' https://*.vscode-webview.net https: data:;\">\n\t\t\t\t<meta id=\"vscode-markdown-preview-data\"\n\t\t\t\t\tdata-settings=\"{&quot;source&quot;:&quot;file:///c%3A/Users/raymondzhao/work/emmet-work/testworkspace3/test.md&quot;,&quot;line&quot;:0,&quot;lineCount&quot;:1,&quot;scrollPreviewWithEditor&quot;:true,&quot;scrollEditorWithPreview&quot;:true,&quot;doubleClickToSwitchToEditor&quot;:true,&quot;disableSecurityWarnings&quot;:false,&quot;webviewResourceRoot&quot;:&quot;https://file%2B.vscode-resource.vscode-webview.net/c%3A/Users/raymondzhao/work/emmet-work/testworkspace3/test.md&quot;}\"\n\t\t\t\t\tdata-strings=\"{&quot;cspAlertMessageText&quot;:&quot;Some content has been disabled in this document&quot;,&quot;cspAlertMessageTitle&quot;:&quot;Potentially unsafe or insecure content has been disabled in the Markdown preview. Change the Markdown preview security setting to allow insecure content or enable scripts&quot;,&quot;cspAlertMessageLabel&quot;:&quot;Content Disabled Security Warning&quot;}\"\n\t\t\t\t\tdata-state=\"{&quot;resource&quot;:&quot;file:///c%3A/Users/raymondzhao/work/emmet-work/testworkspace3/test.md&quot;,&quot;line&quot;:0,&quot;imageInfo&quot;:[],&quot;resourceColumn&quot;:1,&quot;locked&quot;:false}\">\n\t\t\t\t<script src=\"https://file%2B.vscode-resource.vscode-webview.net/c%3A/Users/raymondzhao/work/vscode-11/extensions/markdown-language-features/media/pre.js\" nonce=\"1624043226590590\"></script>\n\t\t\t\t<link 
rel=\"stylesheet\" type=\"text/css\" href=\"https://file%2B.vscode-resource.vscode-webview.net/c%3A/Users/raymondzhao/work/vscode-11/extensions/markdown-language-features/media/markdown.css\">\n<link rel=\"stylesheet\" type=\"text/css\" href=\"https://file%2B.vscode-resource.vscode-webview.net/c%3A/Users/raymondzhao/work/vscode-11/extensions/markdown-language-features/media/highlight.css\">\n<link rel=\"stylesheet\" type=\"text/css\" href=\"https://file%2B.vscode-resource.vscode-webview.net/c%3A/Users/raymondzhao/work/vscode-11/extensions/markdown-math/node_modules/katex/dist/katex.min.css\">\n<link rel=\"stylesheet\" type=\"text/css\" href=\"https://file%2B.vscode-resource.vscode-webview.net/c%3A/Users/raymondzhao/work/vscode-11/extensions/markdown-math/preview-styles/index.css\">\n\t\t\t\n\t\t\t<style>\n</style>\n\n\t\t\t\t<base href=\"https://file%2B.vscode-resource.vscode-webview.net/c%3A/Users/raymondzhao/work/emmet-work/testworkspace3/test.md\">\n\t\t\t</head>\n\t\t\t<body class=\"vscode-body scrollBeyondLastLine wordWrap 
showEditorSelection\">\n\t\t\t\t<p data-line=\"0\" class=\"code-line\">Some texta</p>\n\n\t\t\t\t<div class=\"code-line\" data-line=\"1\"></div>\n\t\t\t\t<script async\n\t\t\t\tsrc=\"https://file%2B.vscode-resource.vscode-webview.net/c%3A/Users/raymondzhao/work/vscode-11/extensions/markdown-language-features/media/index.js\"\n\t\t\t\tnonce=\"1624043226590590\"\n\t\t\t\tcharset=\"UTF-8\"></script>\n\t\t\t</body>\n\t\t\t</html>","options":{"enableFindWidget":true,"allowScripts":true,"localResourceRoots":[{"$mid":1,"fsPath":"c:\\Users\\raymondzhao\\work\\vscode-11\\extensions\\markdown-language-features","_sep":1,"external":"file:///c%3A/Users/raymondzhao/work/vscode-11/extensions/markdown-language-features","path":"/c:/Users/raymondzhao/work/vscode-11/extensions/markdown-language-features","scheme":"file"},{"$mid":1,"fsPath":"c:\\Users\\raymondzhao\\work\\vscode-11\\extensions\\markdown-math","_sep":1,"external":"file:///c%3A/Users/raymondzhao/work/vscode-11/extensions/markdown-math","path":"/c:/Users/raymondzhao/work/vscode-11/extensions/markdown-math","scheme":"file"},{"$mid":1,"external":"file:///c%3A/Users/raymondzhao/work/emmet-work/testworkspace3","path":"/c:/Users/raymondzhao/work/emmet-work/testworkspace3","scheme":"file"}]},"state":"{\"resource\":\"file:///c%3A/Users/raymondzhao/work/emmet-work/testworkspace3/test.md\",\"line\":0,\"imageInfo\":[],\"resourceColumn\":1,\"locked\":false,\"scrollProgress\":0}","cspSource":"https://*.vscode-webview.net"}", source: file:///C:/Users/raymondzhao/work/vscode-11/out/vs/workbench/contrib/webview/electron-sandbox/iframeWebviewElement.js (75)

I tried adding some code that stopped the find before that message was posted, but the crash continued to occur.

bpasero · 2021-06-19T06:54:28Z

👍 , I have extracted #126729 for the crash. I found a second case where I can reproduce a crash.

Fixes #96307

rzhao271 · 2021-06-23T23:23:36Z

I have rebased the changes on top of Deepak's FindRequestManager fix, which now avoids the crash described in #126729.

I also added on a commit that adds on onDidStopFind event, because the find could be stopped by a content update, in which case, focus should stay in the editor (previously, focus would always go back to the search box).

src/vs/workbench/contrib/codeEditor/browser/find/simpleFindWidget.ts

rzhao271 requested review from bpasero and mjbvz June 9, 2021 15:30

rzhao271 self-assigned this Jun 9, 2021

mjbvz requested changes Jun 10, 2021

View reviewed changes

bpasero added this to the June 2021 milestone Jun 10, 2021

bpasero requested changes Jun 10, 2021

View reviewed changes

rzhao271 requested review from bpasero and mjbvz June 10, 2021 19:30

rebornix approved these changes Jun 11, 2021

View reviewed changes

bpasero approved these changes Jun 11, 2021

View reviewed changes

bpasero mentioned this pull request Jun 19, 2021

Iframe webview: search can crash the process #126729

Closed

mjbvz and others added 8 commits June 22, 2021 15:47

Add search for iframe based webviews

76ed52e

Fixes #96307

Update

a2ccdbf

Fix up webviewManagerService

67b604a

Add debounce

afb53dd

Clear productline

d23ca15

Fix event handling

acf8bc4

Check IME completion

f6df43a

Fix focus being stolen from editor during content update

4da1d00

rzhao271 force-pushed the dev/mjbvz/iframe-search branch from c4b12a4 to 4da1d00 Compare June 23, 2021 23:20

rzhao271 commented Jun 24, 2021

View reviewed changes

src/vs/workbench/contrib/codeEditor/browser/find/simpleFindWidget.ts Outdated Show resolved Hide resolved

Fix IME completion condition

f324443

rzhao271 merged commit 9a8a439 into main Jun 24, 2021

rzhao271 deleted the dev/mjbvz/iframe-search branch June 24, 2021 18:49

github-actions bot locked and limited conversation to collaborators Aug 8, 2021

Add iframe search with debounce #125856

Add iframe search with debounce #125856

Uh oh!

Conversation

rzhao271 commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rzhao271 commented Jun 9, 2021

Uh oh!

rzhao271 commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjbvz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isidorn commented Jun 10, 2021

Uh oh!

rzhao271 commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rzhao271 commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rzhao271 commented Jun 10, 2021

Uh oh!

bpasero left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpasero commented Jun 11, 2021

Uh oh!

rzhao271 commented Jun 18, 2021

Uh oh!

bpasero commented Jun 19, 2021

Uh oh!

rzhao271 commented Jun 23, 2021

Uh oh!

Uh oh!

Uh oh!

rzhao271 commented Jun 9, 2021 •

edited

Loading

rzhao271 commented Jun 9, 2021 •

edited

Loading

rzhao271 commented Jun 10, 2021 •

edited

Loading

rzhao271 commented Jun 10, 2021 •

edited

Loading

bpasero left a comment •

edited

Loading