WebEnginePart: A new tab requested by user is not a popup
ClosedPublic

Authored by marten on Sep 24 2019, 2:29 PM.

Details

Summary

When using this viewer part, a new tab (requested by Ctrl-LMB or MMB over a link, when the "Open links in new tab..." option is on) seems to be treated as a popup window and puts up a message box "This site is requesting to open a popup window to...". This is reasonable when a site is actually opening a popup, but for a user requested new tab it should not do this. The popup menu "Open in New Tab" action does not prompt and correctly just opens a new tab.

Neither KHTML nor the WebKit part put up this prompt, they just open a new tab as requested.

The fix to NewWindowPage::acceptNavigationRequest() checks the type of navigation request (passed up from QWebEnginePage) and does not prompt for user requests for a new tab.

The submitted diff is not reindented, so as not to confuse the code change with whitespace changes. I will indent the block in accordance with coding style before committing.

Test Plan

Built Konqueror (including the WebEnginePart plugin) with this change. Observed that Ctrl-LMB, MMB or "Open in New Tab" simply open a new tab without prompting. Observed that opening a site popup still prompts as before.

Diff Detail

Repository
R226 Konqueror
Lint
Lint Skipped
Unit
Unit Tests Skipped
marten requested review of this revision.Sep 24 2019, 2:29 PM
marten created this revision.
dfaure requested changes to this revision.Oct 15 2019, 6:18 AM
dfaure added a reviewer: stefanocrocco.
dfaure added inline comments.
webenginepart/src/webenginepage.cpp
774

I think you want to edit this condition instead. The point of this bool is "this action was requested by the user". If the condition is incorrect, this is where it should be fixed.

780

coding style: spaces around !=

This revision now requires changes to proceed.Oct 15 2019, 6:19 AM
marten updated this revision to Diff 67956.Oct 15 2019, 10:39 AM

If you mean to incorporate the tests directly into actionRequestedByUser:

const bool actionRequestedByUser = type != QWebEnginePage::NavigationTypeOther &&
  m_type != QWebEnginePage::WebBrowserBackgroundTab &&
  m_type != QWebEnginePage::WebBrowserTab;
if (actionRequestedByUser) { ...

it doesn't seem to be possible because that bool is used not only to control whether the question is asked but also to set KParts::OpenUrlArguments below. Setting the changed value here also makes anything that should be opened in a new tab open in a new window instead.

Adding the tests for m_type conditions to the if (actionRequestedByUser) suppresses the question and opens tabs correctly; the block does not now need to be indented.

marten marked 2 inline comments as done.Oct 15 2019, 10:40 AM
marten added inline comments.
webenginepart/src/webenginepage.cpp
774

See update diff comment

Then that bool is misused further down.

"requested by user" should be true if requested by user, and false otherwise. Anything else makes the code really hard to understand.

From what you're saying, the BrowserArgs code should check for type != QWebEnginePage::NavigationTypeOther explicitly, rather than using the bool "requested by user".

marten updated this revision to Diff 68133.Oct 17 2019, 1:24 PM
marten marked an inline comment as done.

Updated in accordance with review comments, variable names corresponding to what they actually do.

Not sure whether the '!part() && !isMainFrame' condition is significant and needs to be before the '!actionRequestsNewTab' test, i.e. whether it should read:

if (actionRequestedByUser) {
  if (!part() && !isMainFrame) {
    return false;
  }
  if (!actionRequestsNewTab) {
    // ask the question
    ...

but doing this means the question block would have to be reindented.

dfaure accepted this revision.Oct 20 2019, 6:04 PM

Reindenting is fine, if needed :-)

I'm not sure what the "no part" bit is about, maybe stefanocrocco knows.

In any case, this new patch looks much better to me :)

This revision is now accepted and ready to land.Oct 20 2019, 6:04 PM

Reindenting is fine, if needed :-)

I'm not sure what the "no part" bit is about, maybe stefanocrocco knows.

Unfortunately, I don't. I may be wrong, but I think that conditional has been part of the initial porting from KWebKitPart.

This revision was automatically updated to reflect the committed changes.