Merge ~osomon/oxide:updated-popup-menu into oxide:master

[Olivier Tilloy]

Improved Oxide.Ubuntu web popup menu UX.

Backported recent webbrowser-app update, by Florian Boucault.

3fbf137... by Olivier Tilloy on 2017-03-13

Add unit tests for keyboard navigation in popup menu.

Unmerged commits

3fbf137... by Olivier Tilloy on 2017-03-13

Add unit tests for keyboard navigation in popup menu.

d0f2173... by Olivier Tilloy on 2017-03-10

Improved Oxide.Ubuntu web popup menu UX.

Backported recent webbrowser-app update, by Florian Boucault.

diff --git a/qt/tests/qmltests/ubuntu_ui/tst_WebViewPopupMenu.qml b/qt/tests/qmltests/ubuntu_ui/tst_WebViewPopupMenu.qml
index ca36c27..2cc02f0 100644
--- a/qt/tests/qmltests/ubuntu_ui/tst_WebViewPopupMenu.qml
+++ b/qt/tests/qmltests/ubuntu_ui/tst_WebViewPopupMenu.qml
@@ -52,7 +52,7 @@ UbuntuTestWebView {
     }
 
     function getPopupMenuEntryAtIndex(index) {
-      return TestSupport.findItemInScene(getPopupMenu(), "webView_WebPopupMenu_item" + index);
+      return TestSupport.findItemInScene(getPopupMenu(), "itemDelegate_%1".arg(index));
     }
 
     function getSelectedIndex(selector) {
@@ -69,13 +69,13 @@ UbuntuTestWebView {
       return [
         { selector: "#select1",
           items: [
-            { type: "header", text: "Group A" },
+            { type: "header", text: "Group A", enabled: true },
             { type: "item", text: "    Test 1", enabled: true },
             { type: "item", text: "    Test 2", enabled: true },
-            { type: "header", text: "Group B" },
+            { type: "header", text: "Group B", enabled: true },
             { type: "item", text: "    Test 3", enabled: true },
             { type: "item", text: "    Test 4", enabled: false },
-            { type: "header", text: "Group C" },
+            { type: "header", text: "Group C", enabled: false },
             { type: "item", text: "    Test 5", enabled: false },
             { type: "item", text: "    Test 6", enabled: false }
           ]
@@ -102,24 +102,21 @@ UbuntuTestWebView {
       var items = [];
       for (var i in listView.children[0].children) {
         var item = listView.children[0].children[i];
-        if (item.z == 0) {
-          // Ignore the highlight item. This isn't a great way to detect this
+        if (item.objectName.indexOf("itemDelegate_") != 0 &&
+            item.objectName.indexOf("headerDelegate_") != 0) {
           continue;
         }
-        items.push(listView.children[0].children[i]);
+        items.push(item);
       }
       items.sort(function(a, b) { return a.y - b.y; });
 
       compare(items.length, data.items.length);
       for (var i in items) {
-        if (data.items[i].type == "item") {
-          compare(items[i].listItemData[0].title.text, data.items[i].text);
-          compare(items[i].enabled, data.items[i].enabled);
-        } else if (data.items[i].type == "header") {
-          compare(items[i].text, data.items[i].text);
-        } else {
-          fail();
-        }
+        var itemType = data.items[i].type;
+        verify(itemType == "item" || itemType == "header");
+        compare(items[i].objectName.indexOf("%1Delegate_".arg(itemType)), 0);
+        compare(items[i].text, data.items[i].text);
+        compare(items[i].enabled, data.items[i].enabled);
       }
     }
 
@@ -187,5 +184,36 @@ UbuntuTestWebView {
       webView2.destroy();
       verify(TestUtils.waitFor(function() { return helper.destroyed; }));
     }
+
+    function test_WebViewPopupMenu6_arrow_key_navigation() {
+      var r = webView.getTestApi().getBoundingClientRectForSelector("#select2");
+      mouseClick(webView, r.x + r.width / 2, r.y + r.height / 2);
+      verify(waitForPopupMenu());
+
+      var listView = TestSupport.findItemInScene(getPopupMenu(), "webView_WebPopupMenu_ListView");
+      compare(listView.currentIndex, 2);
+
+      keyPress(Qt.Key_Up);
+      compare(listView.currentIndex, 1);
+      keyPress(Qt.Key_Up);
+      compare(listView.currentIndex, 0);
+      keyPress(Qt.Key_Up);
+      compare(listView.currentIndex, 0);
+
+      keyPress(Qt.Key_Down);
+      compare(listView.currentIndex, 1);
+      keyPress(Qt.Key_Down);
+      compare(listView.currentIndex, 2);
+      keyPress(Qt.Key_Down);
+      compare(listView.currentIndex, 4);
+      keyPress(Qt.Key_Down);
+      compare(listView.currentIndex, 5);
+      keyPress(Qt.Key_Down);
+      compare(listView.currentIndex, 5);
+
+      keyPress(Qt.Key_Enter);
+      verify(waitForPopupMenuToClose());
+      compare(getSelectedIndex("#select2"), 5);
+    }
   }
 }
diff --git a/qt/uitk/lib/resources/WebPopupMenu.qml b/qt/uitk/lib/resources/WebPopupMenu.qml
index a2987d3..9fa68cf 100644
--- a/qt/uitk/lib/resources/WebPopupMenu.qml
+++ b/qt/uitk/lib/resources/WebPopupMenu.qml
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013-2016 Canonical Ltd.
+// Copyright (C) 2013-2017 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -17,54 +17,192 @@
 
 import QtQuick 2.4
 import Ubuntu.Components 1.3
-import Ubuntu.Components.ListItems 1.3 as ListItems
-import Ubuntu.Components.Popups 1.3 as Popups
-
-Popups.Popover {
-  property var model
+import com.canonical.Oxide 1.5
 
+Item {
   id: root
   objectName: parent.objectName ? parent.objectName + "_WebPopupMenu" : ""
 
   signal selectedItem(int index)
+  signal closed()
+
+  property alias model: listView.model
+  property rect elementBounds
+
+  readonly property var webview: parent
 
-  caller: parent
+  property real contentHeight
+  width: elementBounds.width
+  height: contentHeight
 
-  contentWidth: Math.min(parent.width - units.gu(10), units.gu(40))
+  property real listContentHeight: (listView.count + listView.sectionCount) * (listItemHeight + units.dp(1))
+  property real listItemHeight: units.gu(4)
+  property real addressBarHeight: webview.locationBarController.height
 
-  property real listContentHeight: 0 // intermediate property to avoid binding loop
-  contentHeight: Math.min(parent.height - units.gu(10), listContentHeight)
+  // When the webview's size changes, dismiss the menu.
+  // Ideally we would call updatePosition instead but because elementBounds
+  // is not updated, it would result in incorrect positioning.
+  Connections {
+    target: webview
+    onWidthChanged: closed()
+    onHeightChanged: closed()
+  }
+  onListContentHeightChanged: updatePosition()
+  onAddressBarHeightChanged: updatePosition()
+
+  function updatePosition() {
+    root.x = elementBounds.x;
+    var availableAbove = elementBounds.y - addressBarHeight;
+    var availableBelow = webview.height - elementBounds.y - elementBounds.height;
+
+    if (availableBelow >= listContentHeight || availableBelow >= availableAbove) {
+      // position popover below the box
+      root.contentHeight = Math.min(availableBelow, listContentHeight);
+      root.y = elementBounds.y + elementBounds.height;
+    } else {
+      // position popover above the box
+      root.contentHeight = Math.min(availableAbove, listContentHeight);
+      root.y = elementBounds.y - root.contentHeight;
+    }
+  }
+
+  Keys.onEscapePressed: closed()
+  Keys.onPressed: {
+    // Swallow left and right key events
+    if (event.key == Qt.Key_Left || event.key == Qt.Key_Right) {
+      event.accepted = true;
+    }
+  }
+
+  // Eat mouse events beneath the list so that they never reach the webview below
+  MouseArea {
+    anchors.fill: parent
+    acceptedButtons: Qt.AllButtons
+    onWheel: wheel.accepted = true
+  }
+
+  // Eat mouse events around the list so that they never reach the webview below
+  InverseMouseArea {
+    anchors.fill: parent
+    acceptedButtons: Qt.AllButtons
+    onPressed: selectedItem(listView.currentIndex)
+    onWheel: wheel.accepted = true
+  }
 
   Rectangle {
     anchors.fill: parent
-    color: "#ececec"
+    color: theme.palette.normal.overlay
+    function newColorWithAlpha(color, alpha) {
+      return Qt.rgba(color.r, color.g, color.b, alpha);
+    }
+    border.color: newColorWithAlpha(theme.palette.normal.base, 0.4)
+    border.width: units.dp(1)
   }
 
   ListView {
+    id: listView
+    objectName: root.objectName ? root.objectName + "_ListView" : ""
     clip: true
-    width: root.contentWidth
-    height: root.contentHeight
+    anchors.fill: parent
+    focus: true
+
+    // Forces all delegates to be instantiated so that initialIndex is
+    // set adequately to the index of the selected item
+    cacheBuffer: 32767
+    property int initialIndex
+    Component.onCompleted: {
+      // calling forceLayout ensures that all delegates have been created and
+      // that initialIndex is set adequately as a consequence
+      forceLayout();
+      currentIndex = initialIndex;
+      positionViewAtIndex(initialIndex, ListView.Contain);
+    }
 
-    objectName: root.objectName ? root.objectName + "_ListView" : ""
+    property int sectionCount: 0
 
-    model: root.model
+    Keys.onDownPressed: {
+      var nextIndex = model.nextEnabledItem(currentIndex);
+      if (nextIndex != -1) {
+        currentIndex = nextIndex;
+      }
+    }
+    Keys.onUpPressed: {
+      var prevIndex = model.prevEnabledItem(currentIndex);
+      if (prevIndex != -1) {
+        currentIndex = prevIndex;
+      }
+    }
 
     delegate: ListItem {
-      objectName: root.objectName ? root.objectName + "_item" + index : ""
-
+      objectName: "itemDelegate_%1".arg(model.index)
+      readonly property string text: listItemLayout.title.text
+      enabled: model.enabled
+      height: listItemLayout.height + (divider.visible ? divider.height : 0)
       ListItemLayout {
-        title.text: model.text
+        id: listItemLayout
+        padding {
+          top: 0
+          bottom: 0
+          leading: 0
+          trailing: 0
+        }
+        height: listItemHeight
+        title {
+          height: listItemHeight
+          verticalAlignment: Text.AlignVCenter
+          text: model.text
+          textSize: Label.Small
+          color: enabled ? theme.palette.normal.baseText : theme.palette.disabled.baseText
+        }
+      }
+
+      color: (ListView.isCurrentItem || mouseArea.pressed) ? theme.palette.normal.focus : "transparent"
+      Component.onCompleted: if (model.checked) listView.initialIndex = model.index
+
+      Keys.onReturnPressed: selectedItem(model.index)
+      Keys.onEnterPressed: selectedItem(model.index)
+      Keys.onTabPressed: selectedItem(model.index)
+
+      // Use a separate MouseArea because ListItem.onClicked
+      // is invoked when opening the popup with the Enter key.
+      MouseArea {
+        id: mouseArea
+        anchors.fill: parent
+        enabled: model.enabled
+        onClicked: selectedItem(model.index)
       }
-      enabled: model.enabled
-      selected: model.checked
-      onClicked: selectedItem(model.index)
     }
 
     section.property: "group"
-    section.delegate: ListItems.Header {
-      text: section
+    section.delegate: ListItem {
+      objectName: "headerDelegate_%1".arg(model.index)
+      readonly property string text: listItemLayout.title.text
+      enabled: model.groupEnabled(section)
+      height: listItemHeight + (divider.visible ? divider.height : 0)
+      Component.onCompleted: listView.sectionCount += 1
+      ListItemLayout {
+        id: listItemLayout
+        padding {
+          top: 0
+          bottom: 0
+          leading: 0
+          trailing: 0
+        }
+        height: listItemHeight
+        title {
+          verticalAlignment: Text.AlignVCenter
+          height: listItemHeight
+          text: section
+          textSize: Label.Small
+          font.bold: true
+          color: enabled ? theme.palette.normal.baseText : theme.palette.disabled.baseText
+        }
+      }
     }
+  }
 
-    onContentHeightChanged: root.listContentHeight = contentHeight
+  Scrollbar {
+    flickableItem: listView
+    align: Qt.AlignTrailing
   }
 }
diff --git a/qt/uitk/lib/uitk_web_popup_menu.cc b/qt/uitk/lib/uitk_web_popup_menu.cc
index 7175169..ac18dc5 100644
--- a/qt/uitk/lib/uitk_web_popup_menu.cc
+++ b/qt/uitk/lib/uitk_web_popup_menu.cc
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2016 Canonical Ltd.
+// Copyright (C) 2016-2017 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -36,6 +36,8 @@ namespace uitk {
 using qt::MenuItem;
 
 class WebPopupMenu::Model : public QAbstractListModel {
+  Q_OBJECT
+
  public:
   Model(const std::vector<MenuItem>& items);
   ~Model() override;
@@ -55,6 +57,10 @@ class WebPopupMenu::Model : public QAbstractListModel {
                 int role = Qt::DisplayRole) const override;
   QHash<int, QByteArray> roleNames() const override;
 
+  Q_INVOKABLE int nextEnabledItem(int currentIndex) const;
+  Q_INVOKABLE int prevEnabledItem(int currentIndex) const;
+  Q_INVOKABLE bool groupEnabled(const QString& group) const;
+
  private:
   struct Item {
     QString label;
@@ -137,12 +143,43 @@ QHash<int, QByteArray> WebPopupMenu::Model::roleNames() const {
   return roles;
 }
 
+int WebPopupMenu::Model::nextEnabledItem(int currentIndex) const {
+  int index = currentIndex + 1;
+  while (index < items_.count()) {
+    if (items_.at(index).enabled) {
+      return index;
+    }
+    ++index;
+  }
+  return -1;
+}
+
+int WebPopupMenu::Model::prevEnabledItem(int currentIndex) const {
+  int index = currentIndex - 1;
+  while (index >= 0) {
+    if (items_.at(index).enabled) {
+      return index;
+    }
+    --index;
+  }
+  return -1;
+}
+
+bool WebPopupMenu::Model::groupEnabled(const QString& group) const {
+  for (const auto& item : items_) {
+    if (item.group == group && item.enabled) {
+      return true;
+    }
+  }
+  return false;
+}
+
 void WebPopupMenu::Show() {
-  QMetaObject::invokeMethod(item_.get(), "show");
+  item_->setVisible(true);
 }
 
 void WebPopupMenu::Hide() {
-  QMetaObject::invokeMethod(item_.get(), "hide");
+  item_->setVisible(false);
 }
 
 WebPopupMenu::WebPopupMenu(const std::vector<MenuItem>& items,
@@ -192,22 +229,20 @@ bool WebPopupMenu::Init(QQuickItem* parent) {
   }
 
   item_->setProperty("model", QVariant::fromValue(model_.get()));
+  item_->setProperty("elementBounds", bounds_);
   item_->setParentItem(parent);
 
   component.completeCreate();
 
-  connect(item_.get(), &QQuickItem::visibleChanged,
-          this, &WebPopupMenu::OnVisibleChanged);
+  connect(item_.get(), SIGNAL(closed()), this, SLOT(OnClosed()));
   connect(item_.get(), SIGNAL(selectedItem(int)),
           this, SLOT(OnItemSelected(int)));
 
   return true;
 }
 
-void WebPopupMenu::OnVisibleChanged() {
-  if (!item_->isVisible()) {
-    client_->cancel();
-  }
+void WebPopupMenu::OnClosed() {
+  client_->cancel();
 }
 
 void WebPopupMenu::OnItemSelected(int index) {
@@ -235,3 +270,5 @@ WebPopupMenu::~WebPopupMenu() = default;
 
 } // namespace uitk
 } // namespace oxide
+
+#include "uitk_web_popup_menu.moc"
diff --git a/qt/uitk/lib/uitk_web_popup_menu.h b/qt/uitk/lib/uitk_web_popup_menu.h
index d1553d2..5fa2772 100644
--- a/qt/uitk/lib/uitk_web_popup_menu.h
+++ b/qt/uitk/lib/uitk_web_popup_menu.h
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2016 Canonical Ltd.
+// Copyright (C) 2016-2017 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -56,8 +56,8 @@ class WebPopupMenu : public QObject,
   ~WebPopupMenu() override;
 
  private Q_SLOTS:
-  void OnVisibleChanged();
   void OnItemSelected(int index);
+  void OnClosed();
 
  private:
   WebPopupMenu(const std::vector<qt::MenuItem>& items,