hide tooltips and popovers when a user clicks away #4419

SystemDisc · 2015-09-15T18:50:26Z

hide triggers when a click occurs that does not target the popover/tooltip link and does not target elements with the popover/tooltip classes

This solves issue #1055 and helps with similar issues (#618, #590, #749, #751)

Let me know if this is something that's desired and if an option needs to be added for toggling this behavior.

wesleycho · 2015-09-15T20:52:38Z

This needs to be opt-in behind a feature - there are some cases where developers explicitly want to programmatically close it.

wesleycho · 2015-09-15T20:53:07Z

In addition, this needs unit tests as well.

SystemDisc · 2015-09-15T20:56:56Z

Is the feature to programmatically close Popovers and Tooltips already implemented? What are the relevant commits for that?

I can add an attribute for toggling this feature. Any preference as to what that attribute/directive should be called?

Also, what are you guys using for unit tests? I might not know how to do that, but I can look into adding them if you point me in the right direction.

SystemDisc · 2015-09-15T20:59:36Z

Oh, I'm sorry, you're talking about the isOpen property, aren't you?

wesleycho · 2015-09-15T21:06:40Z

Not just that - the programmatic triggers for hiding/showing

SystemDisc · 2015-09-15T21:08:25Z

Using $tooltipProvider.setTriggers( ... )?

SystemDisc · 2015-09-15T21:11:52Z

Should I just make this another trigger?

  var triggerMap = {
    'mouseenter': 'mouseleave',
    'click': 'click',
+   'clickaway': 'clickaway',
    'focus': 'blur',
    'none': ''
  };

wesleycho · 2015-09-15T21:16:20Z

That could work, and logic should be added for the binding and unbinding of event listeners depending on if it matches the string clickaway - technically it would be a breaking change, but I think it's one that should be minimal in impact.

SystemDisc · 2015-09-15T21:24:16Z

Something like that? b00a550

wesleycho · 2015-09-15T21:27:57Z

Yep, that works - just needs unit tests now :) .

Also for simplicity, you can do $document[0].body

SystemDisc · 2015-09-15T22:11:38Z

I just noticed that in unregisterTriggers, toggleTooltipBind is never unbound. Can I fix that in this same pull request?

wesleycho · 2015-09-15T22:12:16Z

Sure

SystemDisc · 2015-09-15T22:26:37Z

I made some updates, including unbinding toggleTooltipBind in unregisterTriggers, added a unit test, and updated the readme.

wesleycho · 2015-09-15T22:41:30Z

src/tooltip/tooltip.js

+            // hide tooltips/popovers when click away
+            function bodyHideTooltipBind(e) {
+              var target_elem = angular.element(e.target);
+              if (e.target != element[0] && !target_elem.hasClass('popover') && !target_elem.hasClass('popover-inner') && !target_elem.hasClass('popover-title') && !target_elem.hasClass('popover-content') && !target_elem.hasClass('tooltip') && !target_elem.hasClass('tooltip-inner')) {


Can you explain the specific classes you're targeting here with the check? This seems a bit fragile to me.

Or better, can you explain what is being accomplished by the check.

The check makes sure the user is not clicking on a popover or tooltip itself, allowing the user to select text in the popover or tooltip. Otherwise, if you clicked on anything that wasn't the popover or tooltip element/link, it would close the popover. These classes are part of bootstrap.

A better check out be to iterate over all of the elements in the stacked map and compare directly with each element - I am wary about tying it too heavily with a hardcoded class.

Yeah, I agree with that. I hadn't thought much about it. I'll make the change when I have some time again - hopefully tomorrow.

RobJacobs · 2015-09-16T13:37:02Z

I implemented this feature using a slightly different approach. Rather than rely on the body click exclusively, I also watch for a focus event which will handle tabbing out of the popover. The approach I use for adding the event handlers to the body are:

  $window.document.body.addEventListener('focus', eventHandler, true);
  $window.document.body.addEventListener('click', eventHandler, true);

With the useCapture enabled, I can simply use 'contains' to see if the triggering element is a child of the popover element like so:

  var eventHandler = function(evt) {
    if (element[0].contains(evt.target)) {
      // element is a child of the popover/tooltip
    }
  };

SystemDisc · 2015-09-16T14:42:35Z

Clicks bubble up to body, so we're not just handling when the body is clicked - we're handling when anything is clicked. Listening for click on the body and ignoring it when the click is on the link or the tooltip/popover is the best way I can think of, because it allows users to click the popover, and select text.

Also, is .contains part of jqlite?

SystemDisc · 2015-09-16T14:44:18Z

Also, this feature is specifically for "clicking away". I'm not sure it makes sense to hide the popover when a key is pressed, even tab to move focus.

wesleycho · 2015-09-16T15:05:51Z

contains is native DOM api

SystemDisc · 2015-09-16T16:33:25Z

Oh, ok. That's good to know. Let me think about this some more, and later today I'll make an update that I hope will satisfy you. Thanks for your continued feedback!

Hrafnkellos · 2015-10-08T14:47:41Z

Can we merge this into the library?

abelmokadem · 2015-10-19T09:06:04Z

Question, how will this work with multiple angular apps on 1 page? Will one app close all other popovers from the other app?

SystemDisc · 2015-10-21T11:49:34Z

No, it should not affect popovers in other apps on the page.

On Mon, Oct 19, 2015, 5:06 AM Ash Belmokadem notifications@github.com
wrote:

Question, how will this work with multiple angular apps on 1 page? Will
one app close all other popovers from the other app?

—
Reply to this email directly or view it on GitHub
#4419 (comment)
.

alanirudh · 2015-10-21T18:52:10Z

@SystemDisc Question: Does this also work for the above workflow, like when you open a popover will it close the already opened popover? Or will it require more logic to get this to work?

wesleycho · 2015-10-21T19:04:35Z

Should be fine - there is a contains check to check whether the event target is coming from a child of the popover in question.

SystemDisc · 2015-10-21T19:10:58Z

It would keep popovers open when you click on another popover. It closes
them when a click happens on something that isn't a popover. See the gif
above for an example of the current functionality.

It would be easy to change the functionality so that only one
"outsideClick" popover can be open at a time. Is this more desirable?

On Wed, Oct 21, 2015, 2:52 PM alanirudh notifications@github.com wrote:

[image: popoveraction]
https://cloud.githubusercontent.com/assets/6833271/10646668/f7c716cc-77f9-11e5-9def-5fdffed5f145.gif

@SystemDisc https://github.com/SystemDisc Question: Does this also work
for the above workflow, like when you open a popover will it close the
already opened popover? Or will it require more logic to get this to work?

—
Reply to this email directly or view it on GitHub
#4419 (comment)
.

alanirudh · 2015-10-21T19:41:34Z

@SystemDisc

Thanks for the quick response. The reason I have asked about it is because if we have multiple rows with popover menu opening on left or the right. Then previously opened popover will be covered by the newly opened popover. It's nice that it doesn't close the popover when we click with in the popover though.

We have something called autoclose in dropdown module which closes any opened dropdown when opening new dropdown. So, to be consistent with outsideClick action of dropdown we can have this behavior which closes when we click outside.

SystemDisc · 2015-10-22T03:39:59Z

Good point. I'll make the behavior of popover's outsideClick consistent
with the behavior of dropdown's outsideClick.

On Wed, Oct 21, 2015, 3:42 PM alanirudh notifications@github.com wrote:

Thanks for the quick response. The reason I have asked about it is because
if we have multiple rows with popover menu opening on left or the right.
Then previously opened popover will be covered by the newly opened popover.
It's nice that it doesn't close the popover when we click with in the
popover though.

We have something called autoclose in dropdown module which closes any
opened dropdown when opening new dropdown. So, to be consistent with
outsideClick action of dropdown we can have this behavior which closes when
we click outside.

—
Reply to this email directly or view it on GitHub
#4419 (comment)
.

SystemDisc · 2015-10-29T07:26:43Z

I'll try to get to it this weekend.

SystemDisc · 2015-11-01T00:26:27Z

tooltip/popover outsideClick should now behave more closely to dropdown outsideClick

It will behave the way @alanirudh's gif illustrates above.

SystemDisc · 2015-11-01T00:29:59Z

@wesleycho This PR no longer needs to be squashed, and I think it works the way everyone wanted it to.

RobJacobs · 2015-11-02T12:15:24Z

src/tooltip/tooltip.js

-              close: hide
+              close: hide,
+              triggerElement: element,
+              element: null


I don't think we need the 'triggerElement' and 'element' to be in the modal stack since you are no longer ignoring close events from other tooltip elements?

No, we don't, but I figured it might be useful in the future. I can remove it. Let me know if I should.

My philosophy is don't add it unless it's needed. It just adds unnecessary overhead to the openedTooltips collection.

@RobJacobs Pushing now

outsideClick will open a tooltip/popover on click and close that tooltip/popover when a click happens that is not targeting a tooltip/popover trigger element or a tooltip/popover itself

SystemDisc · 2015-11-10T03:57:23Z

@wesleycho I've addressed some concerns @RobJacobs brought up. Let me know if you guys need anything else before merging.

wesleycho · 2015-11-10T22:20:55Z

Can you clean up the eslint errors?

wesleycho · 2015-11-10T22:33:49Z

Actually, did the cleanup - thanks for sticking with this PR & putting in the good work @SystemDisc!

rand0m86 · 2015-11-17T11:08:22Z

@wesleycho, please, can you make a release for bower version that includes this update? It will be awesome.

icfantv · 2015-11-17T14:23:14Z

The fix (and repos) will be available when we release 1.0. We don't have a date for this release yet but want to get it out before the end of the year. We still have some things we need to finish.

In the meantime, please feel free to pull down the latest code and do an npm install followed by grunt to get the latest SNAPSHOT build. The files will be in the dist directory.

h6784rfg6 · 2015-11-29T14:10:04Z

Hello, I am also interested in this feature. I installed the npm version: npm install angular-ui-bootstrap and linked the ui-bootstrap-tpls.js (which was not in the disc folder). I could't get the popover open providing the popover-trigger="outsideClick" option. Could you please provide some plunker with the example usage of this feature?

wesleycho · 2015-11-29T15:12:19Z

@h6784rfg6 currently this is not released - if one wants to use this feature, one has to consume master directly. Otherwise, keep an eye out for #4838 - once that issue is complete, we will release 1.0 shortly after

wesleycho added component: tooltip needs: work component: popover labels Sep 15, 2015

wesleycho added this to the Backlog milestone Sep 15, 2015

wesleycho added the needs: PR test(s) label Sep 15, 2015

wesleycho added needs: review and removed needs: PR test(s) needs: work labels Sep 15, 2015

wesleycho reviewed Sep 15, 2015
View reviewed changes

wesleycho modified the milestones: 0.14.0 (Bootstrap 3.3), Backlog Sep 15, 2015

wesleycho modified the milestones: 0.14.3, 1.0.0 Oct 23, 2015

RobJacobs reviewed Nov 2, 2015
View reviewed changes

feat(tooltip): add outsideClick trigger to tooltips/popovers

b48bd7f

outsideClick will open a tooltip/popover on click and close that tooltip/popover when a click happens that is not targeting a tooltip/popover trigger element or a tooltip/popover itself

aaronleesmith mentioned this pull request Nov 10, 2015

hide tooltips and popovers when a user clicks away (corrected) #4871

Closed

wesleycho removed the needs: squash label Nov 10, 2015

wesleycho closed this in 8737303 Nov 10, 2015

riltsken mentioned this pull request Dec 7, 2015

Add helptext for checkPrecondition expression spinnaker/deck#1783

Closed

hide tooltips and popovers when a user clicks away #4419

hide tooltips and popovers when a user clicks away #4419

Conversation

SystemDisc commented Sep 15, 2015

wesleycho commented Sep 15, 2015

wesleycho commented Sep 15, 2015

SystemDisc commented Sep 15, 2015

SystemDisc commented Sep 15, 2015

wesleycho commented Sep 15, 2015

SystemDisc commented Sep 15, 2015

SystemDisc commented Sep 15, 2015

wesleycho commented Sep 15, 2015

SystemDisc commented Sep 15, 2015

wesleycho commented Sep 15, 2015

SystemDisc commented Sep 15, 2015

wesleycho commented Sep 15, 2015

SystemDisc commented Sep 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobJacobs commented Sep 16, 2015

SystemDisc commented Sep 16, 2015

SystemDisc commented Sep 16, 2015

wesleycho commented Sep 16, 2015

SystemDisc commented Sep 16, 2015

Hrafnkellos commented Oct 8, 2015

abelmokadem commented Oct 19, 2015

SystemDisc commented Oct 21, 2015

alanirudh commented Oct 21, 2015

wesleycho commented Oct 21, 2015

SystemDisc commented Oct 21, 2015

alanirudh commented Oct 21, 2015

SystemDisc commented Oct 22, 2015

SystemDisc commented Oct 29, 2015

SystemDisc commented Nov 1, 2015

SystemDisc commented Nov 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SystemDisc commented Nov 10, 2015

wesleycho commented Nov 10, 2015

wesleycho commented Nov 10, 2015

rand0m86 commented Nov 17, 2015

icfantv commented Nov 17, 2015

h6784rfg6 commented Nov 29, 2015

wesleycho commented Nov 29, 2015