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

Add OnGotFocus and OnSetFocus #341

Closed
wants to merge 4 commits into from

Conversation

amaitland
Copy link
Member

Add OnGotFocus and OnSetFocus
Add CefFocusSource enum for mapping from cef_focus_source_t enum

@amaitland
Copy link
Member Author

Currently the OnGotFocus, OnTakeFocus methods don't get called.

Anyone have any thoughts?

Once I can get those working I'll add the relevant event handlers so they can be handled client side.

@jornh
Copy link
Contributor

jornh commented Apr 29, 2014

I suspect it could have to do with this comment: https://github.com/cefsharp/CefSharp/blob/v31.0.0-pre1/CefSharp.Wpf/WebView.cs#L713

To me it sounds that we are actually in luck that OnTakeFocus() doesn't get called or end users would experience a NotImplementedException. I don't think we should require from those implementing CefSharp in their application that they must implement those handlers. If they don't implement anything it should still work.

I haven't actually tried it out but if you compare it to the CefSharp1 branch it looks like it was working there.

@perlun
Copy link
Member

perlun commented Apr 29, 2014

Let me ask a stupid question: why do we need this? Isn't the focus support (i.e. http://msdn.microsoft.com/en-US/library/system.windows.uielement.lostfocus(v=vs.110).aspx and friends) that WPF and Windows Forms already offers enough? What additional functionality do we gain by exposing the CEF API in this case?

@perlun
Copy link
Member

perlun commented Apr 29, 2014

Also, I would personally think it would be better if you finish the #327 stuff so we can merge it... 😜

@amaitland
Copy link
Member Author

I was hoping that as with other browsers we'd be able to Set the focus to the address bar (something that I think would be a fairly common requirement) when OnTakeFocus is fired. I realise adding the the extra methods aren't required for that, it just made sense to me to look at finishing implementing all the methods exposed by CefFocusHandler whilst I was at it.

@amaitland
Copy link
Member Author

As for #327 I was hoping to resolve #331 before progressing with a new PR that splits #327 up.
(I'm not trying to force #331 through either, I'd just like a clear direction one way or the other)

After that I'm happy to progress with tiding #327 up 😄

@brock8503
Copy link
Contributor

@perlun I think that is a great question worth exploring. I am not 100% sure myself. How do we currently handle the case when the DOM HTMLElement.focus() is called. Shouldn't we also plum this focus event up to the WPF/WinForm control to make sure windows is aware of the focus change?

@amaitland
Copy link
Member Author

Latest commit gets Focus working correctly in the WinForms Example

@amaitland
Copy link
Member Author

@perlun As the Focus methods are relevant to the WinForms implementation (which I've been working on a Tabbed version), I'd like to properly implement these methods. As they don't currently work and may well not be relevant to the WPF version, I've moved them from the IWebBrowserInternal to IWinFormsWebBrowser and added a check so they're only called in the WinForms context.

Does this mitigate any of your previous objections?

@amaitland amaitland added the cef3 label Aug 6, 2014
jornh referenced this pull request in gakibbie/CefSharp Oct 2, 2014
@amaitland amaitland added this to the 33.0.1 milestone Oct 3, 2014
@jornh jornh modified the milestones: 33.0.2, xx.0.3 Oct 24, 2014
@amaitland amaitland modified the milestones: xx.0.3, 37.0.0, 3000 Nov 11, 2014
@jankurianski
Copy link
Member

@amaitland This PR would be a great feature - as we are trying to show the WinForms control dynamically as the user types in a separate TextBox, but ChromiumWebBrowser is stealing focus as it is being shown. Would be great to have access to OnSetFocus and be able to return true. Let me know if there's anything I can do to help test out this PR.

@amaitland
Copy link
Member Author

@jankurianski Maybe have a look over the code and let me know if you've got any comments?

@amaitland
Copy link
Member Author

Would be great to have access to OnSetFocus and be able to return true

Also those methods aren't exposed as yet, so need to create an interface I guess.

@jankurianski
Copy link
Member

@amaitland If you're not in the middle of this change (as it seems it was a while ago), I'm happy to extend this further and expose a FocusHandler on ChromiumWebBrowser. Don't want to step on your toes though.

}
}

bool ClientAdapter::OnSetFocus(CefRefPtr<CefBrowser> browser, FocusSource source)
Copy link
Member

Choose a reason for hiding this comment

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

Extra space here after browser parameter.

public enum CefFocusSource
{
///
// The source is explicit navigation via the API (LoadURL(), etc).
Copy link
Member

Choose a reason for hiding this comment

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

Best as <summary> tags for the XML docs.

@amaitland
Copy link
Member Author

If you're not in the middle of this change (as it seems it was a while ago), I'm happy to extend this further and expose a FocusHandler on ChromiumWebBrowser

Help finishing this one off is most welcome! 😄

@jankurianski
Copy link
Member

https://github.com/numbersint/CefSharp/compare/feature/focus-handler-ext

  • I added an IFocusHandler interface similar to IMenuHandler (similarly, it does not really apply for WPF, but I guess it is better to be consistent and leave it to the implementer to decide).
  • Modified the WinForms example so that tabbing out of the browser moves focus to the address bar.
  • I tested (but didn't commit) returning true from OnSetFocus - it works and prevents the browser from taking focus away from other controls (e.g. the address bar), so this will be good for my app.

@amaitland I couldn't figure out how to make a clean PR for your fork, as git seems to want to include every master commit since your branch. What do you reckon is easiest - for you to copy my commits over to update your PR, or for me to make a new PR to mainline CefSharp from my branch?

Add CefFocusSource enum for mapping from cef_focus_source_t enum
…d only call them in the context of the WinForms ChromiumWebBrowser
@amaitland amaitland force-pushed the feature/focus-handler branch from a2debed to 5b56436 Compare November 25, 2014 12:08
@amaitland
Copy link
Member Author

@jankurianski I've just rebased this PR on the latest master

I'm happy if you want to cherry pick my changes and create a new PR. Whatever is easiest.

@amaitland
Copy link
Member Author

Any thoughts on how we can preserve the current default behavior of the WinForms control?

void IWebBrowserInternal.OnTakeFocus(bool next)
{
    SelectNextControl(this, next, true, true, true);
}

@jankurianski jankurianski mentioned this pull request Nov 25, 2014
@amaitland
Copy link
Member Author

Closing in favor of #639

@amaitland amaitland closed this Nov 25, 2014
@amaitland amaitland deleted the feature/focus-handler branch March 19, 2015 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants