Bug 79738 – [Qt] Authentication dialog does not work

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Yael]

With latest Qt and latest webkit, cannot type username/password in HTTP Authentication dialog.

Try loading e.g. http://iop4.nokia-boston.com. The Authentication dialog shows, but users cannot type in it.

[Dinu Jacob]

The mouse press event on the text input field is translated to touch event in MiniBrowser. This results in qquickwebview getting focus (it calls forceActiveFocus in touchEvent handler). Hence, the text input field loses focus and doesn't take any input.

[Dinu Jacob]

I have a patch preventing event conversion in MiniBrowser if dialogRunner is active. Needed to expose an API from webView experimental to indicate this.

[Dinu Jacob]

Created attachment 129734 [details]
Prevent coverting mouse events to touch events in MiniBrowser if dialogRunner is active

[Simon Hausmann]

Comment on attachment 129734 [details]
Prevent coverting mouse events to touch events in MiniBrowser if dialogRunner is active

I don't like that we have to work around our touch mocking. I'm not even sure this workaround is needed anymore now that we send the event synchronously, but ultimately the fix belongs into QtQuick to make MouseArea accept touch events (see discussion on qt development list).

[Dinu Jacob]

(In reply to comment #4)
> (From update of attachment 129734 [details])
> I don't like that we have to work around our touch mocking. I'm not even sure this workaround is needed anymore now that we send the event synchronously, but ultimately the fix belongs into QtQuick to make MouseArea accept touch events (see discussion on qt development list).

I should have updated the bug. With the changes for sending the event synchronously, it is not an issue just with the input editor anymore. None of the dialogs can even be dismissed/accepted because the mouse events are not sent to the dialog item as QQuickWebView accepts the touch event. Agreed that QQuickItems should handle touch events (we have been waiting for such a fix for quite some time :) as there are other issues like QQuickWebView receiving touch events which can possibly cause a link to be selected etc. ). 

On hardware the input editor should still have an issue as QQuickWebView would grab the focus (I added the work-around in MiniBrowser thinking that this is limited to only touch mocking). The touch event handler in QQuickWebView should grab focus only if there is no active dialog (agreed this too is a work-around in the absence of a Qt fix).

[Simon Hausmann]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 129734 [details] [details])
> > I don't like that we have to work around our touch mocking. I'm not even sure this workaround is needed anymore now that we send the event synchronously, but ultimately the fix belongs into QtQuick to make MouseArea accept touch events (see discussion on qt development list).
> 
> I should have updated the bug. With the changes for sending the event synchronously, it is not an issue just with the input editor anymore. None of the dialogs can even be dismissed/accepted because the mouse events are not sent to the dialog item as QQuickWebView accepts the touch event. Agreed that QQuickItems should handle touch events (we have been waiting for such a fix for quite some time :) as there are other issues like QQuickWebView receiving touch events which can possibly cause a link to be selected etc. ). 
> 
> On hardware the input editor should still have an issue as QQuickWebView would grab the focus (I added the work-around in MiniBrowser thinking that this is limited to only touch mocking). The touch event handler in QQuickWebView should grab focus only if there is no active dialog (agreed this too is a work-around in the absence of a Qt fix).

Right. We are also in discussion with the QQuick folks about the event handling, and in the meantime it seems the best "workaround" is to not accept touch events while the dialogrunner is running.

[Dinu Jacob]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 129734 [details] [details] [details])
> > > I don't like that we have to work around our touch mocking. I'm not even sure this workaround is needed anymore now that we send the event synchronously, but ultimately the fix belongs into QtQuick to make MouseArea accept touch events (see discussion on qt development list).
> > 
> > I should have updated the bug. With the changes for sending the event synchronously, it is not an issue just with the input editor anymore. None of the dialogs can even be dismissed/accepted because the mouse events are not sent to the dialog item as QQuickWebView accepts the touch event. Agreed that QQuickItems should handle touch events (we have been waiting for such a fix for quite some time :) as there are other issues like QQuickWebView receiving touch events which can possibly cause a link to be selected etc. ). 
> > 
> > On hardware the input editor should still have an issue as QQuickWebView would grab the focus (I added the work-around in MiniBrowser thinking that this is limited to only touch mocking). The touch event handler in QQuickWebView should grab focus only if there is no active dialog (agreed this too is a work-around in the absence of a Qt fix).
> 
> Right. We are also in discussion with the QQuick folks about the event handling, and in the meantime it seems the best "workaround" is to not accept touch events while the dialogrunner is running.

I will modify the patch to add the check in QQuickWebView rather than in MiniBrowser.

[Dinu Jacob]

Created attachment 130628 [details]
Updated Patch

[Kenneth Rohde Christiansen]

Comment on attachment 130628 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130628&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:285
> +    dialogRunner = new QtDialogRunner;

This should use an OwnPtr and you should make a PassOwnPtr QtDialogRunner::create()

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:518
> +void QQuickWebViewPrivate::destroyDialogRunner()
> +{
> +    delete dialogRunner;
> +    dialogRunner = 0;
> +}

ieck! this calls for an OwnPtr

[Dinu Jacob]

(In reply to comment #9)
> (From update of attachment 130628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130628&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:285
> > +    dialogRunner = new QtDialogRunner;
> 
> This should use an OwnPtr and you should make a PassOwnPtr QtDialogRunner::create()
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:518
> > +void QQuickWebViewPrivate::destroyDialogRunner()
> > +{
> > +    delete dialogRunner;
> > +    dialogRunner = 0;
> > +}
> 
> ieck! this calls for an OwnPtr

Ah, didn't think about it :( Will change it.

[Dinu Jacob]

Created attachment 130656 [details]
Updated with Review Comments

[Simon Hausmann]

Comment on attachment 130656 [details]
Updated with Review Comments

View in context: https://bugs.webkit.org/attachment.cgi?id=130656&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1385
> +    if (d->dialogRunner.get())
> +        event->ignore();
> +    else {
> +        forceActiveFocus();
> +        d->pageView->eventHandler()->handleTouchEvent(event);
> +    }

I think webkit style would be an early return, i.e.

    if (showingDialog)
        return;

    forceActiveFocus();
    d->pageView->eventHandler()->handleTouchEvent(event);

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:157
> +    OwnPtr<QtDialogRunner> dialogRunner;

Do we really need all that ownptr, etc. and all the resulting cosmetic code changes if all we want to know is if we're showing the runner? Why not simply put a boolean in place?

[Dinu Jacob]

(In reply to comment #12)
> (From update of attachment 130656 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130656&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1385
> > +    if (d->dialogRunner.get())
> > +        event->ignore();
> > +    else {
> > +        forceActiveFocus();
> > +        d->pageView->eventHandler()->handleTouchEvent(event);
> > +    }
> 
> I think webkit style would be an early return, i.e.
> 
>     if (showingDialog)
>         return;
> 
>     forceActiveFocus();
>     d->pageView->eventHandler()->handleTouchEvent(event);
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:157
> > +    OwnPtr<QtDialogRunner> dialogRunner;
> 
> Do we really need all that ownptr, etc. and all the resulting cosmetic code changes if all we want to know is if we're showing the runner? Why not simply put a boolean in place?

I though of adding a bool but went the way of making dialogRunner a pointer so that we don't need to set the boolean every time we create a dialogRunner.

[Dinu Jacob]

(In reply to comment #12)
> (From update of attachment 130656 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130656&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1385
> > +    if (d->dialogRunner.get())
> > +        event->ignore();
> > +    else {
> > +        forceActiveFocus();
> > +        d->pageView->eventHandler()->handleTouchEvent(event);
> > +    }
> 
> I think webkit style would be an early return, i.e.
> 
>     if (showingDialog)
>         return;
> 
>     forceActiveFocus();
>     d->pageView->eventHandler()->handleTouchEvent(event);

Need to explicitly ignore the event as it is set to accepted (by Qt) when it is received here.

> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:157
> > +    OwnPtr<QtDialogRunner> dialogRunner;
> 
> Do we really need all that ownptr, etc. and all the resulting cosmetic code changes if all we want to know is if we're showing the runner? Why not simply put a boolean in place?

[Dinu Jacob]

Created attachment 130670 [details]
Updated Patch

[Simon Hausmann]

Comment on attachment 130670 [details]
Updated Patch

That looks indeed much simpler to me :-)

All those chunks of duplicated code when executing the dialogs smell like they could be simplified, but that's completely unrelated to this bug.

[Dinu Jacob]

(In reply to comment #16)
> (From update of attachment 130670 [details])
> That looks indeed much simpler to me :-)
> 
> All those chunks of duplicated code when executing the dialogs smell like they could be simplified, but that's completely unrelated to this bug.

Yes. I agree. In fact for authentication and proxy authentication I was going to do a patch as that is almost entirely duplicated.

[WebKit Review Bot]

Comment on attachment 130670 [details]
Updated Patch

Clearing flags on attachment: 130670

Committed r110098: <http://trac.webkit.org/changeset/110098>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.