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

Change in Modal behavior - Links embedded in Tooltips no longer work. #3021

Closed
2 tasks done
JoeAdamski opened this issue Jun 11, 2019 · 9 comments
Closed
2 tasks done

Comments

@JoeAdamski
Copy link

JoeAdamski commented Jun 11, 2019

What package(s) are you using?

  • carbon-components
  • carbon-components-react

Detailed description

Inside of Modals where we use tool-tips with links - the link fails to open when clicked.

This appears to be a regression starting in 6.114.0 - it worked in 6.113.1.

I suspect that it is related to this commit that introduces a focus-trap.
carbon-design-system/carbon-components-react@b9f5d9e#diff-b9cfc7f2cdf78a7f4b91a753d10865a2

Is this issue related to a specific component?

Modal

What did you expect to happen? What happened instead? What would you like to happen?

Expect link to open in new window. Would like to see this fixed in the V6.* branch as we have breaking changes preventing us from upgrading to V7+ at this time.

see changed?
Started in V6.114.0 - clicking link was ignored.

What browser are you working in?

Chrome.

What version of the Carbon Design System are you using?

"carbon-components": "^9.91.3",
"carbon-components-react": "6.114.0",

What offering/product do you work on? Any pressing ship or release dates we
should be aware of?

VPC UI

Steps to reproduce the issue

We have this Tooltip inside of a Modal:

    const renderTooltip = (
      <div className={styles.publicKeyTooltip}>
        <Tooltip
          showIcon={false}
          triggerText={publicKeyHelperText}
          dataToolTip={'publicKey'}
        >
          <p>
            <Link
              to="/docs/vpc-on-classic-vsi?topic=vpc-on-classic-vsi-ssh-keys#ssh-keys"
              target="_blank"
            >
              Link-text
            </Link>
          </p>
        </Tooltip>
      </div>
    );
@JoeAdamski JoeAdamski changed the title Links no longer work on modals. Regression: Links no longer work on modals. Jun 11, 2019
@JoeAdamski
Copy link
Author

@asudoh , @dakahn - mentioning to raise awareness.

@JoeAdamski JoeAdamski changed the title Regression: Links no longer work on modals. Regression: Links embedded in Tooltips no longer work on modals. Jun 11, 2019
@asudoh
Copy link
Contributor

asudoh commented Jun 11, 2019

@JoeAdamski Given you believe focus trap introduction is the culprit, would you want to turn it off? (focusTrap={false})

@JoeAdamski
Copy link
Author

Thanks Akira. I actually already did that and it's working now.

However, is this a good overall solution? What if others who want this functionality have tool tips with links? Should there be some other solution?

@asudoh
Copy link
Contributor

asudoh commented Jun 12, 2019

Actually <Modal> has had focus-wrap behavior, with or without focusTrap={true}. focusTrap={true} was introduced so it's a bit more perfect.

@JoeAdamski
Copy link
Author

@asudoh - I agree its nice to have this option, but should it be disabled by default?

  • This would preserve backwards compatibility and behavior
  • The current default behavior seems a little unexpected now. Meaning, if someone sees a link in a pop-up is not working - they might not realize that its the fault of focusTrap being enabled for them. Where if they needed a stronger focus control then ( focusTrap ) being added a as an attribute makes sense.

@JoeAdamski JoeAdamski changed the title Regression: Links embedded in Tooltips no longer work on modals. Change in Modal behavior - Links embedded in Tooltips no longer work. Jun 12, 2019
@asudoh
Copy link
Contributor

asudoh commented Jun 12, 2019

@JoeAdamski I tend to agree with you - Flipping the switch back causes another breaking change, though...

@JoeAdamski
Copy link
Author

JoeAdamski commented Jun 13, 2019

I think the long standing behavior is more important to not break - as there are unlikely many people taking advantage of the newer functionality yet. Don't you think?

Also, I always prefer boolean attributes to have default value false. Then we never have to say focusTrap={false}. We just leave it out for false and add it focusTrap to enable it.

Mine is working now though, by using focusTrap={false}.

I think my suggestion would make this a little better, but if you think its better the way it is, I'm ok with closing the issue.

And... I do think the focusTrap is a nice feature to add.... Thanks for that.

@asudoh
Copy link
Contributor

asudoh commented Jun 13, 2019

Yeah got your point. I'm reluctant with making another change, though given there is a workaround. Thank you @JoeAdamski for sharing your thoughts!

@asudoh asudoh closed this as completed Jun 13, 2019
@JoeAdamski
Copy link
Author

Ok, Akira. 👍

asudoh added a commit to asudoh/carbon-components that referenced this issue Feb 4, 2020
This change eliminates the need for application to put focus sentinel
by having `<Modal>`, `<ComposedModal>` and `<FloatingMenu>`
automatically put the focus sentinels.

This change also add support for reverse-focus-wrap feature to
`<Modal>` and `<ComposedModal>`, without needing using 3rd-party
`focus-trap-react` library. This helps applications hitting adverse
side-effects that `focus-trap-react` library causes (e.g. carbon-design-system#3021, carbon-design-system#3665
and carbon-design-system#4600).

Fixes carbon-design-system#3817.
Fixes carbon-design-system#4036.
Fixes carbon-design-system#4600.
asudoh added a commit that referenced this issue Feb 14, 2020
This change eliminates the need for application to put focus sentinel
by having `<Modal>`, `<ComposedModal>` and `<FloatingMenu>`
automatically put the focus sentinels.

This change also add support for reverse-focus-wrap feature to
`<Modal>` and `<ComposedModal>`, without needing using 3rd-party
`focus-trap-react` library. This helps applications hitting adverse
side-effects that `focus-trap-react` library causes (e.g. #3021, #3665
and #4600).

Fixes #3817.
Fixes #4036.
Fixes #4600.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants