Bug 103158 - Pressing ctrl+shift switches direction on key down
Summary: Pressing ctrl+shift switches direction on key down
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
5.2.0.4 release
Hardware: All All
: medium normal
Assignee: Maxim Monastirsky
URL:
Whiteboard: target:5.4.0 target:5.3.4
Keywords: bibisected, bisected, regression
: 107284 107629 (view as bug list)
Depends on:
Blocks: RTL-CTL
  Show dependency treegraph
 
Reported: 2016-10-12 13:43 UTC by Yousuf Philips (jay) (retired)
Modified: 2017-05-25 19:43 UTC (History)
11 users (show)

See Also:
Crash report or crash signature:


Attachments
Revert part of tdf#92630 (1.11 KB, patch)
2016-10-21 00:53 UTC, coypu
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yousuf Philips (jay) (retired) 2016-10-12 13:43:44 UTC
Steps:
1) Start Writer with CTL enabled (tools > options > language settings > languages)
2) Click right-to-left button in toolbar
3) Switch keyboard layout to arabic
4) Type some arabic words
5) Press shift+ctrl and it activates left-to-right which has the shift+ctrl+A shortcut

Regression as this works correctly in 4.3.

Version: 5.3.0.0.alpha0+
Build ID: f309531cfe1d6a1b6ea1306d45ed3e121145bc5f
CPU Threads: 2; OS Version: Linux 3.19; UI Render: default; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2016-10-04_03:52:18
Locale: en-US (en_US.UTF-8); Calc: group
Comment 1 Maxim Monastirsky 2016-10-19 21:51:36 UTC
That's a feature, see here:

http://opengrok.libreoffice.org/xref/core/sw/source/uibase/docvw/edtwin.cxx#5535

But this code is there since 2002, so not sure what was changed since 4.3.

BTW Ctrl+Shift is a standard shortcut in Windows to change text direction (in Notepad, Word etc.). Left Ctrl+Shift change to LTR, and the right ones to RTL.
Comment 2 Yousuf Philips (jay) (retired) 2016-10-19 22:54:11 UTC
(In reply to Maxim Monastirsky from comment #1)
> That's a feature, see here:
> 
> http://opengrok.libreoffice.org/xref/core/sw/source/uibase/docvw/edtwin.
> cxx#5535
> 
> But this code is there since 2002, so not sure what was changed since 4.3.

Well that is strange.

> BTW Ctrl+Shift is a standard shortcut in Windows to change text direction
> (in Notepad, Word etc.). Left Ctrl+Shift change to LTR, and the right ones
> to RTL.

The standard shortcut on windows is Left Alt+Shift for switching back and forth between RTL/LTR languages, atleast thats what i've been using for years. I use the same one on linux.

http://www.digitalcitizen.life/sites/default/files/img/win8_inputlang/change_keyboard10.png

But with ctrl+shift toggling LTR rather than only working with its defined shortcut key, it makes it quite a chore to select text with the keyboard for RTL users, because one it switches to LTR you have to select in the opposite direction to get what you were expecting to select and you also have to switch the paragraph back to RTL once you are finished.
Comment 3 Maxim Monastirsky 2016-10-20 07:35:37 UTC
(In reply to Yousuf Philips (jay) from comment #2)
> The standard shortcut on windows is Left Alt+Shift for switching back and
> forth between RTL/LTR languages,
Alt+Shift is for switching input languages (not just RTL/LTR), Ctrl+Shift is for changing direction. These two are unrelated:

https://blogs.technet.microsoft.com/office_global_experience/2010/02/19/useful-tricks-and-shortcuts-for-users-around-the-world/

As I said it also works in Notepad, and even flips the side of the scrollbar. It has the same effect as choosing "Right to left Reading order" from the context menu.

> But with ctrl+shift toggling LTR rather than only working with its defined
> shortcut key, it makes it quite a chore to select text with the keyboard for
> RTL users, because one it switches to LTR you have to select in the opposite
> direction to get what you were expecting to select and you also have to
> switch the paragraph back to RTL once you are finished.
Then the solution would be to make Ctrl+Shift work on key-up event, similar to Windows, as now it seems to work on key-down, and so is toggling every time you try to use some defined Ctrl+Shift+something shortcut.
Comment 4 Yousuf Philips (jay) (retired) 2016-10-20 16:39:22 UTC
(In reply to Maxim Monastirsky from comment #3)
> Alt+Shift is for switching input languages (not just RTL/LTR), Ctrl+Shift is
> for changing direction. These two are unrelated:

Yep true, i normally never use the keyboard for that option. :D

> Then the solution would be to make Ctrl+Shift work on key-up event, similar
> to Windows, as now it seems to work on key-down, and so is toggling every
> time you try to use some defined Ctrl+Shift+something shortcut.

Yes i was going to suggest that in my last comment but left it out, as i wasnt sure if it was possible to do that as i tested Ctrl+B and it takes affect on key-down and not key release.
Comment 5 Yousuf Philips (jay) (retired) 2016-10-20 19:12:01 UTC
This also negatively affects users who are using the Ctrl + Shift combination with other keyboard layouts (bug 88027).
Comment 6 Roman Kuznetsov 2016-10-20 19:30:19 UTC
(In reply to Yousuf Philips (jay) from comment #5)
> This also negatively affects users who are using the Ctrl + Shift
> combination with other keyboard layouts (bug 88027).

hmm.. i don't think, that my bug and this bug linked. In my bug shortcut simple don't work in RU keyboard layout (by default on my computer). Even shortcut "Ctrl + ," without key "Shift". And after switch keyboard layout to EN (Ctrl+Shift), shortcuts work.
Comment 7 Maxim Monastirsky 2016-10-20 20:45:13 UTC
So I did some testing with old versions:

4.3.7, 4.4.3, 5.0.6 - Ctrl+Shift works with key-up (correct).
5.1.5, 5.1.6 - does not work at all.
5.2.2, master - works with key-down (wrong).

So would be interesting to find out both what broke it in 5.1, and what "fixed" it in 5.2 the wrong way...
Comment 8 Maxim Monastirsky 2016-10-20 21:16:58 UTC
bisect points to:

commit 8d53d01f38b856f177aca3ed4d3cba3db10f24a5
Author: Katarina Behrens <Katarina.Behrens@cib.de>
Date:   Wed Jan 27 15:46:41 2016 +0100

    tdf#96739: Send Ctrl-Left/RightShift events to correct window

Adding Cc: Katarina Behrens
Comment 9 coypu 2016-10-21 00:53:13 UTC
Created attachment 128113 [details]
Revert part of tdf#92630

Hello, I reported the bug referenced here - tdf#96739.
It is for "ctrl+shift does not work at all", and so is the commit
mentioned.

as per comment #6 of said bug, the offending commit is: 74407aef94b6d8dfdd69891c4a6e578587ef3e71 (tdf#92630)

the check removed in commit 74407aef94b6d8dfdd69891c4a6e578587ef3e71 (tdf#92630)

-    // send modkey events only if useful data is available
-    if( pEvent->mnModKeyCode != 0 )

in core/vcl/unx/generic/window/salframe.cxx:3040-3050 we can see
that mnModKeyCode is only non-zero if( pEvent->type == KeyRelease ...

this ensured that the send is only for keyrelease, which caused this regression.

while here, write a better comment for the reasoning for the check.

I'm fairly confident this will help, so I am sending it without checking.
Sorry for that - it will take me a very long time to confirm it is correct on
my slow hardware.
Comment 10 coypu 2016-11-01 22:34:02 UTC
The patch doesn't seem to have the desired effect (besides the regression)
Maybe better luck next time, sorry for the noise
Comment 11 Katarina Behrens (Inactive) 2016-11-02 10:49:41 UTC
> The patch doesn't seem to have the desired effect (besides the regression)
> Maybe better luck next time, sorry for the noise

Well the patch goes in the right direction and it's easy to improve it, so why not give it a try and get a badge for contributing libreoffice code ;-) in the month of libreoffice
Comment 12 Anass Ahmed 2016-11-19 12:39:10 UTC
It happens to me too, I use CTL (Arabic) language with GTK3 LibreOffice 5.2.x under Fedora 25 Wayland.

It's really annoying to use any Ctrl+Shift+Anything combination because it activates the switching without me lifting my fingers from the buttons yet!! (i.e. key up)

The most annoying part is that it also affects next Ctrl uses (on key down). If I pressed "Left/Right Shift" alone and lift my finger (key up) then used "Ctrl" (key down) It switches text direction immediately!! (This reminds me of an accessibility option to press key combinations without combining them, but I didn't enable it)

I didn't find the shortcut in the "Customize > Shortcuts" window to disable it, I only found the "Ctrl+Shift+D" and "Ctrl+Shift+A" shortcuts.
Comment 13 aliva 2017-01-28 13:04:49 UTC
I have similar issue, when I press ctrl (for using save, copy, paste, ...) , writer changes line direction to ltr

writer help -> about:
Version: 5.2.2.2
Build ID: 1:5.2.2-0ubuntu2
CPU Threads: 4; OS Version: Linux 4.8; UI Render: default; 
Locale: fa-IR (fa_IR.UTF-8); Calc: group

lsb_release -a:
Distributor ID: Ubuntu
Description:    Ubuntu 16.10
Release:        16.10
Codename:       yakkety


desktop:
xfce (xubuntu)
Comment 14 ⁨خالد حسني⁩ 2017-04-23 01:43:06 UTC
*** Bug 107284 has been marked as a duplicate of this bug. ***
Comment 15 Nadav Har'El 2017-04-23 04:29:17 UTC
The fact that this bug has been open for half a year although it is already understood what causes it leads me to conclude that the people involved underestimate its severity or believe it bothers only few people.

As an RTL user, let me tell you that this is a very serious issue, because it causes paragraphs' directions to spontaneously flip on the mere press of the "control" key, as explained previously in comments here, and in the duplicate I created, bug 107284. The effect is very subtle - you edit a long document, and suddenly the direction of one of the paragraph "flips". For a long paragraph, often the only change visible is the alignment of the paragraph's last line, which you can sometimes miss and only notice later. Moreover, it's not clear *why* this happened. I've seen this problem dozens of times over the last few months, and even added a shortcut for "correcting" it (making a paragraph RTL again), before I figured out what (the "control" key) can actually trigger this problem.

I'm guessing that 99% of the RTL users (Hebrew and Arabic writers) have come across this problem in recent months, without being able to explain it, and certainly not figuring out that *this* issue is about the same problem. So it's not surprising we don't see a hundreds voters or duplicates on this issue. But it's still extremely annoying.

It's *not* a feature - it's a bug. Nobody wants a "feature" which flips a paragraph direction on the press of a key. Flipping a paragraph's direction is very rarely a useful operation - and doing it with just the "control" key (just because previously you also pressed, somehow, shift, in an unrelated operation) is absurd.
Comment 16 Yousuf Philips (jay) (retired) 2017-04-23 08:44:51 UTC
@Khaled, @Maxim, @Bubli: Can we do something with the patch in comment 9 to move this issue along.
Comment 17 Maxim Monastirsky 2017-05-05 07:09:10 UTC
An attempt to solve this bug is on gerrit:

https://gerrit.libreoffice.org/37275/
Comment 18 Commit Notification 2017-05-05 09:28:52 UTC
Maxim Monastirsky committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=fe0451259d2fb93c809c1bfa3baf5abd90019c58

tdf#103158 ctrl+shift should work on key up

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 19 Maxim Monastirsky 2017-05-05 09:35:54 UTC
Please test. I'll wait a few days before backporting to 5-3, to make sure there are no visible regressions.
Comment 20 Maxim Monastirsky 2017-05-05 12:38:38 UTC
*** Bug 107629 has been marked as a duplicate of this bug. ***
Comment 21 Nadav Har'El 2017-05-05 13:24:25 UTC
Hi, unfortunately I cannot test the new version (yet), but I just wanted to ask - did you verify that this fix also fixes bug 107284, which was marked as a duplicate of this bug but is actually quite different issue?

In bug 107284, just pressing the "control" key, without the shift key (with the shift key having been previously pressed and released as part of a completely different action, such as selecting text), activated the paragraph-direction-switch operation. That bug report includes simple instructions how to reproduce that bug. Is that bug gone too?

Thanks for working on this!
Comment 22 Maxim Monastirsky 2017-05-05 14:03:16 UTC
(In reply to Nadav Har'El from comment #21)
> In bug 107284, just pressing the "control" key, without the shift key (with
> the shift key having been previously pressed and released as part of a
> completely different action, such as selecting text)
Bug 107284 actually caused by releasing the shift key *after* pressing the control key, so it's the same issue as here. (Just try to take your hand away from the keyboard after releasing shift, and only then press control, and you'll see what I mean.)
Comment 23 Yousuf Philips (jay) (retired) 2017-05-05 21:33:40 UTC
(In reply to Maxim Monastirsky from comment #19)
> Please test. I'll wait a few days before backporting to 5-3, to make sure
> there are no visible regressions.

Still switching to LTR when pressing shift+ctrl on my git build. :(
Comment 24 Maxim Monastirsky 2017-05-06 18:46:25 UTC
(In reply to Yousuf Philips (jay) from comment #23)
> Still switching to LTR when pressing shift+ctrl on my git build. :(
Does it happen on key press or key release? I thought we agreed in comment 4 to keep that behavior, but do it on key release, so it won't conflict with other control+shift+something shortcuts.
Comment 25 Yousuf Philips (jay) (retired) 2017-05-07 02:40:17 UTC
(In reply to Maxim Monastirsky from comment #24)
> Does it happen on key press or key release?

It now happens on key release, so i was mistaken and it does fix the reason i filed the bug of not being able to select with ctrl+shift+arrow and it switching the direction.

> I thought we agreed in comment 4
> to keep that behavior, but do it on key release, so it won't conflict with
> other control+shift+something shortcuts.

Ctrl+Shift switching directions is a windows OS-level shortcut that doesnt work in other apps if you dont have an RTL language installed. So this shortcut shouldnt work in LO unless there is someway to check with windows that its enabled elsewhere. In LO, the shortcut only works when CTL is enabled and now that CTL is enabled by default, a number of users are going to experience a weird behaviour that they cant explain and think is a bug. On a separate note, the shortcut doesnt work on linux in any other app, so not sure if it should work on linux at all.

@Khaled: Whats your thoughts on this?
Comment 26 Maxim Monastirsky 2017-05-07 12:17:30 UTC
(In reply to Yousuf Philips (jay) from comment #25)
> Ctrl+Shift switching directions is a windows OS-level shortcut that doesnt
> work in other apps if you dont have an RTL language installed. So this
> shortcut shouldnt work in LO unless there is someway to check with windows
> that its enabled elsewhere.
We can check whether there is a RTL keyboard installed. That what seems to make the difference for several apps I tested (Notepad, Word, IE, Firefox, Chrome), and we actually did that in the past to determine whether to enable CTL. (Although it's a bit weird to have working RTL/LTR buttons on the toolbar by default, but not some of their shortcuts.)

> On a separate note, the shortcut doesnt work on linux in any other app, so
> not sure if it should work on linux at all.
Indeed. Both Firefox and Chrome which have this shortcut working under Windows, don't use it under Linux. Same for QtWidget based apps. But on the other hand, given that this shortcut worked in OOo/LO for many years, some people might use it. There is Bug 96739 which was originally reported about Linux, and judging by some comments in [1] it seems that some people want such behavior under Linux too. So this likely will need more discussion.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=98160
Comment 27 Yousuf Philips (jay) (retired) 2017-05-11 19:32:15 UTC
(In reply to Maxim Monastirsky from comment #26)
> We can check whether there is a RTL keyboard installed. That what seems to
> make the difference for several apps I tested (Notepad, Word, IE, Firefox,
> Chrome), and we actually did that in the past to determine whether to enable
> CTL. (Although it's a bit weird to have working RTL/LTR buttons on the
> toolbar by default, but not some of their shortcuts.)

When in the past did CTL auto enable if it was applicable, as it never happened with me since the 3.5 days and i always had to go into the options dialog to enable it. Khaled enabled CTL by default recently.

> But on the
> other hand, given that this shortcut worked in OOo/LO for many years, some
> people might use it.

Definitely some people might be using these shortcuts, but ideally we would want them to use the shortcuts that are assigned to too these commands.

> There is Bug 96739 which was originally reported about
> Linux, and judging by some comments in [1] it seems that some people want
> such behavior under Linux too. So this likely will need more discussion.

Other than the bug reporter, i didnt see any comment that stated so, but i'm fine with it being also on Linux as long as the checking of an RTL keyboard can also be done. So i guess this functionality is also happening on Mac? @Steve, @Alex: Can you guys check?
Comment 28 Maxim Monastirsky 2017-05-11 20:26:42 UTC
(In reply to Yousuf Philips (jay) from comment #27)
> When in the past did CTL auto enable if it was applicable, as it never
> happened with me since the 3.5 days and i always had to go into the options
> dialog to enable it.
Under Windows it always worked for me since OOo days, and I confirm it still works with 5.3. Under Linux it never worked, simply because the keyboard detection was never implemented there.

> Definitely some people might be using these shortcuts, but ideally we would
> want them to use the shortcuts that are assigned to too these commands.
Well, Control+Shift is also "assigned", although hardcoded, as we have other hardcoded shortcuts like F6. But if we really consider this as a problem given that CTL is now on, maybe we can just turn it off by default under Linux, and provide a config option to turn it on for whoever needs it?

> Other than the bug reporter, i didnt see any comment that stated so,
I meant the Mozilla bug I linked where some people asked this for Firefox under Linux, and one mentioned that OOo provides it.

> So i guess this functionality is also happening on Mac?
Unlikely, as a search for SalEvent::KeyModChange doesn't give any results in the Mac sources folder.
Comment 29 Commit Notification 2017-05-25 19:43:33 UTC
Maxim Monastirsky committed a patch related to this issue.
It has been pushed to "libreoffice-5-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=30c17a288b03656633b626f6e7f679ca7c1aa6ff&h=libreoffice-5-3

tdf#103158 ctrl+shift should work on key up

It will be available in 5.3.4.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.