johndoh / roundcube-swipe

Adds swipe actions to the message list of Roundcube
https://plugins.roundcube.net/#/packages/johndoh/swipe
11 stars 5 forks source link

Swipe issues with display:flex on messagelist tr with rc release-1.5 branch HEAD and iOS Safari #21

Closed gutmensch closed 2 years ago

gutmensch commented 2 years ago

Hi! First of all thanks for the great plugin, I really appreciate and love it.

Now, I have digged a lot why I was seeing especially a bug with right swiping on iPhone with the latest rc 1.5 release branch changes - please see the pictures in [2]: the right swipe option is not aligned with the message and "wanders" around after I try it multiple times and also seems to lose the touch connection (maybe because it is misplaced). I could not reproduce this with Chrome responsive mode, and neither with Safari mobile mode, but it's repeatable for me on iPhone devices and the error for me is also disappearing if I'm exchanging styles.min.css from an early January 1.5.2 build with a new 1.5.2 (branch build), so I assume a change related to elastic skin styles caused this.

After a build without minification I am attaching in [1] the change that appears to have happened (potentially causing this), but I didn't fully bisect the malicious commit here. I don't have proper UI testing for crappy iOS Safari, but it would be great to get this fixed somehow as the swiping is really feeling broken on the apple devices now. I can do more testing or potentially get down to the real commit causing this if necessary. Let me know. Keep up the good work!

Cheers, Robert

versions: roundcube, release-1.5 branch, current HEAD swipe-plugin 0.4.0

[1]

diff -ad -uNrp /tmp/styles.min.css.old /tmp/styles.min.css.new
--- /tmp/styles.min.css.old 2022-02-20 01:17:17.000000000 +0100
+++ /tmp/styles.min.css.new 2022-02-20 01:19:22.000000000 +0100
@@ -3500,6 +3500,13 @@ table.fixedcopy {
 html.layout-phone .messagelist tr,
 html.touch .messagelist tr {
   position: relative;
+  display: flex;
+}
+html.layout-phone .messagelist td.selection,
+html.touch .messagelist td.selection,
+html.layout-phone .messagelist td.threads,
+html.touch .messagelist td.threads {
+  line-height: 3.8em;
 }
 html.layout-phone .messagelist td.flags,
 html.touch .messagelist td.flags {

[2]

johndoh commented 2 years ago

With the 1.5.2 release of Roundcube you see the problem but if you checkout the latest release-1.5 branch from git then everything is fine without any changes in the plugin, is that right?

Pretty much the exact patch you propose has already been applied in the core, see https://github.com/roundcube/roundcubemail/issues/8433 and the plugin should respect that.

gutmensch commented 2 years ago

No, exactly the opposite - I‘m building from release-1.5 all the time, and this shows the swipe issue since roughly 3+ weeks. :D So the change you linked might be the root cause - trying to fix attachment icon ( I was aware of this too), but creating an issue with swipe now. The „old“ styles from the diff is the working one, the „new“ is broken as described.

gutmensch commented 2 years ago

So I could just confirm that the display: flex; addition from the mentioned rc PR is indeed causing the issue described here. If I remove this CSS setting from current rc 1.5 HEAD, swipe 0.4.0 works correctly again.

johndoh commented 2 years ago

I think this patch would revert the display: flex change made in the core but may be it also reintroduces the attachment icon alignment bug?

diff --git a/skins/elastic/swipe.less b/skins/elastic/swipe.less
index 1855c5d..a27d05c 100644
--- a/skins/elastic/swipe.less
+++ b/skins/elastic/swipe.less
@@ -142,6 +142,10 @@
   }
 }

+.swipe-active:not(#swipe-action) {
+  display: table-row;
+}
+
 .swipe-active:not(#swipe-action),
 .swipe-active > td {
   background-color: @color-layout-list-background;

I do not have iOS device I can test on so I would appreciate your help in finding a solution if possible.

gutmensch commented 2 years ago

Thanks for looking into this, I'll check and report findings!

gutmensch commented 2 years ago

So the swiping seems to have the right offset again (like before malicious rc change), but it now duplicates the messagelist column after executing the "swiping right" (in both cases: swipe and release/do nothing and swipe and execute).

johndoh commented 2 years ago

Please reopen if problem persists in v0.5