Need to use (Q)Shortcuts instead of Keys.onPressed for shortcuts

Bug #1537782 reported by Olivier Tilloy
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Canonical System Image
Fix Released
High
Bill Filler
qtdeclarative-opensource-src (Ubuntu)
Invalid
Undecided
Unassigned
webbrowser-app (Ubuntu)
Fix Released
High
Olivier Tilloy
webbrowser-app (Ubuntu RTM)
Fix Released
Undecided
Unassigned

Bug Description

On a phone with a bluetooth keyboard connected, with the following standalone example, if I press Ctrl+T I expect the message "Ctrl+T pressed" to be printed on the console, but instead the Ctrl key is ignored and "t" is inserted in the text field.

import QtQuick 2.4
Item {
  TextInput {
    anchors.centerIn: parent
    width: parent.width - 20
    height: 100
    Component.onCompleted: forceActiveFocus()
  }
  Keys.onPressed: {
    console.log("key pressed:", event.key, event.modifiers)
    if (event.key == Qt.Key_T && event.modifiers == Qt.ControlModifier) {
      console.log("Ctrl+T pressed")
      event.accepted = true
    }
  }
}

=========================================================================

See the discussion on https://codereview.qt-project.org/#/c/147556/, Qt's behaviour is correct here.

QShortcut [1], or QML's Shortcut [2] from qt-5.5 should be used for this instead.

[1] http://doc.qt.io/qt-5/qshortcut.html
[2] http://doc.qt.io/qt-5/qml-qtquick-shortcut.html

Tags: patch keyboard

Related branches

Víctor R. Ruiz (vrruiz)
affects: qtubuntu → canonical-devices-system-image
Revision history for this message
Olivier Tilloy (osomon) wrote :

And as a real-life example that is affected by this issue, the browser app, when the address bar has active focus, won’t open a new tab when the user presses Ctrl+T, instead it will input "t" in the address bar.

Revision history for this message
Víctor R. Ruiz (vrruiz) wrote :

I confirm that pressing Control+T in the address bar on webbrowser-app doesn't open any tab, but types a 't'.

Changed in qtubuntu (Ubuntu):
status: New → Confirmed
Michał Sawicz (saviq)
Changed in canonical-devices-system-image:
status: New → Triaged
importance: Undecided → High
assignee: nobody → Michał Sawicz (saviq)
milestone: none → ww04-2016
Revision history for this message
Michael Terry (mterry) wrote :

unity8 is seeing the event as a Ctrl+T. But the app doesn't. So something's getting stripped along the way. Looking into where.

Revision history for this message
Michael Terry (mterry) wrote :

Correction, the app as a whole gets the event. But if the TextInput is focused, it seems to accept the event so that the outer Keys.onPressed doesn't see it.

Revision history for this message
Michael Terry (mterry) wrote :

OK, this code in qtdeclarative looks to me like it could be responsible:

src/quick/item/qquicktextinput.cpp:4427:

    if (unknown && !m_readOnly) {
        QString t = event->text();
        if (!t.isEmpty() && t.at(0).isPrint()) {
            insert(t);
            event->accept();
            return;
        }
    }

For that code to work like this bug wants, I believe we'd need to add a check that "event->modifiers() == Qt::NoModifier" there. But I need to talk to some Qt-heads to see if that's sensible.

Revision history for this message
Michael Terry (mterry) wrote :

There's already an upstream bug for this: https://bugreports.qt.io/browse/QTBUG-37994

I've attached a possible patch (fixes it for me on desktop, am testing on phone for good measure).

I'll note that on xenial desktop, Ctrl already was fixed. But Alt did hit this bug. I'm not sure why Ctrl worked...

But the patch fixes it explicitly at least. I'll put it on the upstream bug tracker.

tags: added: patch
Revision history for this message
Michael Terry (mterry) wrote :

I've uploaded my patch to the landing-019 PPA.

affects: qtubuntu (Ubuntu) → qtdeclarative-opensource-src (Ubuntu)
Changed in qtdeclarative-opensource-src (Ubuntu):
assignee: nobody → Michael Terry (mterry)
status: Confirmed → In Progress
Revision history for this message
Albert Astals Cid (aacid) wrote :

Mike, i'd suggest you upload the patch to https://codereview.qt-project.org/#/q/status:open,n,z and add some reviewers, patches in the bug tracker have an history of not being seen.

Ping me if you need help with Qt's gerrit, it's not the most helpful of the tools to use for the first time.

Revision history for this message
Michael Terry (mterry) wrote :

Uploaded to gerrit: https://codereview.qt-project.org/#/c/147556/

(Thanks for the help in doing so)

Michał Sawicz (saviq)
Changed in canonical-devices-system-image:
status: Triaged → In Progress
Revision history for this message
Michał Sawicz (saviq) wrote :

See the discussion on https://codereview.qt-project.org/#/c/147556/, Qt's behaviour is correct here.

QShortcut [1], or QML's Shortcut [2] from qt-5.5 should be used for this instead.

[1] http://doc.qt.io/qt-5/qshortcut.html
[2] http://doc.qt.io/qt-5/qml-qtquick-shortcut.html

Changed in qtdeclarative-opensource-src (Ubuntu):
status: In Progress → Invalid
Changed in canonical-devices-system-image:
assignee: Michał Sawicz (saviq) → Bill Filler (bfiller)
milestone: ww04-2016 → none
status: In Progress → New
Changed in qtdeclarative-opensource-src (Ubuntu):
assignee: Michael Terry (mterry) → nobody
summary: - Modifier ignored when pressing a key if TextInput has active focus
+ Need to use (Q)Shortcuts instead of Keys.onPressed for shortcuts
description: updated
Olivier Tilloy (osomon)
tags: added: keyboard
Changed in canonical-devices-system-image:
milestone: none → ww08-2016
status: New → Confirmed
Revision history for this message
Olivier Tilloy (osomon) wrote :

The QML Shortcut type was introduced in Qt 5.5, and the stable vivid overlay has Qt 5.4.1 (and according to Timo a full upgrade to 5.5 in the stable vivid overlay PPA is not planned).

That leaves us with the option of exposing QShortcut to QML in the browser.

Changed in webbrowser-app (Ubuntu):
status: New → Confirmed
assignee: nobody → Olivier Tilloy (osomon)
importance: Undecided → High
Changed in canonical-devices-system-image:
status: Confirmed → In Progress
Bill Filler (bfiller)
Changed in canonical-devices-system-image:
milestone: ww08-2016 → 11
status: In Progress → Confirmed
Revision history for this message
Olivier Tilloy (osomon) wrote :

I just did a quick test on my desktop running xenial (with Qt 5.5.1), and I can confirm that the Shortcut QML type (https://doc.qt.io/qt-5/qml-qtquick-shortcut.html) does what we need, so we could replace the custom KeyboardShortcuts.

This won’t work on touch devices where the version of Qt is 5.4.1.

Revision history for this message
Olivier Tilloy (osomon) wrote :

And for future reference, in the browser, the Ctrl+T shortcut would look like this:

  Shortcut {
    sequence: StandardKey.AddTab
    enabled: …
    onActivated: …
  }

Olivier Tilloy (osomon)
Changed in webbrowser-app (Ubuntu):
status: Confirmed → In Progress
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

This bug was fixed in the package webbrowser-app 0.23+15.04.20160408.1-0ubuntu1 in https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/stable-phone-overlay

---------------

webbrowser-app (0.23+15.04.20160408.1-0ubuntu1) vivid; urgency=medium

  [ CI Train Bot ]
  * Resync trunk.

  [ Loïc Molinari ]
  * Made AddressBar height scalable with regards to the grid units
    system.

  [ Olivier Tilloy ]
  * Add dep8 tests and instructions to run them in qemu or on a phone.
    Original work by Leo Arias and Vincent Ladeuil. added: debian/tests/
    debian/tests/control debian/tests/touch-session-autopilot
  * Catch ESC key event one level up to ensure that it’s not incorrectly
    bubbled up to the outer component. (LP: #1557016)
  * Customize the contents of the media permission dialog to avoid
    truncated text. (LP: #1554220)
  * Do not write the session to a temporary file when no target file is
    defined.
  * Fix a failing unit test with Qt 5.6. (LP: #1565507)
  * Fix broken webapp container autopilot tests. (LP: #1557019)
  * Fix issues with item selection in the downloads page: do not allow
    selecting multiple files when only one is expected do not allow
    entering delete mode (with a long press on an item) while in picker
    mode (LP: #1534112, #1561575)
  * Fix unit tests when run under an sbuild chroot. (LP: #1567294)
  * Import QQuickShortcut from Qt 5.5 to properly handle window-level
    keyboard shortcuts. We cannot bump the dependency on Qt to 5.5 as
    the stable overlay PPA for devices currently has Qt 5.4.1. (LP:
    #1542224, #1545802, #1537782)
  * Make the autopilot tests more reliable when dragging the bottom
    edge. (LP: #1560109)
  * Remove workaround for bug #1526940 that was fixed in the latest
    release of the UITK. (LP: #1526940)
  * Rename debian packages to follow new QML module naming convention.
    (LP: #1342031)
  * Simplify the implementation of HistoryViewWide quite a bit, and as a
    side effect fix a unit test failure when run against the staging
    branch of the UITK. (LP: #1567337)

 -- Olivier Tilloy <email address hidden> Fri, 08 Apr 2016 17:07:04 +0000

Changed in webbrowser-app (Ubuntu RTM):
status: New → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package webbrowser-app - 0.23+16.04.20160408.1-0ubuntu1

---------------
webbrowser-app (0.23+16.04.20160408.1-0ubuntu1) xenial; urgency=medium

  [ CI Train Bot ]
  * Resync trunk.

  [ Loïc Molinari ]
  * Made AddressBar height scalable with regards to the grid units
    system.

  [ Olivier Tilloy ]
  * Add dep8 tests and instructions to run them in qemu or on a phone.
    Original work by Leo Arias and Vincent Ladeuil. added: debian/tests/
    debian/tests/control debian/tests/touch-session-autopilot
  * Catch ESC key event one level up to ensure that it’s not incorrectly
    bubbled up to the outer component. (LP: #1557016)
  * Customize the contents of the media permission dialog to avoid
    truncated text. (LP: #1554220)
  * Do not write the session to a temporary file when no target file is
    defined.
  * Fix a failing unit test with Qt 5.6. (LP: #1565507)
  * Fix broken webapp container autopilot tests. (LP: #1557019)
  * Fix issues with item selection in the downloads page: do not allow
    selecting multiple files when only one is expected do not allow
    entering delete mode (with a long press on an item) while in picker
    mode (LP: #1534112, #1561575)
  * Fix unit tests when run under an sbuild chroot. (LP: #1567294)
  * Import QQuickShortcut from Qt 5.5 to properly handle window-level
    keyboard shortcuts. We cannot bump the dependency on Qt to 5.5 as
    the stable overlay PPA for devices currently has Qt 5.4.1. (LP:
    #1542224, #1545802, #1537782)
  * Make the autopilot tests more reliable when dragging the bottom
    edge. (LP: #1560109)
  * Remove workaround for bug #1526940 that was fixed in the latest
    release of the UITK. (LP: #1526940)
  * Rename debian packages to follow new QML module naming convention.
    (LP: #1342031)
  * Simplify the implementation of HistoryViewWide quite a bit, and as a
    side effect fix a unit test failure when run against the staging
    branch of the UITK. (LP: #1567337)

 -- Olivier Tilloy <email address hidden> Fri, 08 Apr 2016 17:07:04 +0000

Changed in webbrowser-app (Ubuntu):
status: In Progress → Fix Released
Olivier Tilloy (osomon)
Changed in canonical-devices-system-image:
status: Confirmed → Fix Committed
Changed in canonical-devices-system-image:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.