ome / omero-figure

An OMERO.web app for creating Figures from images in OMERO
http://figure.openmicroscopy.org
GNU Affero General Public License v3.0
15 stars 31 forks source link

Patch label text selection and cue for dragging #599

Open Tom-TBT opened 1 month ago

Tom-TBT commented 1 month ago

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: https://github.com/SortableJS/Sortable/issues/972#issuecomment-280253000

pwalczysko commented 1 month ago

@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 commented 1 month ago

I have added the include label

pwalczysko commented 1 month ago

Tested in FF and Chrome

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

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

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

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

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 ?

Tom-TBT commented 1 month ago

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 commented 1 month ago

Recommended bootstrap icon for indicating that it can be grabbed: https://icons.getbootstrap.com/icons/grip-vertical/ image

https://icons.getbootstrap.com/icons/grip-horizontal/ image

I also saw those, but their meaning is not "can be grabbed":

pwalczysko commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

will-moore commented 1 month ago

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 commented 1 month ago

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 commented 4 weeks ago

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 commented 3 weeks ago

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

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 commented 3 weeks ago

I tested it on Windows with:

Can be merged from my side.

Tom-TBT commented 3 days ago

There is a CSS conflict with https://github.com/ome/omero-figure/pull/591, because both are using bi-arrow-down-up

image

Only this PR affects the CSS property of bi-arrow-down-up so it should be sufficient for me to add here more precise CSS rules.