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

Create FindOnPage.md #4399

Open
wants to merge 64 commits into
base: main
Choose a base branch
from
Open

Create FindOnPage.md #4399

wants to merge 64 commits into from

Conversation

maxwellmyers
Copy link

This is a review for the new FindOnPage API.

@maxwellmyers maxwellmyers added the API Proposal Review WebView2 API Proposal for review. label Feb 27, 2024
@maxwellmyers maxwellmyers marked this pull request as ready for review February 27, 2024 21:24
FindOnPage.md Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
Updated supress def dialog and should highlight
Updated stopfind description/documentation
updated find next and previous
start find async
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
Added info to startfind, added /// for documentation of matchcountchanged events, addressed case sensitivity question
Copy link

@ajtruckle ajtruckle left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough worked examples. Looking forward to the official release!

Copy link
Contributor

@david-risney david-risney left a comment

Choose a reason for hiding this comment

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

Please fix issues

FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Outdated Show resolved Hide resolved
FindOnPage.md Show resolved Hide resolved
maxwellmyers and others added 4 commits May 13, 2024 16:39
Co-authored-by: David Risney <dave@deletethis.net>
Co-authored-by: David Risney <dave@deletethis.net>
removed c# section
FindOnPage.md Outdated Show resolved Hide resolved
updated midl3 definition
CoreWebView2FindDirection FindDirection { get; set; };

/// Determines if the find operation is case sensitive. Returns TRUE if the find is case sensitive, FALSE otherwise.
/// The locale used to determine case sensitivity is the document's language specified by the HTML lang attribute, or if unspecified then the WebView2's UI locale.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The locale used to determine case sensitivity is the document's language specified by the HTML lang attribute, or if unspecified then the WebView2's UI locale.
/// The locale used to determine case sensitivity is the document's language specified by the HTML lang attribute. If unspecified then the WebView2's UI locale

Punctuation at end of line interrupted the sentence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.


/// Gets or sets the state of whether all matches are highlighted.
/// Returns TRUE if all matches are highlighted, FALSE otherwise.
/// Note: Changes to this property take effect only when StartFind, FindNext, or FindPrevious is called.
Copy link
Contributor

Choose a reason for hiding this comment

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

This claims that Next()/Previous() calls will pick up changes to this property, but below it says "To change the ongoing find session, StartFindAsync must be called again with a new or modified configuration object.", which indicates changing the property between Next() calls is ignored. Which is correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix. This property is not live. Update docs to say correct info.

/// If called with no Find session active, it will silently do nothing.
void StopFind();

/// Retrieves the index of the currently active match in the find session. Returns the index of the currently active match, or -1 if there is no active match.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this repo support IReference for "may not exist" primitives, instead of a sentinel value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave as is. Discussion: We can support nullable / ireference with a bunch of additional work but don't find it to be ROI worthwhile

/// To change the ongoing find session, StartFindAsync must be called again with a new or modified configuration object.
/// StartFind supports, HTML, PDF, and TXT document queries. In general this api is designed for text based find sessions.
//// If you start a find session programmatically on another file format that doesnt have text fields, the find session will try to execute but will fail to find any matches. (It will silently fail)
/// Note: The asynchronous action completes when the UI has been displayed with the find term in the UI bar, and the matches have populated on the counter on the find bar.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the completion of the IAsyncAction the point at which calls to Next()/Previous()/Stop() methods become allowed? What happens if I call Next() before this IAsyncAction completes, is it the same "silently fail" as when there's no active session?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the completion of the IAsyncAction the point at which calls to Next()/Previous()/Stop() methods become allowed?

Yes

What happens if I call Next() before this IAsyncAction completes, is it the same "silently fail" as when there's no active session?

Yes

Please make sure this is documented in the section about Start and what its async completion means.


#### Description

To initiate a find operation in a WebView2 control, use the `StartFind` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we pre-committed to using "Find" as both a noun and a verb in this API, e.g. because of industry standards or existing webview2 code?

"Find" is commonly a verb in API method names.

Here "Find" is will be a noun twice in coreWebView2.Find.StartFindAsync(..., and a noun and verb once each in coreWebView2.Find.FindNext().

Copy link

Choose a reason for hiding this comment

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

I also find the naming awkward here. I think we should consider something like FindOperation or FindSession over Find.

Copy link
Contributor

Choose a reason for hiding this comment

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

The old API (Microsoft internal link) called it a "find session", initiated by a "find request".

Copy link
Contributor

Choose a reason for hiding this comment

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

I think part of the awkwardness is usage of the word "find" rather than "search"; "search" as both a noun and a verb is more common, at least in en-us. (But "find" is the right name for this feature)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update:

  • CoreWebView2.Find -> CoreWebView2.FindSession
  • StartFindAsync -> StartAsync
  • StopFind -> Stop

(but leave FindNext/Previous as is with Find)


#### Description

To initiate a find operation in a WebView2 control, use the `StartFind` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "Find" be consistently capitalized in this document when it's a noun?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix after renaming property name its now FindSession (including casing) throughout doc.

### Retrieve the Index of the Active Match

#### Description
Developers can retrieve the index of the currently active match
Copy link
Contributor

Choose a reason for hiding this comment

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

Will developers need information about the current found word instance other than its ActiveMatchIndex? For example what longer string it's a part of?

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above we don't support this yet but can look at it for the future.


```cpp

//! [InitializeFindConfiguration]
Copy link

Choose a reason for hiding this comment

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

Each example has a comment with this attribute-like syntax. Is this intended to signify something?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's used to extract the example code into a doc somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove from doc.

### Setting Up Find Configuration with MIDL3

### CoreWebView2 Find Configuration and Direction

Copy link

Choose a reason for hiding this comment

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

Are these sections missing? Or can these headings be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove.

CoreWebView2FindConfiguration CreateFindConfiguration();
};

runtimeclass CoreWebView2FindConfiguration : [default]ICoreWebView2FindConfiguration {}
Copy link

Choose a reason for hiding this comment

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

fix indent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

CoreWebView2FindConfiguration CreateFindConfiguration();
};

runtimeclass CoreWebView2FindConfiguration : [default]ICoreWebView2FindConfiguration {}
Copy link

Choose a reason for hiding this comment

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

Since this is midl 3, if the only implementation of the interface is this runtimeclass, you can just define the runtimeclass and the matching interface will be defined implicitly.

Copy link

Choose a reason for hiding this comment

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

Same comment with regards to CoreWebView2Find/ICoreWebView2Find.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please validate that the generated MIDL3 doesn't have this interface as public.

```cpp

//! [InitializeFindConfiguration]
wil::com_ptr<ICoreWebView2FindConfiguration> AppWindow::InitializeFindConfiguration(const std::wstring& findTerm)
Copy link

Choose a reason for hiding this comment

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

I assume that AppWindow here just refers to the class that this sample code belongs to? I initially thought it had something to do with the Microsoft.UI.Windowing.AppWindow class in WinAppSDK. Consider renaming to something else to avoid confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow up with me to ensure we open a bug to change this in our sample app code.



/// Adds an event handler for the `ActiveMatchIndexChanged` event.
/// Registers an event handler for the ActiveMatchIndexChanged event. This event is raised when the index of the currently active match changes. This can happen when the user navigates to a different match or when the active match is changed programmatically. The parameter is the event handler to be added. Returns a token representing the added event handler. This token can be used to unregister the event handler.
Copy link

Choose a reason for hiding this comment

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

Add line breaks to keep the width of the lines at a reasonable length, e.g. 80 chars or 120 chars.

Copy link

Choose a reason for hiding this comment

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

Similarly with the rest of the long lines in this section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

/// Displays the Find bar and starts the find operation. If a find session was already ongoing, it will be stopped and replaced with this new instance.
/// If called with an empty string, the Find bar is displayed but no finding occurs. Changing the configuration object after initiation won't affect the ongoing find session.
/// To change the ongoing find session, StartFind must be called again with a new or modified configuration object.
/// This method is primarily designed for HTML document queries.
Copy link

Choose a reason for hiding this comment

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

This method is primarily designed for HTML document queries.

I don't understand what this comment means.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

This comment is out of date. The comment on the MIDL3 StartFindAsync is more up to date.


/// Navigates to the next match in the document.
// MSOWNERS: core (maxwellmyers@microsoft.com)
HRESULT FindNext(
Copy link

Choose a reason for hiding this comment

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

If the FindConfiguration FindDirection is set to Backwards, FindNext will find the match that occurs before the current match - is that correct?
We should explicitly document the relation between FindNext/FindPrevious and FindDirection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct - Please explicitly state that in the ref docs for FindNext/Previous.

CHECK_FAILURE(findConfiguration->put_FindTerm(findTerm.c_str()));
CHECK_FAILURE(findConfiguration->put_IsCaseSensitive(false));
CHECK_FAILURE(findConfiguration->put_ShouldMatchWord(false));
CHECK_FAILURE(findConfiguration->put_FindDirection(COREWEBVIEW2_FIND_DIRECTION_FORWARD));
Copy link

Choose a reason for hiding this comment

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

I don't know what the default find UI looks like. Does it have UI to control these options? E.g. is there a TextBox for the find term and checkboxes for case-sensitive/match-word?
If so, what does interacting with this UI do?
Similarly, is there a 'next' button? If so, what does clicking on it do?

In general, it would be good to describe what using this API with the default find UI looks like. I can understand how using this API would work in the case where I am suppressing the default UI. But in the case where I am relying on the default UI, it's unclear what I would do with this API. What is the scenario that is trying to be achieved?

Copy link
Contributor

Choose a reason for hiding this comment

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

See above to add screenshot and more details on the scenario.

[com_interface("staging=ICoreWebView2StagingFind")]
[ms_owner("core", "maxwellmyers@microsoft.com")]
[availability("staging")]
interface ICoreWebView2Find
Copy link

Choose a reason for hiding this comment

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

If I call WebView.Find to get the Find object, there is no way for me to query what state it is in. If I want to know that, I have to keep track myself.

For example:
Is there an active Find operation in place at the moment? I can figure this out by checking ActiveMatchIndex==-1 but that is a little obscure.
If there is an active Find operation in place, what string is being searched for? There is no way for me to get to the FindConfiguration object.

Copy link
Contributor

Choose a reason for hiding this comment

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

No change. From discussion it seems reasonable for the app to keep track of this itself.

```cpp

/// Specifies the direction of find Parameters.
// MSOWNERS: core (maxwellmyers@microsoft.com)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't normally include the MSOWNERS comments in the public doc

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix (throughout)

/// This interface allows for the creation of new find configuration objects.
// MSOWNERS: core (maxwellmyers@microsoft.com)
[uuid(f10bddd3-bb59-5d5b-8748-8a1a53f65d0c), object, pointer_default(unique)]
interface ICoreWebView2StagingEnvironment5 : IUnknown {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove "staging" from interface names and use the final names

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

}


/// Interface providing methods and properties for finding and navigating through text in the web view.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: webview should be 1 word here and other places

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - or WebView2!

[propget] HRESULT FindDirection([out, retval] COREWEBVIEW2_FIND_DIRECTION* value);


///
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing descriptions for setters

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is generated by our tooling, please follow up with me/Jessica about generating ref docs for these.


#### Description

To initiate a find operation in a WebView2 control, use the `StartFind` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

The old API (Microsoft internal link) called it a "find session", initiated by a "find request".

This method allows setting the find term and find parameters via the
`ICoreWebView2FindConfiguration` interface. Only one find session can be active per
webview environment. Starting another with the same configuration will adjust
the active match index based on the selected Find Direction.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about starting another find session with a different configuration? Does that implicitly cancel the previous find session?

Copy link
Contributor

Choose a reason for hiding this comment

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

I read this as really this always starts a Find session with the first match active. You get the same result whether or not you had the same find already running. Took me a minute to figure out, but if that's what it means, then the behavior sounds right

Copy link
Contributor

Choose a reason for hiding this comment

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

The current behavior is confusing. Please ensure that calling StartFindAsync always cancels the existing find and starts a new one from scratch as if you had only just called StartFindAsync without an existing find already going on.


// Query for the ICoreWebView2_17 interface to access the Find feature.
auto webView2_17 = m_webView.try_query<ICoreWebView2_17>();
CHECK_FEATURE_RETURN(webView2_17);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is just a feature check (we throw the QI away), shouldn't this be done first, before we ask for ICoreWebView2Environment5?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix by moving to top of function.

{
// Query for the ICoreWebView2Environment5 interface.
auto webView2Environment5 = m_webViewEnvironment.try_query<ICoreWebView2Environment5>();
CHECK_FEATURE_RETURN(webView2Environment5);
Copy link
Contributor

Choose a reason for hiding this comment

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

CHECK_FEATURE_RETURN_EMPTY? (I can't find definitions of these macros, but it seems that the caller expects null to be returned on failure.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please validate. Follow up with me if there's something in WIL or something more standard we can use instead throughout our sample app code.

}
// Query for the ICoreWebView2_17 interface to access the Find feature.
auto webView2_17 = m_webView.try_query<ICoreWebView2_17>();
CHECK_FEATURE_RETURN(webView2_17);
Copy link
Contributor

Choose a reason for hiding this comment

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

CHECK_FEATURE_RETURN_FALSE?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please validate if this is the correct macro


// Specify that a custom UI will be used for the find operation.
webView.CoreWebView2.Find.SuppressDefaultDialog = true;
webView.CoreWebView2.Find.ShouldHighlightAllMatches = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not obvious at first glance which things are "find configuration" things and which are "find things", particularly since "ShouldHighlightAllMatches" is configuring the default Find UI!

Maybe call one of them FindRequest and the other FindUI?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix sample code. These properties are now on the Configuration / Options object.

// Start the find operation with the specified configuration.
await webView.CoreWebView2.Find.StartFindAsync(findConfiguration);
// It's expected that the custom UI for navigating between matches (next, previous)
// and stopping the find operation will be managed by the developer's custom code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the custom code have the ability to push the ActiveMatchIndex back into the WebView? (So that other plugins can read it / be notified when it changes?) Or is the idea that the custom UI should just respond to the active match but not try to change it? When the user hits "Next" in the custom UI, the custom UI should perform a StartFind?

I think an example of a custom UI would help.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to a sample. User hits "Next" should call FindNext. Moving to an explicit index isn't supported in Edge today either

Copy link
Contributor

Choose a reason for hiding this comment

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

That feature (setting the ActiveMatchIndex) is not currently supported. We can look at supporting that in the future if necessary.

#### .NET C#
```csharp
//! [GetActiveMatchIndex]
public async Task<int> GetActiveMatchIndexAsync()
Copy link
Contributor

Choose a reason for hiding this comment

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

This method contains no await statements. No need to be async.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.


/// Gets the `ShouldHighlightAllMatches` property.
// MSOWNERS: core (maxwellmyers@microsoft.com)
[propget] HRESULT ShouldHighlightAllMatches([out, retval] BOOL* value);
Copy link
Contributor

Choose a reason for hiding this comment

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

No event for when this changes? How would the custom UI know when to update its checkbox?

Similarly, how does the custom UI know when to dismiss (when StopFind is called)? How does the custom UI know whether to show the "case sensitive" box checked or unchecked? It seems that the current API is not sufficient to build the custom Find UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the custom UI is driving all of this, not responding to it. E.g. it's the custom UI that defines if matches should be highlighted, and then updates the WebView to do the right thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. This property is driven entirely by the custom UI so no need for an event.


/// Navigates to the next match in the document.
// MSOWNERS: core (maxwellmyers@microsoft.com)
HRESULT FindNext(
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that there are two ways to perform a FindNext.

  1. Call StartFind with the same configuration as the currently active Find operation. ("Starting another [find session] with the same configuration will adjust the active match index based on the selected Find Direction.")
  2. Call FindNext explicitly.

Does this make FindNext redundant?

Copy link
Contributor

Choose a reason for hiding this comment

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

See above. We said StartFind will now always start a new/fresh find session.

@ajtruckle
Copy link

ajtruckle commented May 21, 2024 via email

try
{
// Check if the webView is already initialized and is an instance of CoreWebView2.
if (webView.CoreWebView2 == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sample: just call EnsureWebView2Async() instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix: Calling Ensure may be easier to understand. Please use that (assuming it doesn't otherwise cause issues for the sample).


#### Description

To initiate a find operation in a WebView2 control, use the `StartFind` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think part of the awkwardness is usage of the word "find" rather than "search"; "search" as both a noun and a verb is more common, at least in en-us. (But "find" is the right name for this feature)

wil::com_ptr<ICoreWebView2FindConfiguration> findConfiguration;
CHECK_FAILURE(webView2Environment5->CreateFindConfiguration(&findConfiguration));
CHECK_FAILURE(findConfiguration->put_FindTerm(findTerm.c_str()));
CHECK_FAILURE(findConfiguration->put_IsCaseSensitive(false));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is false not the default? We shouldn't show setting the default in examples, as it confuses what the default is

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix. Ensure the ref docs say what the default value is. If the default matches what you are setting here then do not explicitly set it to the default.


To initiate a find operation in a WebView2 control, use the `StartFind` method.
This method allows setting the find term and find parameters via the
`ICoreWebView2FindConfiguration` interface. Only one find session can be active per
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of class is usually called an Options

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix

  • CoreWebView2FindConfiguration -> CoreWebView2FindOptions

//! [ConfigureAndExecuteFindWithDefaultUI]
async Task ConfigureAndExecuteFindWithDefaultUIAsync(string findTerm)
{
try
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't show try/catch in examples as it implies it's necessary. (And if it's necessary we should make it unnecessary.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix. Only include try/catch in sample code if its actually required for the particular sample scenario (not just as a good idea / catch all)

//! [GetActiveMatchIndex]
public async Task<int> GetActiveMatchIndexAsync()
{
var webViewFind = webView.CoreWebView2.Find; // Assuming webView is your WebView2 control
Copy link
Contributor

Choose a reason for hiding this comment

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

Sample nit: make it a class member and declare it

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix (make it _webView)


#### Description

To initiate a find operation in a WebView2 control, use the `StartFind` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To initiate a find operation in a WebView2 control, use the `StartFind` method.
To initiate a find operation in a WebView2 control, use the `StartFindAsync` method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix if this is not talking about COM specifically.

This method allows setting the find term and find parameters via the
`ICoreWebView2FindConfiguration` interface. Only one find session can be active per
webview environment. Starting another with the same configuration will adjust
the active match index based on the selected Find Direction.
Copy link
Contributor

Choose a reason for hiding this comment

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

I read this as really this always starts a Find session with the first match active. You get the same result whether or not you had the same find already running. Took me a minute to figure out, but if that's what it means, then the behavior sounds right


```cpp

//! [InitializeFindConfiguration]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's used to extract the example code into a doc somewhere

// Start the find operation with the specified configuration.
await webView.CoreWebView2.Find.StartFindAsync(findConfiguration);
// It's expected that the custom UI for navigating between matches (next, previous)
// and stopping the find operation will be managed by the developer's custom code.
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to a sample. User hits "Next" should call FindNext. Moving to an explicit index isn't supported in Edge today either


#### Description
Developers can retrieve the index of the currently active match
within a WebView2 control using the `GetActiveMatchIndex` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Or

Suggested change
within a WebView2 control using the `GetActiveMatchIndex` method.
within a WebView2 control using the `ActiveMatchIndex` property.


/// Gets the `ShouldHighlightAllMatches` property.
// MSOWNERS: core (maxwellmyers@microsoft.com)
[propget] HRESULT ShouldHighlightAllMatches([out, retval] BOOL* value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the custom UI is driving all of this, not responding to it. E.g. it's the custom UI that defines if matches should be highlighted, and then updates the WebView to do the right thing

// you can change the SuppressDefaultDialog and ShouldHighlightAllMatches properties here.

// Start the find operation with a callback for completion.
CHECK_FAILURE(webView2Find->StartFind(
Copy link
Contributor

Choose a reason for hiding this comment

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

The IDL calls this StartFindAsync, but I think "Start" and "Async" are redundant, we should go with one or the other

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix:

  • COM: Start
  • .NET/WinrT: StartAsync


The WebView2Find API offers methods and events for text finding and navigation
within a WebView2 control. It enables developers to programmatically initiate find
operations, navigate find results, suppress default UI, and customize find configurations
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.


#### Description

To initiate a find operation in a WebView2 control, use the `StartFind` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update:

  • CoreWebView2.Find -> CoreWebView2.FindSession
  • StartFindAsync -> StartAsync
  • StopFind -> Stop

(but leave FindNext/Previous as is with Find)


#### Description

To initiate a find operation in a WebView2 control, use the `StartFind` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix after renaming property name its now FindSession (including casing) throughout doc.


#### Description

To initiate a find operation in a WebView2 control, use the `StartFind` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix if this is not talking about COM specifically.


To initiate a find operation in a WebView2 control, use the `StartFind` method.
This method allows setting the find term and find parameters via the
`ICoreWebView2FindConfiguration` interface. Only one find session can be active per
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix

  • CoreWebView2FindConfiguration -> CoreWebView2FindOptions

/// This interface allows for the creation of new find configuration objects.
// MSOWNERS: core (maxwellmyers@microsoft.com)
[uuid(f10bddd3-bb59-5d5b-8748-8a1a53f65d0c), object, pointer_default(unique)]
interface ICoreWebView2StagingEnvironment5 : IUnknown {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

}


/// Interface providing methods and properties for finding and navigating through text in the web view.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - or WebView2!


/// Gets the `ShouldHighlightAllMatches` property.
// MSOWNERS: core (maxwellmyers@microsoft.com)
[propget] HRESULT ShouldHighlightAllMatches([out, retval] BOOL* value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. This property is driven entirely by the custom UI so no need for an event.



/// Adds an event handler for the `ActiveMatchIndexChanged` event.
/// Registers an event handler for the ActiveMatchIndexChanged event. This event is raised when the index of the currently active match changes. This can happen when the user navigates to a different match or when the active match is changed programmatically. The parameter is the event handler to be added. Returns a token representing the added event handler. This token can be used to unregister the event handler.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

/// If called with an empty string, the Find bar is displayed but no finding occurs. Changing the configuration object after initiation won't affect the ongoing find session.
/// To change the ongoing find session, StartFindAsync must be called again with a new or modified configuration object.
/// StartFind supports, HTML, PDF, and TXT document queries. In general this api is designed for text based find sessions.
//// If you start a find session programmatically on another file format that doesnt have text fields, the find session will try to execute but will fail to find any matches. (It will silently fail)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update: It doesn't silently fail. It will show the find UI but not find any matches.

/// Displays the Find bar and starts the find operation. If a find session was already ongoing, it will be stopped and replaced with this new instance.
/// If called with an empty string, the Find bar is displayed but no finding occurs. Changing the configuration object after initiation won't affect the ongoing find session.
/// To change the ongoing find session, StartFind must be called again with a new or modified configuration object.
/// This method is primarily designed for HTML document queries.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

This comment is out of date. The comment on the MIDL3 StartFindAsync is more up to date.


/// Navigates to the next match in the document.
// MSOWNERS: core (maxwellmyers@microsoft.com)
HRESULT FindNext(
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct - Please explicitly state that in the ref docs for FindNext/Previous.


/// Navigates to the next match in the document.
// MSOWNERS: core (maxwellmyers@microsoft.com)
HRESULT FindNext(
Copy link
Contributor

Choose a reason for hiding this comment

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

See above. We said StartFind will now always start a new/fresh find session.

[propget] HRESULT FindDirection([out, retval] COREWEBVIEW2_FIND_DIRECTION* value);


///
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is generated by our tooling, please follow up with me/Jessica about generating ref docs for these.

### Setting Up Find Configuration with MIDL3

### CoreWebView2 Find Configuration and Direction

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove.


/// Gets or sets the state of whether all matches are highlighted.
/// Returns TRUE if all matches are highlighted, FALSE otherwise.
/// Note: Changes to this property take effect only when StartFind, FindNext, or FindPrevious is called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix. This property is not live. Update docs to say correct info.

/// Note: Changes to this property take effect only when StartFind, FindNext, or FindPrevious is called.
/// Preferences for the session cannot be updated unless another call to the StartFind function on the server-side is made.
/// Therefore, changes will not take effect until one of these functions is called.
Boolean SuppressDefaultFindDialog { get; set; };
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix - the property should be on the config

[com_interface("staging=ICoreWebView2StagingFind")]
[ms_owner("core", "maxwellmyers@microsoft.com")]
[availability("staging")]
interface ICoreWebView2Find
Copy link
Contributor

Choose a reason for hiding this comment

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

No change. From discussion it seems reasonable for the app to keep track of this itself.

/// To change the ongoing find session, StartFindAsync must be called again with a new or modified configuration object.
/// StartFind supports, HTML, PDF, and TXT document queries. In general this api is designed for text based find sessions.
//// If you start a find session programmatically on another file format that doesnt have text fields, the find session will try to execute but will fail to find any matches. (It will silently fail)
/// Note: The asynchronous action completes when the UI has been displayed with the find term in the UI bar, and the matches have populated on the counter on the find bar.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the completion of the IAsyncAction the point at which calls to Next()/Previous()/Stop() methods become allowed?

Yes

What happens if I call Next() before this IAsyncAction completes, is it the same "silently fail" as when there's no active session?

Yes

Please make sure this is documented in the section about Start and what its async completion means.

/// If called with no Find session active, it will silently do nothing.
void StopFind();

/// Retrieves the index of the currently active match in the find session. Returns the index of the currently active match, or -1 if there is no active match.
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave as is. Discussion: We can support nullable / ireference with a bunch of additional work but don't find it to be ROI worthwhile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Proposal Review WebView2 API Proposal for review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants