Skip to content
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

Find in webview based views (html preview, release notes, extension editor) #30016

Merged
merged 13 commits into from Jul 7, 2017

Conversation

hoovercj
Copy link
Member

@hoovercj hoovercj commented Jul 2, 2017

@mjbvz this PR replaces PR #22918 to address issue #2187.

I noticed that @Tyriar had essentially created a "simple find widget" to use in the terminal which was much simpler to reuse than trying to refactor the full "find widget".

I refactored that out into a reusable widget (SimpleFindWidget) and updated the terminal to use it (TerminalFindWidget).

I then created a WebviewFindWidget to be used by the webview to interact with the native webview find api. To respect the layering contract, I had to make the existing WebviewEditor into an abstract class and extend it in the correct layer so it could have access to the webview. HtmlPreviewPart and ReleaseNotesEditor now extend from that new WebviewEditor implementation and therefore both have access to the new find widget.

ExtensionEditor also now leverages the simple find widget to enable searching through readmes and release notes within the extension editor view.

Finally, I improved the styling of the simple find widget by adding "overflow: hidden" to the containers that use it so that it can properly be animated in.

Caveat: I don't know what HtmlEditorZone is so I didn't test that.

Here are gifs of it in action:

This is the find working in a markdown preview:
simplefindpreview

This is the find working in release notes
simplefindreleasenotes

This is the find working the same as before in the terminal, but now with animation:
simplefindterminal

This is the find working in the webview sections of the ExtensionEditor
simplefindextensions

@hoovercj hoovercj changed the title Html preview/find/find input based Find in webview based views (html preview, release notes, extension editor) Jul 2, 2017
@mjbvz mjbvz requested review from Tyriar, rebornix and mjbvz July 3, 2017 19:37
}
});
this.webview.style(this.themeService.getTheme());
this._themeChangeSubscription = this.themeService.onThemeChange(this.onThemeChange);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it matters here, but bind this for this.onThemChange

Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Webview parts of the change look good to me. Very excited to get this functionality in.

@rebornix @Tyriar should be able to provide more feedback on the code that was extracted from the terminal

@Tyriar
Copy link
Member

Tyriar commented Jul 5, 2017

@mjbvz FYI the plan with the terminal one is to bring it on par with the editor's functionality eventual #28768

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty cool! 😮 terminal side looks good to me. @rebornix should also check it out since he did the find ui in the terminal.

@Tyriar
Copy link
Member

Tyriar commented Jul 5, 2017

I think this also closes #29795

@hoovercj
Copy link
Member Author

hoovercj commented Jul 5, 2017

@Tyriar @mjbvz the webview find api supports case sensitivity and searching only from the start of the word (or treating capital letters inside words as the start of a word) for whole word search but not regex.

It also gives info about the number of results and the position of the current result (shown in the closed PR).

This should make it easy to keep the webview find at feature parity with the terminal find and/or reach feature parity with the normal find. I'd also be happy to help with that

@rebornix
Copy link
Member

rebornix commented Jul 5, 2017

Issues found while playing with the code

Find Previous button doesn't work when invoking second time in webiew editor

  • Open markdown preview, side by side
  • Invoke Find Widget, type something existed in the document
  • Find Next/Previous, then dismiss the find widget
  • Invoke Find Widget again
  • Click Find Previous button, nothing happens

The first time when we invoke Find Widget and type, we have automatic searching. Whi.le if we dismiss it and then invoke again, it doesn't automatically research the document. Sometimes it even raises null reference exception.

Esc should dismiss the Find Widget when the widget loses focus in webview editor

  • Invoke Find Widget in Markdown Preview or Extension Page
  • Click the webview, Find widget loses focus
  • Press esc, it doesn't closes the find widget

Right now in both editor and terminal, pressing Esc will close the find widget.

Enter in Find input can only search once after launching the second

This is different from the first issue. This time we always put the focus in find input box.

  • Open an extension detail view, for example, Vim
  • Invoke Find Widget, search e
  • Press enter multiple times, cycle through find results
  • Esc --> dismiss the widget
  • Invoke Find Widget again
  • Press enter, the next result is focused. From now on, pressing enter no longer works. Neither does shift-enter.

Lastly, it seems webview doesn't have any UI state persistence, switching to another editor and then switching back loses the Find Widget, but it's likely an existing issue. @mjbvz

@rebornix
Copy link
Member

rebornix commented Jul 5, 2017

In overall it looks pretty neat and it's really useful :) Everything can be searchable.

@Tyriar
Copy link
Member

Tyriar commented Jul 5, 2017

The build is also failing:

{"errorCode":"load","moduleId":"vs/workbench/parts/html/browser/WebviewEditor","neededBy":["vs/workbench/parts/update/electron-browser/releaseNotesEditor"],"detail":{"errno":-2,"code":"ENOENT","syscall":"open","path":"out-build/vs/workbench/parts/html/browser/WebviewEditor.js"}}

@mjbvz
Copy link
Contributor

mjbvz commented Jul 5, 2017

@rebornix Yes webviews themselves are not persistent by design. The htmlEditor which holds the webview is persisted so we should be able to save search state there and then restore it when the webview is shown again. Webview search persistence is not a blocking issue for this PR in my mind

@hoovercj
Copy link
Member Author

hoovercj commented Jul 5, 2017

@rebornix good finds, I should be able to iron out those issues.

@Tyriar unfortunately that error message isn't very useful to me. Without knowing more about the build system, the best I can do is "works on my machine".

@Tyriar
Copy link
Member

Tyriar commented Jul 5, 2017

@hoovercj the module can't be found so I suspect you need to register vs/workbench/parts/html/browser/WebviewEditor inside workbench.main.ts.

@hoovercj
Copy link
Member Author

hoovercj commented Jul 5, 2017

@Tyriar thanks for the pointer, hopefully I registered it correctly now. I also noticed that "ctrl+f" with the terminal find already opened and focus in the textinput would select the input. It would NOT do so if the focus was on the buttons. I did a bit more refactoring so that the webview find would do that too, and also so that both of them would work if it was pressed with the buttons focused. This aligns with the normal find widget.

@rebornix

"The first time when we invoke Find Widget and type, we have automatic searching. Whi.le if we dismiss it and then invoke again, it doesn't automatically research the document. Sometimes it even raises null reference exception."

The current iteration still will not begin searching when you reactivate the find widget until you start typing or press enter. This doesn't exactly match the editor find widget, but it matches Chrome's find and it is due to the way the webview find api works. The webview find api doesn't have a way to find without moving. I think when you see how it behaves in practice, though, you will see that the behavior is fine. However, if you are still seeing a null reference, please let me know :-)

The other two issues you mentioned should also be fixed.

Cody Hoover added 2 commits July 6, 2017 01:58
…ensitive). Removed registration from workbench to see if it was necessary at all. If it fails again, I'll add it back
@hoovercj
Copy link
Member Author

hoovercj commented Jul 6, 2017

[07:45:10] Error: /home/travis/build/Microsoft/vscode/src/vs/workbench/parts/preferences/browser/preferencesEditor.ts(132,79): Argument of type 'typeof SearchWidget' is not assignable to parameter of type 'AsyncDescriptor2<HTMLElement, { ariaLabel: string; placeholder: string; navigateByArrows: boolean...'.
  Property 'moduleName' is missing in type 'typeof SearchWidget'.

any idea what could cause this? I haven't touched this file

@Tyriar
Copy link
Member

Tyriar commented Jul 6, 2017

@hoovercj Looks like an unrelated failure on master, try merging from master again.

@hoovercj
Copy link
Member Author

hoovercj commented Jul 6, 2017

@Tyriar I guess you're right. Green :-)

@mjbvz mjbvz merged commit bf5eac1 into microsoft:master Jul 7, 2017
@mjbvz mjbvz added this to the July 2017 milestone Jul 7, 2017
@mjbvz
Copy link
Contributor

mjbvz commented Jul 7, 2017

Thanks @hoovercj! This should be in the first VSCode 1.15 insiders build which should come out early next week after we ship 1.14

@hoovercj
Copy link
Member Author

hoovercj commented Jul 7, 2017

Great, I can't wait @mjbvz! And if there are bugs/issues associated with it, please do tag me in them and I'll try to fix them, or at the very least I'll be shamed into doing better next time :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants