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

overriding JsDialogHandler is not hiding Javascript alerts #388

Closed
kropewnicki opened this issue Jun 6, 2014 · 9 comments · Fixed by #397
Closed

overriding JsDialogHandler is not hiding Javascript alerts #388

kropewnicki opened this issue Jun 6, 2014 · 9 comments · Fixed by #397
Assignees
Milestone

Comments

@kropewnicki
Copy link
Contributor

my goal is to hide some javascript alerts that are popping up on my html

I have a custom JsDialogHandler with empty methods.

public class JsHandler : IJsDialogHandler
{
    public bool OnJSAlert(IWebBrowser browser, string url, string message)
    {

        return false;
    }

    public bool OnJSConfirm(IWebBrowser browser, string url, string message, out bool retval)
    {
        retval = false;
        return false;
    }

    public bool OnJSPrompt(IWebBrowser browser, string url, string message, string defaultValue, out bool retval, out string result)
    {
        retval = false;
        result="";
        return false;
    }
}

But I am still seeing javascript alerts. Is my override incorrect, or is there an issue in cefsharp JsHandler?

@kropewnicki kropewnicki changed the title overriding JsDialogHandler is not hiding Javacsript alerts overriding JsDialogHandler is not hiding Javascript alerts Jun 6, 2014
@jornh
Copy link
Contributor

jornh commented Jun 6, 2014

I haven't tried this out - just guessing - but have you tried with e.g.:

        return true; // <--- Tell CEF that you handled it so it should just shut up!
    }

The documentation for the upstream CEF OnJSDialog API leaves me with the impression that it would want that.

(We should really add this to the xmldocs too - iff I'm guessing right)

@kropewnicki
Copy link
Contributor Author

@jornh changing to return true has not effect of javascript popup alerts

public class JsHandler : IJsDialogHandler
{
    public bool OnJSAlert(IWebBrowser browser, string url, string message)
    {

        return true;
    }

    public bool OnJSConfirm(IWebBrowser browser, string url, string message, out bool retval)
    {
        retval = true;
        return true;
    }

    public bool OnJSPrompt(IWebBrowser browser, string url, string message, string defaultValue, out bool retval, out string result)
    {
        retval = true;
        result="";
        return true;
    }
}

@jornh jornh added the bug label Jun 8, 2014
@jornh jornh added this to the 31.0.0 milestone Jun 8, 2014
@jornh
Copy link
Contributor

jornh commented Jun 8, 2014

No, you are absolutely right. Thanks for reporting.

If you step out of your handler code into ClientAdapter.cpp

you will see that your return value is simply ignored.

It always returns false. So fixing this should be quite trivial 😄 Apparently you are the first one who tried using this with the CefSharp3/master branch!

@kropewnicki
Copy link
Contributor Author

Glad, to help.

@perlun
Copy link
Member

perlun commented Jun 9, 2014

@jornh, you fix? 😄

@perlun perlun added the cef3 label Jun 9, 2014
@jornh
Copy link
Contributor

jornh commented Jun 9, 2014

Yep yep yep 😃

@amaitland
Copy link
Member

Worth implementing the rest of CefJSDialogHandler while we're at it?

@jornh
Copy link
Contributor

jornh commented Jun 10, 2014

I think we should leave that to after 31.0.0. It would be an enhancement - not an API change AFAICT?
And with the trivial fix we would be on par with CefSharp 1.

@amaitland
Copy link
Member

Works for me 😄 It simply adds to the API.

jornh added a commit to jornh/CefSharp that referenced this issue Jun 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants