Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch label text selection and cue for dragging #599

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Tom-TBT
Copy link
Contributor

@Tom-TBT Tom-TBT commented Oct 18, 2024

Hello,
that's a patch from an issue we saw with the drag and drop sorting of the label.
It became impossible to select text with the mouse cursor (triggers the drag of the label html element), and with Firefox the cursor always goes to the beginning of the input.

I found the description of the issue and the fix here:
SortableJS/Sortable#972 (comment)

@pwalczysko
Copy link
Member

@jburel @Tom-TBT : sorry, I can see just the bug behaviour. This is because this PR is not in the merge

Repository: ome/omero-figure
Excluded PRs:
  - PR 598 Tom-TBT 'Query timestamps from all planes when C or Z dimensions are not filled' (user: Tom-TBT)
  - PR 597 Rdornier 'Rounding zoom label in pdf' (user: Rdornier)
  - PR 596 Tom-TBT 'Dynamic lut for figure' (user: Tom-TBT)
  - PR 591 MinaEnayat 'Add Horizontal and Vertical Image Flipping Functionality' (user: MinaEnayat)
  - PR 543 will-moore 'Plate well labels' (exclude comment)
Already up to date.

Merged PRs:
  - PR 577 Rdornier 'Fill rois'
  - PR 578 Rdornier 'Image outline'
  - PR 584 Rdornier 'Link url to image'
  - PR 592 will-moore 'Filter owners and groups dropdown lists'
  - PR 595 Rdornier 'Rotate image panel by 90°'
  

@jburel jburel added the include label Oct 18, 2024
@jburel
Copy link
Member

jburel commented Oct 18, 2024

I have added the include label

@pwalczysko
Copy link
Member

Tested in FF and Chrome

FF, Mac, release version (without this PR):

  • indeed, difficult to use only mouse for the selection of the text inside labels
  • swapping of the labels is easy

Chrome, Mac, release version (without this PR):

  • easy to use only mouse for the selection of the text inside labels
  • swapping of the labels is easy

FF, Mac, with this PR (merge-ci):

  • easy to use only mouse for the selection of the text inside labels
  • swapping of labels is extremely difficult, you have to click onto a particular place (best is the corner) and then, on several tries, it works. You need both the knowledge that the feature exists and a lot of stamina to make swapping work

Chrome, Mac, with this PR (merge-ci)

  • easy to use only mouse for the selection of the text inside labels
  • swapping of labels is extremely difficult, you have to click onto a particular place (best is the corner) and then, on several tries, it works. You need both the knowledge that the feature exists and a lot of stamina to make swapping work

In summary, my claim would be that

  1. this PR intended to fix the mouse selection in FF -> success
  2. this PR did not intend to break the swapping of labels feature in FF -> fail (it does break the swapping)
  3. this PR did not intend to break the swapping of labels feature in Chrome -> fail (it does break the swapping)

In summary, I am sorry to recommend rejection of this PR.
The points 2., 3. above are fails&regressions of the released functionality.

Happy to discuss, imho the mistake happened when the swapping functionality was introduced -> this is not a crucial bit of functionality, and I agree that mouse selection of the text is more important. I understand this was not tested in FF prior to release and thus released with a regression.

Possibly the thinking on this PR is to "supress" or "degrade" the swap functionality (in all browsers) in favor of salvaging the FF experience for mouse text selection. If this is the case, then this PR has value imho, but almost certainly the authors/users of the swapping feature will be hit hard.

Bearing in mind the above, what is your take on it @Tom-TBT please ?

Copy link
Member

@pwalczysko pwalczysko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the comments from the testing ^^^. Thank you

@Tom-TBT
Copy link
Contributor Author

Tom-TBT commented Oct 24, 2024

Hey Petr,
thank you very much for the thorough testing.
To clarify, my student Tehmina and I are the authors of the sorting label functionality. The sorting label has introduced the issue with text selection, so IMO, this will feel like a regression.

But I think the label selection has a solution. The label can be grabbed from anywhere on the element. There is a whitespace after the color dropdown.

Please tell me if that whitespace exists also on Mac:
image

What misses then is a cue to indicate that the label is draggable from there. I think placing here the right icon will do the trick. What do you think?

@Tom-TBT
Copy link
Contributor Author

Tom-TBT commented Oct 24, 2024

@pwalczysko
Copy link
Member

The sorting label has introduced the issue with text selection, so IMO, this will feel like a regression.

Agreed.

Please tell me if that whitespace exists also on Mac:

Yes it does. Plenty of space . Both FF and Chrome.

What misses then is a cue to indicate that the label is draggable from there. I think placing here the right icon will do the trick. What do you think?

I would suggest the word swap instead of icon. From the icons you are suggesting in your comment, I like the most the arrow-down-up https://icons.getbootstrap.com/icons/arrow-down-up/. But yes, this type of solution would go a long way imho and would solve the problem in most cases.
I understand that the workflow would be to open another PR now with the swapping icon and then test both in all available permutations (I can do Mac FF, Mac Chrome, Mac Safari, I suppose you could do Win Edge at least ?).

Thanks. a lot

@Tom-TBT
Copy link
Contributor Author

Tom-TBT commented Oct 24, 2024

I am not in favor of having the text "swap". Here are test with grab icon and the arrow you suggested:
image

image

I would stay on this PR, because it needs the "text selection" fix regardless. But I can rename the PR to "Patch label text selection and cue for dragging"

(note, don't mind the final CSS tweeks on size of the icon and alignment. This can be taken care of once we made a choice)

@pwalczysko
Copy link
Member

I am not in favor of having the text "swap".

oh, well

Here are test with grab icon and the arrow you suggested:

I am in favor of the double arrow. I understand that it does not suggest "Grab and drag". But to be honest, those 10 dots in 2 rows (the second choice) suggests absolutely nothing to me.

What I can sometimes see is an icon with a hand (which suggests "grab"). Is there one available ? (fine if not or too hard)

I would stay on this PR, because it needs the "text selection" fix regardless. But I can rename the PR to "Patch label text selection and cue for dragging"

Yes and yes.

(note, don't mind the final CSS tweeks on size of the icon and alignment. This can be taken care of once we made a choice)

sure

@Tom-TBT Tom-TBT changed the title Patch label text selection issue from drag and drop Patch label text selection and cue for dragging Oct 24, 2024
@will-moore
Copy link
Member

I have a slight preference for arrows-vertical but arrow-up-down is OK too (better than dots or "swap").
And if you show the grab cursor https://developer.mozilla.org/en-US/docs/Web/CSS/cursor for it too, that would be great.
For css tweaks, maybe just make the arrow icon a little less prominent: (big smaller, maybe even a tiny bit less black??)

@Tom-TBT
Copy link
Contributor Author

Tom-TBT commented Oct 24, 2024

I changed the opacity and the grab cursor.
The cursor now only shows up on top of the icon, so using a larger icon is preferable. I have a preference for arrow-up-down instead of arrows-vertical. But you can say "Tom use this or that icon" and I will make the changes :D

I updated the bootstrap-icons to 1.11 in case, arrows-vertical is only here since 1.10.

@pwalczysko
Copy link
Member

I do think that this PR is included in today's build.

Repository: ome/omero-figure
Excluded PRs:
  - PR 597 Rdornier 'Rounding zoom label in pdf' (user: Rdornier)
  - PR 596 Tom-TBT 'Dynamic lut for figure' (user: Tom-TBT)
  - PR 543 will-moore 'Plate well labels' (exclude comment)
Already up to date.

Merged PRs:
  - PR 577 Rdornier 'Fill rois'
  - PR 578 Rdornier 'Image outline'
  - PR 591 MinaEnayat 'Add Horizontal and Vertical Image Flipping Functionality'
  - PR 592 will-moore 'Filter owners and groups dropdown lists'
  - PR 598 Tom-TBT 'Query timestamps from all planes when C or Z dimensions are not filled'
  - PR 599 Tom-TBT 'Patch label text selection and cue for dragging'

the new features (improved behaviour in FF on text selection using mouse) is present.

But any icon is missing
Screenshot 2024-10-25 at 13 10 44

@pwalczysko
Copy link
Member

Screenshot 2024-11-01 at 14 04 54
I have tested this in Mac:

  • Firefox
  • Chrome
  • Safari

lgtm. I would think we should also try Windows (definitely Edge) in order not to miss anything. Can you @Tom-TBT or your colleagues provide that Windows test please ?

@Tom-TBT
Copy link
Contributor Author

Tom-TBT commented Nov 1, 2024

I tested it on Windows with:

  • Firefox
  • Chrome
  • Edge
  • Opera

Can be merged from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants