Webview appears to think it's in focus when typing in the addressbar

Bug #1599771 reported by Chris Coulson on 2016-07-07
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Oxide
Medium
Santosh
webbrowser-app (Ubuntu)
Undecided
Unassigned

Bug Description

See the screenshot - when keyboard focus is on the addressbar, the webview thinks it is still in focus (indicated by the presence of the caret in the search field).

Not sure if this is Oxide or webbrowser-app

Changed in oxide:
assignee: nobody → Santosh (santoshbit2007)
Olivier Tilloy (osomon) wrote :

I can reliably reproduce the bug on my desktop.

I have instrumented the browser to print a debug statement every time the activeFocus property of the WebView changes (in Browser.qml):

  WebViewImpl {
    onActiveFocusChanged: console.log("webview.activeFocus =", activeFocus)
  }

And indeed when I focus the address bar, webview.activeFocus becomes false.

Could it be an issue in oxide?

Santosh (santoshbit2007) wrote :

Olivier Surprising it is,
I was checking from oxide side, and QQuickWeview activeFocus is set to true.
So I was doubting issue in webbrowser-app

Olivier Tilloy (osomon) wrote :

I think I know where the issue is: when the OxideQQuickWebView is getting a focusOut event, it’s calling WebContentsView::FocusChanged(), which checks whether the view has focus by calling WebContentsView::HasFocus(), but at this point it returns true.

Changed in webbrowser-app (Ubuntu):
status: New → Invalid
Changed in oxide:
status: New → Confirmed
Chris Coulson (chrisccoulson) wrote :

I suspect that oxide::qquick::ContentsView::HasFocus also needs to verify that item_ == item_->window()->activeFocusItem() (http://doc.qt.io/qt-5/qquickwindow.html#activeFocusItem-prop). It looks like this is cleared before dispatching QQuickItem::focusOutEvent(), whereas QQuickItemPrivate::activeFocus isn't.

Chris Coulson (chrisccoulson) wrote :

(after looking at QQuickWindowPrivate::setFocusInScope)

Santosh (santoshbit2007) wrote :

it seems focusOutEvent comes too early to QQuickItem. by that time activefocus is not updated, so it still holds earlier value.

Once activeFocus of QQuickItem is changed we get ItemChanged(http://doc.qt.io/qt-5/qquickitem.html#ItemChange-enum).

I tested it OxideQQuickWebView::itemChange has correct value of activeFocus.
Inside:
itemChangeItemChange : 6 Value : 0 // 6 represent QQuickItem::ItemActiveFocusHasChanged
ActiveFocusItem : 0

So I was wondering if we should move focus handling inside OxideQQuickWebView::itemChange OR
tweak HasFocus()

Chris Coulson (chrisccoulson) wrote :

I think I'd prefer to just use QQuickWindow::activeFocusItem in oxide::qquick::ContentsView::HasFocus(). From looking at the code, I don't even think that QQuickItem::hasActiveFocus has the correct semantics as it will indicate active focus for ancestors of the active focus item which isn't really what we want (ie, if an application adds another item as a child of the webview, we shouldn't think we're actively focused if the child item is the active focus item).

We should really add some integration tests for these cases in qt/tests/qmltests/web_platform

Changed in oxide:
status: Confirmed → Triaged
importance: Undecided → Medium
Santosh (santoshbit2007) wrote :

QQuickWindow::activeFocusItem will still be tricky.

There seems to be fundamental issue with focus events sent.

FocusEvent are only sent to Item when its activeFocus = true;
So when we focus is removed from item it won't immediately set activeFocus=false(since item won't get focus event then). What is does is, first send focusOutEvent to item, then set activeFocus = false;

And when webview is clicked we forcefully need to set activeFocus=true(item_->forceActiveFocus in handleMousePress). then item is eligible to receive focusInEvent.

In short activeFocus is updated after focusOutEvent and before focusInEvent.

In this way In focusOutEvent both activeFocus and QQuickWindow::activeFocusItem both are not upated, QQuickWindow::activeFocusItem is specifically nullptr

David Barth (dbarth) wrote :

Could bug #1566373 be related?

Chris Coulson (chrisccoulson) wrote :

Whatever we do, we should stop using QQuickItem::hasActiveFocus() for the reason outlined in comment 8. I don't understand the issue with QQuickWindow::activeFocusItem() though - it looks like it's cleared to nullptr before a focus out event and initialized to the new focus item before a focus in event. It sounds like exactly what we want.

Santosh (santoshbit2007) wrote :

QQuickWindow::activeFocusItem() is fine , I was just doubtful with it being null.
I am going to patch it with using same

Changed in oxide:
status: Triaged → In Progress
Santosh (santoshbit2007) wrote :

Ready for review:
ttps://code.launchpad.net/~santoshbit2007/oxide/+git/oxide/+merge/301475

Santosh (santoshbit2007) wrote :
Changed in oxide:
status: In Progress → Fix Committed
Changed in oxide:
milestone: none → branch-1.19
status: Fix Committed → Fix Released
Olivier Tilloy (osomon) wrote :

Re-opening as the change had to be reverted because it caused a regression (bug #1649577).

Changed in oxide:
status: Fix Released → Confirmed
milestone: branch-1.19 → branch-1.20
Changed in oxide:
milestone: branch-1.20 → branch-1.21
Santosh (santoshbit2007) wrote :

The real cause here is we rely on focusOut(In)Event to check for activeFocus item. but the any check of ActiveFocus (hasActiveFocus or window->activeFocusItem) inside focusout/in event give incorrect result.

when another item is focused(location bar), window->activeFocusItem() return null not the new activeFocus Item.

While check for activeFocus item is consistent Once we get ItemChanged event.
http://doc.qt.io/qt-5/qquickitem.html#itemChange

So When quickItem activeFocus changes we get ItemChange event and after any check window->activeFocusItem() correctly return the newly focus QuickItem.

In short, we should do webview focus update based on ItemChange event.

see previous comments also for more on activeFocus behaviour inside focusout/in event.

Olivier Tilloy (osomon) on 2016-12-19
Changed in oxide:
status: Confirmed → In Progress
Changed in oxide:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers