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

Tooltip - unable to control using *open* props #2849

Closed
1 task
tomkdgun opened this issue May 28, 2019 · 44 comments · Fixed by #3357
Closed
1 task

Tooltip - unable to control using *open* props #2849

tomkdgun opened this issue May 28, 2019 · 44 comments · Fixed by #3357
Assignees
Labels
package: react carbon-components-react severity: 2 User cannot complete task, and/or no workaround status: waiting for author's response 💬 type: bug 🐛 version: 9 Issues pertaining to legacy Carbon version: 10 Issues pertaining to Carbon v10

Comments

@tomkdgun
Copy link

tomkdgun commented May 28, 2019

Tooltip - unable to control using open props

What package(s) are you using?

Same happens also for version 6.x:
Screenshot 2019-05-28 at 12 47 46

  • carbon-components
  • [ x] carbon-components-react

Detailed description

Unable to control visibility state of the Tooltip (open/close), using already existing open prop. It could be easy observed using provided codesandbox.

Screenshot 2019-05-28 at 12 50 37

What offering/product do you work on? Any pressing ship or release dates we
should be aware of?
We need this functionality in IBM SPSS Statistics. Tooltips are the last components we are trying to migrate from ap-components-react to Carbon.

Steps to reproduce the issue

  1. Assign Tooltip open prop to state variable (e.g. showTooltip).
  2. Change value of state variable (true->false, false->true)
  3. Result: Tooltip doesn't react on changes on open prop.

Expected: Tooltip will react on change for open prop

Please create a reduced test case in CodeSandbox
https://codesandbox.io/embed/codesandbox-lo5ox

Issue exists on both carbon-components-react versions 6.x and 7.x.
We are expecting a fix for version 6.x.

It's a blocking issue for us.

@dakahn dakahn added version: 9 Issues pertaining to legacy Carbon priority: medium severity: 1 Must be fixed ASAP labels May 28, 2019
@dakahn dakahn added this to Issue: needs triage 💊 in Carbon support via automation May 28, 2019
@tomkdgun
Copy link
Author

tomkdgun commented Jun 4, 2019

@dakahn Any update on this issue ?

@dakahn
Copy link
Contributor

dakahn commented Jun 4, 2019

None yet. Haven't had a chance to do any first aid on this issue just yet. @asudoh might have a workaround though

@asudoh
Copy link
Contributor

asudoh commented Jun 4, 2019

A workaround may be force-remount technique.

@dakahn dakahn added severity: 2 User cannot complete task, and/or no workaround and removed severity: 1 Must be fixed ASAP labels Jun 4, 2019
@tomkdgun
Copy link
Author

force-remount is not the good solution for us since tooltip wraps input fields.

@tomkdgun
Copy link
Author

@asudoh @dakahn Do you know when this issue will be fixed ?
We need to upgrade to Carbon 10 (IBM SPSS Statistics), but we are blocked by this issue, even to upgrade completely to Carbon 9.

@emyarod
Copy link
Member

emyarod commented Jul 11, 2019

it looks like this is caused by the handleClickOutside method not accounting for whether or not the component is controlled. a quick and dirty fix is to force the tooltip to accept an outside click listener (<ClickListener onClickOutside={ this.props.handleClickOutside || this.handleClickOutside }>) via props, but we'll need to warn the user about passing in a click listener if the component is controlled

@tomkdgun here is an example of the controlled tooltip functioning properly with the show/hide buttons (although it does not account for clicking the tooltip itself yet) https://deploy-preview-3357--carbon-components-react.netlify.com/?path=/story/tooltip--test, the code is currently a draft in #3357

@asudoh I think we may need to allow refs to be passed into the tooltip trigger node as well as the floating tooltip body to properly determine the source of click events

once we have confirmed the direction we want to take with this fix I can backport this to v9 as well

@emyarod emyarod added the version: 10 Issues pertaining to Carbon v10 label Jul 11, 2019
@tomkdgun
Copy link
Author

@emyarod I've looked into the example, but the issue is more general. In our code we are changing open prop programatically based inside custom validation methods, e.g. during typing in input text/number field. So user doesn't perform clicks, but it types values, we can even validate 3 input fields values during edit of one, and based on the result open tooltip with proper message.
Buttons in sandbox was just used to create simple example.
BTW, it seems there is no animation/transition during showing tooltip in draft example, not sure if this is intended.

@emyarod
Copy link
Member

emyarod commented Jul 12, 2019

@tomkdgun I may be misunderstanding but the open prop can be controlled no? I updated the example to include a text input which will open the tooltip if the input has an empty value and close the tooltip if the input is populated as a simple test

@tomkdgun
Copy link
Author

tomkdgun commented Jul 12, 2019

@emyarod When this issue was opened, open prop didn't work at all (controlled tooltip). We have found this on version 9.x. Later on we created simple example with buttons, which didn't work on both versions, 9.x and 10.x.

  1. Could you try if example you created works for 9.x version ?
  2. What if you remove this empty function handler handleClickOutside={() => {}} ?

@emyarod
Copy link
Member

emyarod commented Jul 12, 2019

@tomkdgun that external handleClickOutside is needed only when click events are involved, like in your example of toggling open with buttons. without it, the text input validation example should still function

I can backport the fix to v9 after we confirm the issue is resolved, just want to understand the issue first so I'm demoing in v10 with the netlify builds

edit: the behavior appears to be identical in v9, as the component itself is unchanged

@asudoh
Copy link
Contributor

asudoh commented Jul 12, 2019

The use case brought up (opening tooltip as user types something IIUC) makes me feel that definition tooltip or icon tooltip is more suitable for the use case. The usage scenario of the interactive tooltip is for letting user open the tooltip by user gesture.

@tomkdgun
Copy link
Author

@emyarod I've updated https://codesandbox.io/embed/codesandbox-lo5ox, by adding the example with controlled tooltip around input. This is simplified example from our app, since open also dependent on value validation.
Tooltip should be open when input is focused. Unfortunately tooltip still handles clicks, which bypass/changes state of tooltip, even open prop is true.

Current behaviour:

  1. When input is focused/blur using Tab key (no clicking) it works as expected, tooltip is open when input field is focused.
  2. When input is focused using click, it seems first tooltip is open because field is focused, and just after this click is handled, tooltip is closed. Additionally additional clicks on focused input field are changing tooltip state, is open or closed, same for clicking on spinners.

@tomkdgun
Copy link
Author

tomkdgun commented Jul 22, 2019

Tooltip content(text) is empty in case of valid input, tooltip should be opened only in case of invalid value (controlled). Because of this issue (still handling clicks on controlled tooltip), user can open controlled tooltip, which in case of empty text results with sth like this:
image
@emyarod @abbeyhrt @asudoh

@emyarod
Copy link
Member

emyarod commented Jul 22, 2019

@tomkdgun the codesandbox example appears to be unchanged from the original example. were your changes saved?

@tomkdgun
Copy link
Author

@asudoh
Copy link
Contributor

asudoh commented Aug 1, 2019

I thought controlling the open state by open prop is not working given it wasn't at some point, but I see it actually is working with latest 7.x as well as with latest 6.x. You can see it by adding a knob for open prop in the Storybook. Wrt the case of CodeSandbox link in the issue description, it appears that "close-on-click-outside" is happening as soon as the tooltip gets open by hitting the button - Which explains why it works with Storybook knob which is in a different frame, and it doesn't with the button in the demo. That said, a viable workaround is setting open state via setTimeout() - I can see it works by making such change to the CodeSandbox.

@tomkdgun
Copy link
Author

tomkdgun commented Aug 1, 2019

@asudoh What in case when click on trigger is performed (input wrapped by tooltip) ?
Click handler to open/close tooltip in this case should be skipped, when Tooltip is controlled (open prop provided).

@asudoh
Copy link
Contributor

asudoh commented Aug 1, 2019

Skipping user interaction controlled case will be an interesting idea, will take a note.

@tomkdgun
Copy link
Author

tomkdgun commented Aug 5, 2019

@asudoh @emyarod When fix will be ready and released ? It seems that some fix were prepared almost 2 weeks ago, which probably require some minor correction and tests on uncontrolled Tooltip, see #2849 (comment)

@asudoh
Copy link
Contributor

asudoh commented Aug 5, 2019

As I stated, you don't have to wait for our fix - setTimeout(stateUpdater, 0) will work for you.

@tomkdgun
Copy link
Author

tomkdgun commented Aug 6, 2019

@asudoh Its difficult to apply this for 40, 200 or 400 places

@tomkdgun
Copy link
Author

tomkdgun commented Aug 20, 2019

@asudoh @emyarod Could you remove "status: waiting for author's response" and assign someone to this issue ? Do fix will be available in next Carbon 10.5.x release ?

@emyarod
Copy link
Member

emyarod commented Aug 20, 2019

@tomkdgun are there any issues that are left unresolved in the latest netlify preview for #3357?

@tomkdgun
Copy link
Author

tomkdgun commented Aug 20, 2019

Latest netlify works well for my scenario (controlled one). Probably you need to just check if most common scenario (uncontrolled one) still works.
This issue is very important for IBM SPSS Statistics.

Edit: Actually I've noticed that on netlify, under Tooltip component test, there are default one examples, which it seems doesn't work, probably because of things I've noticed under this comment #2849 (comment)

@tomkdgun
Copy link
Author

tomkdgun commented Aug 20, 2019

@asudoh I've tried workaround you have suggested, with setTimeout.
Unfortunately it doesn't work as expected for wrapped input. e.g. I'm changing open with setTimeout to true, tooltip opens, but additional clicks inside tooltip trigger (input) are changing internal Tooltip state (closing/opening). My code around tooltip can't change internal state in such situation since parent component is not rendered/updated, open prop doesn't change e.g. always true, while internal Tooltip open state is changing on clicks.

@emyarod
Copy link
Member

emyarod commented Aug 20, 2019

there are default one examples, which it seems doesn't work, probably because of things I've noticed under this comment #2849 (comment)

I don't quite understand what you wrote in your edit, can you elaborate on the issue? there should be an example of an uncontrolled tooltip and another example of a controlled tooltip right?

and the open prop in the default Tooltip story should also be uncontrolled

@tomkdgun
Copy link
Author

@emyarod Yes, default Tooltip story doesn't specify open, Tooltip is uncontrolled, which is correct. In latest netlify, test works fine, but defaultstory, doesn't.

@emyarod
Copy link
Member

emyarod commented Aug 20, 2019

@tomkdgun default tooltip story appears to be working correctly for me (using the open storybook knob), and both tooltips in test story also appear to be working correctly. can you tell me more about the issue you are seeing?

@tomkdgun
Copy link
Author

@emyarod Can we talk on Slack ? https://w3.ibm.com/bluepages/profile.html?uid=P40065820
On https://deploy-preview-3357--carbon-components-react.netlify.com/?path=/story/tooltip--default-bottom I can't close tooltip (previously working by click)
On https://deploy-preview-3357--carbon-components-react.netlify.com/?path=/story/tooltip--no-icon and other with icon, I can't open tooltip (previously working by click)

@asudoh
Copy link
Contributor

asudoh commented Aug 21, 2019

To shed some light on this topic; The reduced case in this issue description (https://codesandbox.io/embed/codesandbox-lo5ox) can be changed below way so the demo works:

--- test.js.orig	2019-08-21 12:38:12.000000000 +0900
+++ test.js	2019-08-21 12:38:01.000000000 +0900
@@ -15,13 +15,13 @@
       <div>
         <Button
           style={{ padding: "15px 20px", margin: "4px 20px" }}
-          onClick={() => this.setState({ value: false })}
+          onClick={() => { setTimeout(() => { this.setState({ value: false }) }, 0); }}
         >
           Hide
         </Button>
         <Button
           style={{ padding: "15px 20px", margin: "4px 20px" }}
-          onClick={() => this.setState({ value: true })}
+          onClick={() => { setTimeout(() => { this.setState({ value: true }) }, 0); }}
         >
           Show
         </Button>

@tomkdgun
Copy link
Author

tomkdgun commented Aug 21, 2019

@asudoh On Buttons yes, but on input wrapped by Tooltip it doesn't, because clicks are perform on tooltip trigger, not outside, and there is not change in state and condition in parent component.
This is latest codesandbox https://codesandbox.io/embed/codesandbox-jilyl
Edit: Please try to click inside input, or on spinner buttons.

@asudoh
Copy link
Contributor

asudoh commented Aug 21, 2019

<Tooltip triggerText={<input>} /> (seen in https://codesandbox.io/embed/codesandbox-jilyl) looks a completely different one from here that should be tracked as a separate issue. It requires UX feedback to see if it's a valid used case.

@tomkdgun
Copy link
Author

@asudoh Jun 11: #2849 (comment)
Jul 22: #2849 (comment)

tomkdgun pushed a commit to tomkdgun/carbon that referenced this issue Aug 21, 2019
Carbon support automation moved this from Issue: backlog to Issue/PR: Closed 🙌 Aug 27, 2019
@emyarod emyarod self-assigned this Aug 28, 2019
@emyarod
Copy link
Member

emyarod commented Aug 28, 2019

reopening for v9

@emyarod emyarod reopened this Aug 28, 2019
Carbon support automation moved this from Issue/PR: Closed 🙌 to Issue: needs triage 💊 Aug 28, 2019
@emyarod emyarod closed this as completed Aug 29, 2019
Carbon support automation moved this from Issue: needs triage 💊 to Issue/PR: Closed 🙌 Aug 29, 2019
@emyarod
Copy link
Member

emyarod commented Aug 29, 2019

since this fix actually requires a breaking change, it isn't suited for backporting to previous versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: react carbon-components-react severity: 2 User cannot complete task, and/or no workaround status: waiting for author's response 💬 type: bug 🐛 version: 9 Issues pertaining to legacy Carbon version: 10 Issues pertaining to Carbon v10
Projects
None yet
6 participants