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

Implemented a Tooltip plugin to handle the JS-driven tooltip system across the UI #12262

Merged
merged 35 commits into from
Aug 22, 2022

Conversation

oleq
Copy link
Member

@oleq oleq commented Aug 11, 2022

Suggested merge commit message (convention)

Feature (ui): Implemented a TooltipManager class to manage UI tooltips across components and features. Closes #12067.

MINOR BREAKING CHANGE (ui): The TooltipView UI component has been removed, please use the new tooltip API instead. Please note that this change does not affect integrations that configure tooltips using ButtonView#tooltip property.

MINOR BREAKING CHANGE (ui): The static properties of BalloonPanelView have been renamed. BalloonPanelView.arrowVerticalOffset is heightOffset and BalloonPanelView.arrowHorizontalOffset is sideOffset.

Other issues that most likely got fixed (to be checked individually):

⚠️ Requires a PR in CF that aligns the code base to the new plugin and removal of TooltipView.


Status of things.

  • This is a WIP without tests (although quite prod-ready)
  • The prototype works on top of the balloon system (BalloonPanelView) and gets controlled by JS.
  • The prototype depends on certain data- set on DOM elements. If an element has these attributes (e.g. a button), the system will display a tooltip when it gets hovered/focused.
  • It supports mouse and keyboard interactions (see screencasts).
  • The implementation works so that moving over/leaving an element with the tooltip displayed will get rid of that tooltip no matter if shown by mouse or keyboard in the first place (see screencasts).
  • There's a single tooltip at a time in the UI now.
  • I introduced some delay before showing up the tooltip for better experience (avoid flashing when moving cursor across the toolbar).
  • I removed a show (opacity) transition because with a delay things got messy and the UX was bad (too many things happening).

2022-08-11 17 00 03

2022-08-11 17 00 48

2022-08-11 17 05 18

2022-08-11 17 06 41

Also:

  • There's a new manual test exclusively for tooltips.
  • BalloonPanelView got two new positions (west and east)

Concerns

  • The implementation is a plugin and the plugin needs to be loaded.
    • Quite easy to forget, I'd say. Even when I added this plugin to Essentials, we have tons of tests that do not load Essentials. Same with integrators, they could miss it.
    • OTOH forgetting this plugin is not critical for the users. These are just tooltips.
    • There is no easy work around. This is JS and must be run whenever the editor has UI. There are two options:
      1. Run it as a plugin (this PR).
      2. Have it somewhere around the base UI View class (so that when you use any View, you get tooltips support out of the box). This isn't going to be pretty, to say the least (the implementation uses BalloonPanelView that requires View on its own...) ☹️
      • Decision: Let's use a singleton loaded by EditorUI
  • The implementation is listening to DOM events on the document: What about iframes?
    • Decision: The editor does not support iframes and shadow roots. The problem concerns all balloons anyway.
  • Previous implementation offered a chance change the tooltip text dynamically when it is displayed. The new one will only update text when the tooltip gets hidden and shows up again. This is because the new implementation is based on data- attributes of HTML elements.
    • I'd don't recall any critical feature relying on this, though. It could've been a side effect of our UI framework.
    • Decision: This is not important ATM.
  • The implementation is affected by the technical debt of the balloon panel system, notably when the editor is located in a scrollable container
    • Decision: Hiding tooltips on scroll is a decent workaround.

2022-08-11 16 58 58

TODOs

  • Tests.
  • Docs.
  • (TBD) Make the alignment feature toolbar horizontal.
    • Decision: Let's postpone the decision.
  • (TBD) Remove tooltips for buttons with visual text labels.
    • Decision: A dedicated issue (e.g. "remove color").
  • (BC) Rename BalloonPanelView.arrowVerticalOffsetBalloonPanelView.arrowHeightOffset

@oleq oleq changed the title Implemented a Tooltip plugin to handle the JS-driven tooltip system across the UI. Implemented a Tooltip plugin to handle the JS-driven tooltip system across the UI Aug 11, 2022
@Reinmar
Copy link
Member

Reinmar commented Aug 16, 2022

  • The tooltip is positioned a bit differently now:

    • After:
      image
    • Before:
      image
    • Interestingly, there's a difference in placement of these tooltips between the Classic editor demo and the Chat with mentions example where they look like this. Some rounding issue?
      image
  • I wondered whether these tooltips@focus + the new focus handling will make the tooltips intrusive. My first feelings are: Definitely not. The thing that save us are:

    • The delay. But I'd even consider making it longer.
    • The fact they disappear when I do something so when I really use the editor with mouse they are either still invisible or disappear before I notice them.

@Reinmar
Copy link
Member

Reinmar commented Aug 16, 2022

Short answer: This works great. Let's go for it :)

@mlewand
Copy link
Contributor

mlewand commented Aug 16, 2022

Some issues that I noticed:

Content changes affecting toolbar position

  1. Open manual test with real time collaboration on two browser.
  2. Place focus in the table.
  3. Open column dropdown from balloon toolbar.
  4. Press esc to move the focus back to the balloon toolbar.
  5. On the second browser connected to the same session: modify the content so that the table moves below.

Actual: tooltip gets mispositioned.

Collaboration.tools._.Classic.-.Google.Chrome.2022-08-16.13-21-02.mp4

Find and replace

  1. Place focus in the find & replace form
  2. Move focus with tab key to the "replace with" field.
  3. Press tab to move focus to "show options"

Actual: Tooltip is offset to bottom.

The problem comes from the fact that "replace with" adds this tip that changes the height of the dropdown. If you move focus in the reverse order this does not happen.

Manual.Test.-.Google.Chrome.2022-08-16.13-16-27.mp4

Tooltips not hiding after clicking the button

I believe this is a small nuisance but somehow it sticks to my eye a lot during clicking 😅 Previously tooltip got hidden upon clicking a button. Now it stays there until you move your cursor.

Actual:

Collaboration.tools._.Classic.-.Google.Chrome.2022-08-16.13-28-23.mp4

Before:

Collaboration.tools._.Classic.-.Google.Chrome.2022-08-16.13-30-17.mp4

Having it the way it is proposed now makes me feel like after clicking I'm partially in previous stage of my interaction and partially in the new stage (interacting with the dropdown content).

oleq added 21 commits August 16, 2022 15:10
…ts position is always accurate after a delay because DOM could change in the meanwhile.
…n class to disable tooltips on an element. Removed remaining TooltipView CSS and mixins.
@oleq oleq marked this pull request as ready for review August 18, 2022 07:43
@Reinmar
Copy link
Member

Reinmar commented Aug 19, 2022

@oleq Could you better document the last change (in both places where it was needed): 6ea5bf4.

Also, please open a followup and cc @scofalik there. Maybe we missed something.

@oleq
Copy link
Member Author

oleq commented Aug 19, 2022

@oleq Could you better document the last change (in both places where it was needed): 6ea5bf4.

50c0801 #12292

@oleq
Copy link
Member Author

oleq commented Aug 19, 2022

@ckeditor/qa-team Could you please take a look at this PR? 

Scope:

  • Well... basically all tooltips, everywhere.
  • Tooltips on mouse hover.
  • Tooltips on keyboard focus.
  • Interaction when the tooltip was displayed using the keyboard then the mouse kicks in (and vice-versa)
  • Bugs discovered by Marek that should be addressed.
  • Tooltips in the presence list (a.k.a. avatars) in collaboration features (this PR requires another PR in CF to work).
    • There should be no degradation of the UX there.
  • There's a "tooltip" manual test in the ckeditor5-ui package that could help you test tooltips when the container with the editor starts to scroll.

@Mgsy
Copy link
Member

Mgsy commented Aug 22, 2022

@dufipl @Acrophost could you please take a look at this PR?

@mlewand mlewand requested a review from arkflpc August 22, 2022 11:44
@Acrophost
Copy link

Acrophost commented Aug 22, 2022

Tooltip going out of the screen when editor is sticking to edge of the screen still persist on one of our documentation examples after build
 

Screenshot 2022-08-22 at 13 48 12

 

Screenshot 2022-08-22 at 13 48 21

(edit: it has nothing to do with #10939, although this issue can't be fully closed due to grouped toolbar still cutting off)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment