Merge into staging : textinputs-rightclick : Code : Ubuntu UI Toolkit

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration	2014-04-16	Approve on 2014-04-28
Cris Dywan		2014-04-16	Approve on 2014-04-28
Review via email: mp+216008@code.launchpad.net

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-10: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) wrote on 2014-04-10: Posted in a previous version of this proposal

#

Problems found while testing:
- Right-click whilst menu is visible shows another menu without closing the previous one(s). It should always close the first menu and then behave exactly as if there was none.
- Select, right-click, selection is lost. Selection should remain before and after showing the menu - unless it's modified due to menu items obviously.

These apply to both components.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-10: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/4/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Zsombor Egri (zsombi) wrote on 2014-04-15: Posted in a previous version of this proposal

#

> Problems found while testing:
> - Right-click whilst menu is visible shows another menu without closing the
> previous one(s). It should always close the first menu and then behave exactly
> as if there was none.
> - Select, right-click, selection is lost. Selection should remain before and
> after showing the menu - unless it's modified due to menu items obviously.
>
> These apply to both components.

The second one should be fixed now. The first one requires changes in the Popover mouse handling as that doe snot listen on right mouse events at all.

Revision history for this message

Cris Dywan (kalikiana) wrote on 2014-04-15: Posted in a previous version of this proposal

#

Selection is working indeed.

- Multiple right-clicks still accumulate menus. I can see that it spans over different entries so I see how it's a Popover issue. Though to be honest it's a pretty serious bug.
- I can't get a menu if I right-click somewhere with no text - all the "Single line" entries in the gallery only work if you pay attention to aim at the text or respectively type something first and then click that.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-15: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/28/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Zsombor Egri (zsombi) wrote on 2014-04-16: Posted in a previous version of this proposal

#

> Selection is working indeed.
>
> - Multiple right-clicks still accumulate menus. I can see that it spans over
> different entries so I see how it's a Popover issue. Though to be honest it's
> a pretty serious bug.

It is a popover issue. Popovers get dismissed only on left-clicks. Right click do not have any effect on them. The same reason is that right clicks are not handled by a mouse area which does not accept right buttons.
It is a one-liner after all, but needs test cases, which makes it bigger and would be better to solve it in a separate MR rather than in this one. The question is whether we do it after this MR or before?

> - I can't get a menu if I right-click somewhere with no text - all the "Single
> line" entries in the gallery only work if you pay attention to aim at the text
> or respectively type something first and then click that.

The same happens with TextArea, if you right-click on the frame or on the gap between the frame and the text. The MouseArea that takes care of the frame does not handle/forward right clicks.

Revision history for this message

Zsombor Egri (zsombi) wrote on 2014-04-16:

#

re-submitted with a different prerequisite.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-16:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/30/rebuild

review: Needs Fixing (continuous-integration)

lp:~zsombi/ubuntu-ui-toolkit/textinputs-rightclick updated on 2014-04-17

1020. By Zsombor Egri on 2014-04-17: staging merge

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-17:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/45/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) wrote on 2014-04-17:

#

In the latest revision the selection is cleared when right-clicking outside of the selected text

1. Select some text
2. Right-click somewhere in the unselected area
3. Should be able to cut the selected text

review: Needs Fixing

lp:~zsombi/ubuntu-ui-toolkit/textinputs-rightclick updated on 2014-04-24

1021. By Zsombor Egri on 2014-04-24: prereq merge

Revision history for this message

Zsombor Egri (zsombi) wrote on 2014-04-24:

#

> In the latest revision the selection is cleared when right-clicking outside of
> the selected text
>
> 1. Select some text
> 2. Right-click somewhere in the unselected area
> 3. Should be able to cut the selected text

One of the requirements in the bug was that right click must work in the same way as the pressAndHold does. pressAndHold works in this way even in the prerequisite.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-24:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/90/rebuild

review: Approve (continuous-integration)

lp:~zsombi/ubuntu-ui-toolkit/textinputs-rightclick updated on 2014-04-25

1022. By Zsombor Egri on 2014-04-25: prereq merge

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-25:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/99/rebuild

review: Approve (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) wrote on 2014-04-25:

#

> > In the latest revision the selection is cleared when right-clicking outside
> of
> > the selected text
> >
> > 1. Select some text
> > 2. Right-click somewhere in the unselected area
> > 3. Should be able to cut the selected text
>
> One of the requirements in the bug was that right click must work in the same
> way as the pressAndHold does. pressAndHold works in this way even in the
> prerequisite.

The bug literally says "right-click should trigger long press". It has no description at all. My best guess is that any menu you will trigger via long press should also be invoked via right-click. This is what I would expect. I'm doubtful it implies deviating from correct right-click behavior on GTK+ apps.

The behavior is as it was in my last test run, so I'm afraid this is still wrong until we reach a conclusion.

review: Needs Fixing

Revision history for this message

Florian Boucault (fboucault) wrote on 2014-04-25:

#

> > > In the latest revision the selection is cleared when right-clicking
> outside
> > of
> > > the selected text
> > >
> > > 1. Select some text
> > > 2. Right-click somewhere in the unselected area
> > > 3. Should be able to cut the selected text
> >
> > One of the requirements in the bug was that right click must work in the
> same
> > way as the pressAndHold does. pressAndHold works in this way even in the
> > prerequisite.
>
> The bug literally says "right-click should trigger long press". It has no
> description at all. My best guess is that any menu you will trigger via long
> press should also be invoked via right-click. This is what I would expect. I'm
> doubtful it implies deviating from correct right-click behavior on GTK+ apps.
>
> The behavior is as it was in my last test run, so I'm afraid this is still
> wrong until we reach a conclusion.

Right click on desktop in QML/toolkit apps should behave like their GTK equivalent unless a strong case against it can be made.
I would even say that the long press on device should mimic what the right click does.

Revision history for this message

Zsombor Egri (zsombi) wrote on 2014-04-25:

#

> > > > In the latest revision the selection is cleared when right-clicking
> > outside
> > > of
> > > > the selected text
> > > >
> > > > 1. Select some text
> > > > 2. Right-click somewhere in the unselected area
> > > > 3. Should be able to cut the selected text
> > >
> > > One of the requirements in the bug was that right click must work in the
> > same
> > > way as the pressAndHold does. pressAndHold works in this way even in the
> > > prerequisite.
> >
> > The bug literally says "right-click should trigger long press". It has no
> > description at all. My best guess is that any menu you will trigger via long
> > press should also be invoked via right-click. This is what I would expect.
> I'm
> > doubtful it implies deviating from correct right-click behavior on GTK+
> apps.
> >
> > The behavior is as it was in my last test run, so I'm afraid this is still
> > wrong until we reach a conclusion.
>
> Right click on desktop in QML/toolkit apps should behave like their GTK
> equivalent unless a strong case against it can be made.
> I would even say that the long press on device should mimic what the right
> click does.

I can make it so right click behaves like other GTK+ apps do, however I need to know which one to consider. I tried two apps, gedit and LibreOffice, they both behave differently.

gedit:
- rigth click over text opens the context menu, does not move the cursor
- right click over area where there's no text works the same as previous
- right click over selected text opens context menu
- rigth click anywhere else but selected text opens context menu, preserves selection

LibreOffice write:
- right click over a text selects the word and opens the context menu
- rigth click over area with no text moves the cursor to the closest position in the line, opens context menu
- rigth click over selected text opens context menu
- rigth click anywhere else but selected text clears selection, moves the cursor to the closest place to the click and opens popover

Our logic behaves as LibreOffice. On touch devices it also behaves like this. If I do both desktop and touch to behave the same way, I will introduce a regression, as pressAndHold so far moved the cursor and opened the context menu in place.

Now what?

> > > > In the latest revision the selection is cleared when right-clicking
> > outside
> > > of
> > > > the selected text
> > > >
> > > > 1. Select some text
> > > > 2. Right-click somewhere in the unselected area
> > > > 3. Should be able to cut the selected text
> > >
> > > One of the requirements in the bug was that right click must work in the
> > same
> > > way as the pressAndHold does. pressAndHold works in this way even in the
> > > prerequisite.
> >
> > The bug literally says "right-click should trigger long press". It has no
> > description at all. My best guess is that any menu you will trigger via long
> > press should also be invoked via right-click. This is what I would expect.
> I'm
> > doubtful it implies deviating from correct right-click behavior on GTK+
> apps.
> >
> > The behavior is as it was in my last test run, so I'm afraid this is still
> > wrong until we reach a conclusion.
> 
> Right click on desktop in QML/toolkit apps should behave like their GTK
> equivalent unless a strong case against it can be made.
> I would even say that the long press on device should mimic what the right
> click does.

I can make it so right click behaves like other GTK+ apps do, however I need to know which one to consider. I tried two apps, gedit and LibreOffice, they both behave differently.

gedit:
- rigth click over text opens the context menu, does not move the cursor
- right click over area where there's no text works the same as previous
- right click over selected text opens context menu
- rigth click anywhere else but selected text opens context menu, preserves selection

LibreOffice write:
- right click over a text selects the word and opens the context menu
- rigth click over area with no text moves the cursor to the closest position in the line, opens context menu  
- rigth click over selected text opens context menu
- rigth click anywhere else but selected text clears selection, moves the cursor to the closest place to the click and opens popover

Our logic behaves as LibreOffice. On touch devices it also behaves like this. If I do both desktop and touch to behave the same way, I will introduce a regression, as pressAndHold so far moved the cursor and opened the context menu in place.

Now what?

Revision history for this message

Zsombor Egri (zsombi) wrote on 2014-04-28:

#

> > > In the latest revision the selection is cleared when right-clicking
> outside
> > of
> > > the selected text
> > >
> > > 1. Select some text
> > > 2. Right-click somewhere in the unselected area
> > > 3. Should be able to cut the selected text
> >
> > One of the requirements in the bug was that right click must work in the
> same
> > way as the pressAndHold does. pressAndHold works in this way even in the
> > prerequisite.
>
> The bug literally says "right-click should trigger long press". It has no
> description at all. My best guess is that any menu you will trigger via long
> press should also be invoked via right-click. This is what I would expect. I'm
> doubtful it implies deviating from correct right-click behavior on GTK+ apps.
>
> The behavior is as it was in my last test run, so I'm afraid this is still
> wrong until we reach a conclusion.

That's what actually does: the right click invokes whatever functionality the pressAndHold has.

If I'd do the same way as GTK+ app does, that would mean I would need to have a huge change on the Popovers as those cannot be opened on a free (x,y) coordinate, they can be opened attached to a caller. So the caller must be positioned prior to open the popover.

OTOH, the direction on pressAndHold was to follow Android/iOS. They do it as we do with pressAndHold.

Revision history for this message

Zoltan Balogh (bzoltan) wrote on 2014-04-28:

#

> Right click on desktop in QML/toolkit apps should behave like their GTK equivalent unless a strong case against it can be made.

I would be happy to hear a properly explained verion of that statement :) In my view the GTK is an obsolate technology on Desktop and I would keep only those UX and User Input paradigms that make sense. The GTK heritage should not limit our design and UX plans we have for the converged UITK.

I would vote on that right click on the desktop does the same as long press on the touch device

lp:~zsombi/ubuntu-ui-toolkit/textinputs-rightclick updated on 2014-04-28

1023. By Zsombor Egri on 2014-04-28: staging merge
1024. By Zsombor Egri on 2014-04-28: import version fix

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-28:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/111/rebuild

review: Approve (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) wrote on 2014-04-28:

#

> GTK heritage should not limit our design

I've thought about that over a cup of tea. The toolkit menu is actually positioning itself more predictably than legacy desktop menus. My biggest concern in an unbiased point of view amounts to cases where directly hitting the selection is hard a) missing grid units on the desktop b) having a small selection - the legacy behavior hides those issues without adressing them.
So, let's proceed without prejudice.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2014-04-28:

#

review: Approve (continuous-integration)

Ubuntu UI Toolkit

Merge lp:~zsombi/ubuntu-ui-toolkit/textinputs-rightclick into lp:ubuntu-ui-toolkit/staging

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'modules/Ubuntu/Components/InputHandler.qml'
 --- modules/Ubuntu/Components/InputHandler.qml	2014-04-15 15:25:09 +0000
 +++ modules/Ubuntu/Components/InputHandler.qml	2014-04-28 06:48:59 +0000
@@ -15,7 +15,7 @@
   */
  import QtQuick 2.0
--import Ubuntu.Components 0.1 as Ubuntu
++import Ubuntu.Components 1.1
  /*
    This component is a unified text selection and scrolling handler for both
@@ -259,8 +259,8 @@
+     }
      // Mouse handling
--    Ubuntu.Mouse.forwardTo: [main]
--    Ubuntu.Mouse.onPressed: {
++    Mouse.forwardTo: [main]
++    Mouse.onPressed: {
          if (input.activeFocus) {
              // start selection timeout
              selectionTimeout.restart();
@@ -270,7 +270,7 @@
          // consume event so it does not get forwarded to the input
          mouse.accepted = true;
+     }
--    Ubuntu.Mouse.onReleased: {
++    Mouse.onReleased: {
          if (!main.focus && !main.activeFocusOnPress) {
              return;
+         }
@@ -283,8 +283,13 @@
+         }
          moveStarts = moveEnds = -1;
          state = "";
++        // check if we get right-click from the frame or the area that has no text
++        if (mouse.button === Qt.RightButton) {
++            // open the popover
++            inputHandler.pressAndHold(input.cursorPosition);
++        }
+     }
--    Ubuntu.Mouse.onPositionChanged: {
++    Mouse.onPositionChanged: {
          // leave if not focus, not the left button or not in select state
          if (!input.activeFocus || (mouse.button !== Qt.LeftButton) || (state !== "select") || !main.selectByMouse) {
              return;
@@ -293,13 +298,20 @@
          selectionTimeout.running = false;
          selectText(mouse);
+     }
--    Ubuntu.Mouse.onDoubleClicked: {
++    Mouse.onDoubleClicked: {
          if (main.selectByMouse) {
              input.selectWord();
              // turn selection state temporarily so the selection is not cleared on release
              state = "selection";
+         }
+     }
--    Ubuntu.Mouse.onPressAndHold: openContextMenu(mouse)
++    Mouse.onPressAndHold: openContextMenu(mouse)
++    // right button handling
++    MouseArea {
++        anchors.fill: parent
++        acceptedButtons: Qt.RightButton
++        // trigger pressAndHold
++        onReleased: openContextMenu(mouse)
++    }
+ }
 === modified file 'modules/Ubuntu/Components/TextArea.qml'
 --- modules/Ubuntu/Components/TextArea.qml	2014-04-25 12:53:58 +0000
 +++ modules/Ubuntu/Components/TextArea.qml	2014-04-28 06:48:59 +0000
@@ -15,7 +15,7 @@
   */
  import QtQuick 2.0
--import Ubuntu.Components 0.1 as Ubuntu
++import Ubuntu.Components 1.1 as Ubuntu
  import "mathUtils.js" as MathUtils
  /*!
@@ -740,6 +740,7 @@
      MouseArea {
          anchors.fill: parent
          enabled: internal.frameSpacing > 0
++        acceptedButtons: Qt.LeftButton | Qt.RightButton
          // activate input when pressed on the frame
          preventStealing: false
          Ubuntu.Mouse.forwardTo: [inputHandler]
 === modified file 'modules/Ubuntu/Components/TextCursor.qml'
 --- modules/Ubuntu/Components/TextCursor.qml	2014-04-09 12:05:49 +0000
 +++ modules/Ubuntu/Components/TextCursor.qml	2014-04-28 06:48:59 +0000
@@ -15,7 +15,7 @@
   */
  import QtQuick 2.0
--import "Popups" 0.1
++import Ubuntu.Components.Popups 1.0
  StyledItem {
      id: cursorItem
 === modified file 'modules/Ubuntu/Components/TextField.qml'
 --- modules/Ubuntu/Components/TextField.qml	2014-04-25 12:53:58 +0000
 +++ modules/Ubuntu/Components/TextField.qml	2014-04-28 06:48:59 +0000
@@ -16,7 +16,7 @@
  import QtQuick 2.0
  import Ubuntu.Unity.Action 1.1 as UnityActions
--import Ubuntu.Components 0.1 as Ubuntu
++import Ubuntu.Components 1.1 as Ubuntu
  /*!
      \qmltype TextField
@@ -840,7 +840,8 @@
      // grab clicks from the area between the frame and the input
      MouseArea {
          anchors.fill: parent
--        // us it only when there is space between the frame and input
++        acceptedButtons: Qt.LeftButton | Qt.RightButton
++        // use it only when there is space between the frame and input
          enabled: internal.spacing > 0
          preventStealing: false
          // forward mouse events to input so we can handle those uniformly
 === modified file 'modules/Ubuntu/Components/TextInputPopover.qml'
 --- modules/Ubuntu/Components/TextInputPopover.qml	2014-04-23 08:50:20 +0000
 +++ modules/Ubuntu/Components/TextInputPopover.qml	2014-04-28 06:48:59 +0000
@@ -16,7 +16,7 @@
  import QtQuick 2.0
  import Ubuntu.Components 1.1 as Toolkit
--import "Popups" 0.1
++import Ubuntu.Components.Popups 1.0
  ActionSelectionPopover {
      objectName: "text_input_popover"
 === modified file 'tests/unit_x11/tst_components/tst_textarea.qml'
 --- tests/unit_x11/tst_components/tst_textarea.qml	2014-04-25 12:53:58 +0000
 +++ tests/unit_x11/tst_components/tst_textarea.qml	2014-04-28 06:48:59 +0000
@@ -481,7 +481,6 @@
              compare(Qt.inputMethod.visible, true, "OSK shown");
+         }
--        // make it to b ethe last test case executed
          function test_TextareaInListItem_RichTextEnterCaptured() {
              textArea.text = "a<br />b";
              textArea.textFormat = TextEdit.RichText;
@@ -719,5 +718,43 @@
              mouseClick(longText, units.gu(10), y);
              verify(longText.selectedText === "", "There is still selected text");
+         }
++
++        function test_rightclick_opens_popover_when_not_focused() {
++            var handler = findChild(longText, "input_handler");
++            var x = longText.width / 2;
++            var y = longText.height / 2;
++            popoverSpy.target = handler;
++            popoverSpy.clear();
++
++            // rclick should bring popover in
++            mouseClick(longText, x, y, Qt.RightButton);
++            waitForRendering(longText);
++            popoverSpy.wait();
++            compare(longText.focus, true, "The input was not focused");
++            compare(handler.state, "", "The input is not in default state.");
++
++            // take the popover away, that should bring the handler back to default state
++            mouseClick(main, 0, 0);
++            compare(handler.state, "", "The input has not returned to default state.");
++        }
++
++        function test_rightclick_opens_popover_when_focused() {
++            longText.focus = true;
++            var handler = findChild(longText, "input_handler");
++            var x = longText.width / 2;
++            var y = longText.height / 2;
++            popoverSpy.target = handler;
++            popoverSpy.clear();
++
++            // rclick should bring popover in
++            mouseClick(longText, x, y, Qt.RightButton);
++            waitForRendering(longText);
++            popoverSpy.wait();
++            compare(handler.state, "", "The input is not in default state.");
++
++            // take the popover away, that should bring the handler back to default state
++            mouseClick(main, 0, 0);
++            compare(handler.state, "", "The input has not returned to default state.");
++        }
+     }
+ }
 === modified file 'tests/unit_x11/tst_components/tst_textfield.qml'
 --- tests/unit_x11/tst_components/tst_textfield.qml	2014-04-25 12:53:58 +0000
 +++ tests/unit_x11/tst_components/tst_textfield.qml	2014-04-28 06:48:59 +0000
@@ -732,5 +732,44 @@
              mouseClick(longText, units.gu(10), y);
              verify(longText.selectedText === "");
+         }
++
++        function test_rightclick_opens_popover_when_not_focused() {
++            var handler = findChild(longText, "input_handler");
++            var x = longText.width / 2;
++            var y = longText.height / 2;
++            popoverSpy.target = handler;
++            popoverSpy.clear();
++
++            // rclick should bring popover in
++            mouseClick(longText, x, y, Qt.RightButton);
++            waitForRendering(longText);
++            popoverSpy.wait();
++            // and also set the focus
++            compare(longText.focus, true, "Component haven't got focused");
++            compare(handler.state, "", "The input is not in inactive state.");
++
++            // take the popover away, that should bring the handler back to default state
++            mouseClick(textItem, 0, 0);
++            compare(handler.state, "", "The input has not returned to default state.");
++        }
++
++        function test_rightclick_opens_popover_when_focused() {
++            longText.focus = true;
++            var handler = findChild(longText, "input_handler");
++            var x = longText.width / 2;
++            var y = longText.height / 2;
++            popoverSpy.target = handler;
++            popoverSpy.clear();
++
++            // rclick should bring popover in
++            mouseClick(longText, x, y, Qt.RightButton);
++            waitForRendering(longText);
++            popoverSpy.wait();
++            compare(handler.state, "", "The input is not in default state.");
++
++            // take the popover away, that should bring the handler back to default state
++            mouseClick(textItem, 0, 0);
++            compare(handler.state, "", "The input has not returned to default state.");
++        }
+     }
+ }