Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Consider add container option, that solves some bootstrap issues #139

Closed
marcospassos opened this issue Feb 13, 2013 · 39 comments · Fixed by #254
Closed

Consider add container option, that solves some bootstrap issues #139

marcospassos opened this issue Feb 13, 2013 · 39 comments · Fixed by #254

Comments

@marcospassos
Copy link

Hello,

The container option was introduced to solve some issues of tootip and popover (like style break in btn-group, popover hidden on modal, etc). So, IMO, it also could be implemented here.

Reference:
twbs/bootstrap#5768

I would be happy in submit a PR if you agree.

Thanks!

@pkozlowski-opensource
Copy link
Member

Yes, this was discussed already in #132

@marcospassos a PR would be awesome. We still got massive code duplication between the tooltip and popover so be warned! We are up to cleaning this (extracting common parts), but ok, not yet there...

@joshdmiller
Copy link
Contributor

These are my initial thoughts, subject to revision:

I don't feel it solves any problems, but it would provide a workaround to a couple of issues, which is why it was added upstream.

I think this exposes one of the (few, but sticky) challenges in porting jQuery plugins line-for-line to angular: I'm not sure how I feel about passing DOM references in as attributes to directives.

Plus, I'm not entirely sure how it would work. ui/bootstrap does not depend on jQuery, so the selectors we could even pass are very limited. So the other option would be passing in a jqLite object, and I can't see how that could work without referencing a DOM object in a controller, which is bad juju.

So my instinctual reaction is something along the lines of "egad!". Maybe it's just me. :-)

@marcospassos Did you have an idea of how this might work?

@pkozlowski-opensource
Copy link
Member

@joshdmiller Josh, actually you are completely right, my brain is half-asleep today. Switching off my laptop, shouldn't do coding anymore today :-)

@marcospassos
Copy link
Author

@joshdmiller I agree with you. But the issues solved with this workaround are very relevant (e.g. if you use the popover inside a modal probably it will be cut due the overflow-y, if you use a tooltip it will move the scrollbar always you hover you cursor over, etc). So, an Angular-way workaround should be provided. I think it should be limited just by id and name selector, mainly due to performance issues. Once jQuery selectors are not supported, IMO, their use should be discouraged to avoiding misunderstanding of new users. So, instead of containter="body" or container="#identifier", we can use container-name="body" or container-id="#identifier", estabilishing a precedence rule (e.g. container-id takes precedence over container-name)

It sounds good for you?

@joshdmiller
Copy link
Contributor

@marcospassos I didn't mean to imply that the issues shouldn't be resolved. I agree they should, but they should be solved in an "angular way". Unfortunately, this kind of thing doesn't have a great solution yet. It's not super common, but it's vexing when we do encounter it.

My personal recommendation is always that new AngularJS coders avoid jQuery like poison. From my experience, it just causes more problems than it solves because their philosophies are just too darned different - so different in fact to be, in some cases, bordering on mutually exclusive. But the philosophy of ui/bootstrap is to have "pure AngularJS" widgets based on Twitter Bootstrap. So while the jQuery selectors would work if the user chose to include jQuery (angular.element would just pass through), they would not out of the box, so we can't rely on them in a bug fix.

But angular.element will only take a DOM node or a string representation of code. So in order to implement your container-id="#id" idea, we'd have to hack through the DOM manually with document.getElementById; that's not in and of itself totally awful, but it's a trifle frail. For container-name="body", I'm not sure how that would be implemented at all, save manually traversing the DOM, which is no good - unless I'm missing something. In these cases, we'd basically be emulating jQuery in a directive!

I'd love to solve this, but I'm not quite sure how to do it yet.

@marcospassos
Copy link
Author

Josh, yes, my idea was use angular.find for the container-name and getElementById for the container-id. I avowed that isn't the most elegant solution, but was the best I've thought for now.

IMO, a good Angular-way high-level solution would be something like the scope declaration(@, =, &, etc) but for passing DOM element in attributes.

@joshdmiller
Copy link
Contributor

jqLite.find() is from a starting element. Are you suggesting it starts with $window? What about apps that don't own the whole document? And find takes only the node name, so it would work with 'body' but would never work with, say, 'div' or 'header'. If you're comfortable with a workaround in your use case, that's cool - each developer can obviously asses and weigh it appropriately - but I want to make sure we really think through how this should work before we include it as a default behavior for everyone.

As for scope-like declaration, I'm not sure I follow. Regardless of the declaration syntax, wouldn't we have the same challenges of locating nodes we're struggling with currently?

To take a step back - for what it's worth, I'm not sure we're even asking the right question. We're asking "how do we pass a DOM element to a directive?". I think we should be asking "do we need to pass a DOM element to a directive?".

Right now, the tooltip inserts the tooltip node (in a very jQuery-like fashion) directly after the element on which it is applied when a mouseenter is hit. Is this the right approach? Should we be inserting a DOM node at all? Perhaps there is another approach that would avoid these challenges.

@marcospassos
Copy link
Author

Yes, you're right, but with this issue we cannot use the tooltip and popover in many cases (modals, button groups, div with overflow: auto, etc). The perfect solution can take a long time, so IMO, I think that a intermediary solution may be used for now until a better idea.

@pkozlowski-opensource
Copy link
Member

While @joshdmiller points are very valid I agree with @marcospassos that we should try to find a pragmatic solution here. At the end of the day we need to stick this DOM element somewhere... I don't know, maybe we should have an option like tooltip-container="true" and is specified stic this tooltip next to http://docs.angularjs.org/api/ng.$rootElement?

Unless there is some other, elegant solution here...

@joshdmiller
Copy link
Contributor

My problem is that this doesn't actually solve the problem - it merely relocates the problem someplace else. The issue is with the last-child psuedo-class; so if the tooltip is moved out of the immediate element, then it will no longer mess with the last-child pseudo-class...except if $rootElement also uses last-child. Then what?

I'm all for a pragmatic solution, but it should actually solve the issue rather than just making it somewhere else's problem.

If we can figure out a way to identify a DOM node to use as a container (that doesn't make us all cringe) then I feel that is the better placement-based solution. On the to-do list is a configuration service for the tooltip and popover to provide default values. Perhaps we could place a container property there that takes a generic selector string. When the configuration service is first requested, that element could be (inefficiently) resolved to a DOM node that is returned to the directive. The benefit of this approach is that we only have to do nasty DOM stuff once; also, we have abstracted all of that out of the view, controller, and directive, so it's a trifle less gross. The usual downsides dealing with ID uniquness, etc., would all still apply, but it's a pragmatic solution.

Another potential solution is to make the tooltip a child element of the directive. This would be much less likely to encounter issues with pseudo-classes, but may have other downsides - I haven't put enough thought into it to formulate a strong opinion.

Anyway, my point is that there are multiple potential solutions and this issue is only a few days old. While Twitter pushed a fix last week, the issue was open for more than a year. I'm not sure why we feel the need to rush a solution into the code without due consideration of the ramifications of said solution.

My two cents...for what it's worth. :-)

@marcospassos
Copy link
Author

@joshdmiller as I've pointed out, the last-child is not the only issue. Try putting a tooltip inside a modal with scrollbars, hover the cursor over and see what happens (or just put a popover that overflows the modal's body). These issues require to set a correct container to be solved.

@joshdmiller
Copy link
Contributor

@marcospassos - @pkozlowski-opensource had mentioned allowing either adjacency or $rootElement. I pointed out that $rootElement wouldn't solve the pseudo-class issue. Regardless of any other issue, that still stands. Can you post a Plunker with the other issues, please?

But thank your for responding to 5% of my comment. What about the rest? Like the part about how to work in a custom container element... :-)

Edit: In retrospect, that last part may have sounded rude, but I intended it to be taken humorously. Just in case, I wanted to clarify.

@marcospassos
Copy link
Author

@joshdmiller, sorry I was very busy in the last days. As I've said, I really don't know how it can be made. I was thinking about in the last days, I don't have a good solution yet.

I was posting the plunk when I noticed this simple example is not working:
http://plnkr.co/edit/ZgtuJP?p=preview

Is this a known bug?

@joshdmiller
Copy link
Contributor

@marcospassos No worries - I've been pretty busy too. Looking at that Plunker, the issue isn't really the tooltip placement in this case, but the constant re-evaluation of the expression-based array in the ngRepeat. Every time we mouseenter, the tooltip is triggering a $digest, forcing a re-evaluation... But when you place the array on the scope instead, it works just fine: http://plnkr.co/edit/h0a9cD?p=preview.

Can you post a modal example?

@marcospassos
Copy link
Author

Thanks, Josh. Here is:
http://plnkr.co/edit/tl0SbK?p=preview

To reproduce de bug:
1 - First, scroll down until the end of modal-body
2 - Hover the cursor over the button with tooltip

Expected result:
The modal-body should be cut off, causing a visual discomfort.

@saurabhnanda
Copy link

test comment to follow this thread

@saurabhnanda
Copy link

Here's how I've hacked my way around this. Might not be the best solution, but works well for tooltips that don't need to react to changing scope:

<div qtip="{'event' : 'click'}" ng-transclude>
    <button type="button" qtip-target>Click me to view the qTip</button>
    <div qtip-content>
        Some content comes here
    </div>
</div>


myModule.directive('qtip', function() {
    return {
        'restrict' : 'A',
        'transclude' : true,
        'link' : function(scope, element, attrs, controller) {
            element.find("[qtip-target]").click(function() { return false; });
            element.find("[qtip-target]").qtip(angular.extend({
                'content' : {
                    'text' : element.find('[qtip-content]')
                },
                'hide'  : {
                    'event' : 'unfocus'
                },
                'show' : {
                    'event' : 'click'
                },
            }, scope.$eval(attrs.qtip)));
        }
    };
});

@marcospassos
Copy link
Author

ping

@Nness
Copy link

Nness commented Mar 4, 2013

Had similar issue today, and here is something I come up with.

attr.$observe('tooltipStair', function (val) {
    //integer. Currently only accept val >= 0
    scope.tt_stair = val;
});

function getPlaceholder() {
   var stair = parseInt(scope.tt_stair), i, locate = element;
   if (stair !== NaN && stair >= 0)
   {
      for (i = 0; i < stair; i++)
      {
         locate = locate.parent();
      }
    }
    return locate;
}

//Inside function show()
 getPlaceholder().after(tooltip);   //instead of "element.after( tooltip );"

We could expand stair to accept positive and negative, so when it is negative, we can insert it into children instead of append at parent.

I use it to append at parents as I want tool tip on input tag, while input tag is inside iniput-prepend or input-append.

@lanterndev
Copy link
Contributor

For me, a pragmatic solution to this problem would just be a boolean option for whether to append to the body or not. I started down the more general path but only ever used it with container = 'body'. Would that solve things (and in a much simpler way) for others, too?

@marcospassos
Copy link
Author

I think just allowing placing into body solves in most of cases.

@Nness
Copy link

Nness commented Mar 8, 2013

For individual body is enough. But for project like ui bootstrap, then it won't be that simple. Once that change has been made, you are happy, I'm OK with that, but what if someone else complain that body is not display properly because we stick tooltip at back.

If all put into body is exactly same as all append after element. Like what josh said, it just move problem elsewhere.

That's why Twitter Bootstrap uses container to let developer to specify where they want to put tooltip. For angular, it's much easier to navigate around parent or child than use selector. So my code above just let them select whether they want to append under parent or grandparent or grandchild. That they have more control instead all put at one place then later on when people complain and move else where again.

@joshdmiller
Copy link
Contributor

Alright. We've heard a lot of helpful debate. I'm not sure what the most optimal approach is - in fact, I'm having trouble finding any "optimal" approach. So I think we'll have to pick one, throw it on its own feature branch, and see how the community reacts.

Here's where we stand with the various options:

  • optional body append: I don't feel this solves the problems. As I and others have noted, it just makes the problems a little more rare. But if that's the best we can do, then I don't object.
  • qtip-target: a requirement of AngularUI Bootstrap is to have no external dependencies. This needs jQuery, so we can't use it. :-(
  • stair-stepping: this solution is a bit limiting, though it would provide a workaround for many use cases (though not all). I also worry about performance.
  • container config: this is a totally ugly hack that feels wrong, but it's simple both to implement and to use, it's relatively performant, and most importantly it does solve the problems.

Options explored, I lean toward the container config - it's a trifle ugly, but it does actually solve the problem rather than just make it more rare and it's a little more graceful and performant than traversing the DOM. I'd like to hear what the rest of the community thinks about this option.

Now for the bad news. Unfortunately, there is a major refactor of the tooltip and the popover to reduce code duplication currently underway and it doesn't make sense to add more duplicate code until this is done. I hope to get the refactor done this weekend, which means it will probably land in master sometime late next week or maybe early the following week. But once that happens, we can try to solve this in the form of a new pull request - and I'll be happy to spearhead that if no one else has the time.

@Nness
Copy link

Nness commented Mar 8, 2013

True that stepping cannot solve some tricky one and also brings performance issue on to table.

Container config sounds fine, it should be good to solve this issue until one day we have other options

@lanterndev
Copy link
Contributor

That all sounds great to me @joshdmiller. Thanks for doing all this great work.

@lanterndev
Copy link
Contributor

Mean to say, ideally it'd still be easy to specify container globally, i.e. tooltip-container="body" as the default for all tooltips. Is that what you have in mind too?

@joshdmiller
Copy link
Contributor

Okay the refactor is done (e22f28b), so we can get to work on this.

I'm going to take a stab at a container option in the $tooltipProvider to get the conversation moving again. Any last thoughts?

@pkozlowski-opensource
Copy link
Member

@joshdmiller so what is the final direction you plan to take? Specify a (limited) selector? Or just attach it to the body?
What about the #218 ?

@joshdmiller
Copy link
Contributor

@pkozlowski-opensource I think #218 is not the right approach. I don't feel this solves the problems. As I and others have noted, it just makes the problems a little more rare. Not only that, but it's not feature-compatible with the jQuery version.

I think the best approach is a limited selector (ID or tag name) specified to the provider and found manually upon directive creation. E.g.:

$tooltipProvider.options({
  container: 'body'
});

Or:

$tooltipProvider.options({
  container: '#tooltipContainer'
});

So something like this:

// ... in $tooltip service ...

if ( options.container.substring(0,1) === '#' ) {
  this.container = $document[0].getElementById(options.container.substring(1));
} else {
  this.container = $rootElement.find(options.container);
}

// ... in the directive ...

container = container || element;

What do you think?

@pkozlowski-opensource
Copy link
Member

@joshdmiller we should define "directive creation" but if this is too early me might get to problems that an element specified in a selector doesn't exist at this point, no?

I must say that personally I'm not in love with the '$document.body' solution but I'm not so crazy-happy about doing selection "by hand" either... And the body trick should work for most cases... Ah, those trade-offs...

@joshdmiller
Copy link
Contributor

@pkozlowski-opensource Yeah, there are no good solutions. The problem initially discovered was a :last-child issue, used throughout Bootstrap. It's not uncommon to use that same psuedo-class on section tags in the body. So while it would work around many (probably most) cases, it doesn't really "fix it".

I meant "directive creation" as in the body of $tooltipProvider.$get, which shouldn't be called until the first tooltip/popover injects it, which shouldn't be until the first time the directive is used. I'm thinking the element has to exist at this point, no? Also important to note, the selection (while nasty) is only occurring once.

But I'm open to all manner of ideas. So bring 'em on, geniuses! :-)

@pkozlowski-opensource
Copy link
Member

@joshdmiller I hate those kind of issues when I don't see good, clean solution. It professional in me wants to scream... At the same time was working in this industry long enough to see my fair share of crazy things and I know that we need some kind of decision here... I guess that we are at the point where any decision will be better than no decision at all :-)

Personally, if faced with this choice I would probably go with the body solution for the simple reason that it adds less code and should support many of the real-life scenarios. Your example with sections is perfectly valid and I know this is not perfect solution. But at this point I would privilege less code so it is easier to change in the future :-)

But this is just my personal view and I'm sure that if we leave it in your hands you will do right choices. So feel free to ignore the above and do the right thing :-)

@pkozlowski-opensource
Copy link
Member

Since the #254 landed in master already this issue should be fixed now.

@lastnico
Copy link
Contributor

@joshdmiller @pkozlowski-opensource @marcospassos: If you are aware of this dialog and tooltip z-index issue, why not implementing the container option instead of the global configuration appendToBody from #254? After all, this container identifier will be described in the View, if it crashes because this container does not exist, it is up to the developer to fix its problem, not the framework's.

... and yes, only supporting DOM element id is far away enough, meaning jqLite will be suitable for doing this job.

Why always trying to hide complexity if this complexity is actually the real world?

@oravecz
Copy link

oravecz commented Jul 9, 2013

The current (0.4.0) version is adding the tooltip to the body correctly, and sets a variable to determine whether tooltip is appended to element or body. Unfortunately, the same variable is not consulted later resulting in incorrect positioning when using tooltip-append-to-body.

var appendToBody = angular.isDefined( options.appendToBody ) ? options.appendToBody : false;
...
position = options.appendToBody ? $position.offset( element ) : $position.position( element );

should be

position = appendToBody ? $position.offset( element ) : $position.position( element );

@pkozlowski-opensource
Copy link
Member

@oravecz You are right, good catch! Those positioning issues are really not easy to track down as those are not unit-tested atm (and frankly we would have to attach doc fragments to the DOM tree and this would make tests run slow :-()

Anyway, would you be up to sending us pull request for this?

@Healforgreen
Copy link

Has this feature been implemented yet?

@capesean
Copy link

capesean commented Sep 8, 2015

@brianrt see: http://angular-ui.github.io/bootstrap/#/tooltip - you can configure the $tooltipProvider to append to the body globally.

@demircancelebi
Copy link

Use it like this

$uibTooltipProvider.options({ appendToBody: true });

z38 pushed a commit to oro-subtree/FilterBundle that referenced this issue Jul 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.