IGreatlyDislikeJavascript / bootstrap-tourist

Quick and easy way to build your product tours with Bootstrap Popovers for Bootstrap 3 and 4. Based on Bootstrap-Tour, but with many fixes and features added.
63 stars 36 forks source link

[v0.3.0] tourHighlight div not correctly positioned #38

Closed 1Luc1 closed 4 years ago

1Luc1 commented 5 years ago

Hello,

Just updated from v0.2.0 to v0.3.0.

v0.2.0 worked without any problems. I know there where some significant changes within v0.3.0, but i don't know where i have to adapt my code to fit version v0.3.0.

Currently there is a problem with steps, which have set and 'element':'#element-id' and 'backdrop':true. The generated <div id=tourHighlight .. /> is out of place from the element.

highlight_div

What it looks like in v0.2.0 v0 2

Thanks for the information, Luc

IGreatlyDislikeJavascript commented 5 years ago

Thanks for the report. Does it happen in every tour step that has an element and a backdrop?

Are you able to reproduce this in a fiddle or codepen?

The upgrade from v0.2.x to v0.3.0 doesn't require any code changes, simply replace the css and js.

1Luc1 commented 5 years ago

Thanks for your reply. In this fiddle you could see the problem. https://jsfiddle.net/upg02zwe/

As i can see, it only happens to steps with element and backdrop. Could the use of Bootstrap v3.4.1 be the Problem?

IGreatlyDislikeJavascript commented 5 years ago

No, it's unlikely to be a bootstrap problem, it's probably a Tourist bug. I'll take a look at this tomorrow, and @ibastevan might have some suggestions too. Thanks for the report, bear with me while we figure it out.

IGreatlyDislikeJavascript commented 5 years ago

Now I'm confused! I've just tried your fiddle and it works perfectly for me with zero changes:

Capture

I just tested it using Chrome Version 77.0.3865.120 (Official Build) (64-bit) . What's your environment, because it has to be a browser issue...

(incidentally, once we get this fixed, this is a classic case where the new backdropOptions will help with UX - you'll be able to change the highlight so that the black backdrop isn't washed out)

1Luc1 commented 5 years ago

Thank you for your reply. Ah sorry, i probably was a little bit unclear about the problem.

Here is a fiddle with Version 0.2.1 https://jsfiddle.net/x0mda4z2/ As you can see, the blue icon is visible.

With version 0.3.0 https://jsfiddle.net/upg02zwe/ the icon is covered with a grey box, like you see on your screenshot.

The code within those two fiddle is the same, except the used tourist libraries. v0.2.1 -> v0.3.0

IGreatlyDislikeJavascript commented 5 years ago

Ah I think I see what you mean. I'm not in front of a machine for a bit, but please try this.

If you add backdropOptions: {backdropColor:#000} to the step with the black element background, see if that fixes it.

I can give you proper instructions tomorrow, but in the meantime have a look at the comments at the top if the .js for this option.

It works per step and globally, so if all your elements are on black backgrounds, just set the option globally instead of per step.

1Luc1 commented 5 years ago

Thanks for your reply. I tried the backdropOptions : backdropColor as well as highlightColor and highlightOpacity with various configurations, but couldn't get the desired result.

ibastevan commented 5 years ago

Thanks for your reply. I tried the backdropOptions : backdropColor as well as highlightColor and highlightOpacity with various configurations, but couldn't get the desired result.

Can you send a screenshot with the highlightcolor option being set to black?

When you say you couldn’t achieve your desired result, are you able to send a screenshot of your desired result? Or explain what your looking to achieve?

(If you use inspect element tool on your browser developer options (Ctrl+Shift+C) you can manipulate the Step code on screen)

ibastevan commented 5 years ago

I just re read your original post and see that the highlight needs to be a dark grey colour, perhaps try using something like #262626 as the highlight color

ibastevan commented 5 years ago

I am at my desk now, and initial checks seem to be that this could be to do with the z-index. The z-index of the tour-highlight-element class doesn't seem to be moving the element above the backdrop and highlight layer.

I am just having a play around with the fiddle, so hopefully find out what the issue is.

1Luc1 commented 5 years ago

I think it could be to do with the offset, you could try this: ‘’’ backdropOptions: { animation:{ ... } } ‘’’

I will check the fiddle when I’m at my computer.

no, that's not working.


But if it’s on a black ground it may best either disabling the backdrop for that step, or change the opacity on the highlight to 0

yea, disabling the backdrop will work, but I'd like to have it with backdrop; since it worked with v0.2.0.


... When you say you couldn’t achieve your desired result, are you able to send a screenshot of your desired result? Or explain what your looking to achieve? ...

within my first post, there is a screenshot from version 0.2.0. this is what i want.

ibastevan commented 5 years ago

The new backdrop works with z-indexing and the issue here is to do with the element being inside the navbar which has a fixed position and z-index. It is definitely a tourist bug as the navbar is a standard component of bootstrap. Thanks for spotting this and I will look into this issue. Hopefully there is a workaround.

IGreatlyDislikeJavascript commented 5 years ago

@ibastevan so it's a css issue? Think you'll be able to solve this better than I can! Let me know what you find and we can implement a fix into Tourist.

ibastevan commented 5 years ago

Yes the step element is inheriting the z-index from the navbar z-index (1030), the backdrop and highlight divs have z-index's of 1110 therefore they are appearing over the step element. The step elements z-index 1111 has no effect. Hopefully there is a workaround.

ibastevan commented 5 years ago

Currently there are only 2 workarounds i know of.

  1. The step element needs to be moved out of the navbar.
  2. The navbar needs to have it's position and z-index removed.

Obviously neither of these options are ideal solutions because there would be a reason why the navbar has had the class "navbar-fixed-top" added. And there will be a reason why the element has been added to within the navbar.

ibastevan commented 5 years ago

There could be another solution but this could major rework of the code.

If we moved the tour-highlight div to inside the navbar (right above the tour-highlight-element) and added in an additional tour-backdrop div under the tour-highlight div we could make it work without having to change anything else.

We would do this because the tour-highlight, tour-backdrop and tour-highlight-elements need to siblings of one another or not contained inside a parent with a position or z-index.

ibastevan commented 5 years ago

Here's a fiddle of the desired output: https://jsfiddle.net/bhx2urLz/ You can see in the html the new divs inside that make it all work. @IGreatlyDislikeJavascript do you have any ideas how we could incorporate this? So basically this would be an option for steps that are child elements of parents that have a position or z-index (such as the navbar with the class .nabvar-fixed-top).

  1. move highlight to next to highlight-element.
  2. add in an additional backdrop to next to highlight-element.
IGreatlyDislikeJavascript commented 5 years ago

I'll need to take a look at this and the fiddle, it's a bit hard to picture it without sitting in front of a proper screen!

My first thought is that if we can't solve this using the container option, we'll be able to solve it with the divs. It's easy enough to reposition/restructure them, but I need to get clear in my own mind what has to happen. Let me take a look later on today.

I'm assuming a !important won't solve this?

ibastevan commented 5 years ago

I'm assuming a !important won't solve this?

Unfortunately not, !important is already applied! It's to do with elemenets being a child element of a parent with position and/or z-index. This website explains it in some detail - https://coder-coder.com/z-index-isnt-working/

ibastevan commented 5 years ago

In bootstrap-tourist.js we could add another backdrop option something like -

In the global options:

backdropOptions:    
{               
    backdropSibling: false,
}

And then in the Tour.prototype._showHighlightOverlay = function (step) function, we could add:

if (step.backdropOptions.backdropSibling == true) 
{
    $(DOMID_HIGHLIGHT).insertAfter(".tour-highlight-element");
    $(DOMID_BACKDROP).css('z-index', '1029').clone().insertAfter(".tour-highlight-element");
}

Now in the step, if we add this to the step, it should do the trick -

backdropOptions: {
    backdropSibling: true
}

The reason why we clone the backdrop is because we still want the backdrop to cover the entire page, but we also want the backdrop to cover the elements inside the container the highlight element is in. The highlight div only needs to be in one place so that can be moved altogether. We add the z-index of 1029 to the backdrop so the backdrop comes behind the navbar div as the cloned backdrop will cover the elements inside the navbar div. We may run into a duplicate ID warning on the cloned backdrop. So if we can remove or change the ID during the clone would be good. Perhaps add "-1" to the ID so we can remove it on next or previous step.

ibastevan commented 5 years ago

Probably would need properly factoring in, taking reflex, modals etc into account. @1Luc1 would still need to add the highlightColor to the backdrop options too.

ibastevan commented 5 years ago

We would have to sort out moving the highlight back to the correct place and removing the cloned backdrop when moving to the next or previous step too.

IGreatlyDislikeJavascript commented 5 years ago

Hang on, this is crazy complicated. Do intro and shepard have the same issues and deal with it in the same way?

Essentially we have to have 2 backdrops - 1 is the "real" backdrop, and the other is the backdrop for all elements that are inside the (grand)parent of the step.element?

I think we can fix this using your code and perhaps a new step flag to internally track the new elements. I'll need to look into this one. @ibastevan thanks again for your input as always!

ibastevan commented 5 years ago

It doesn’t seem all that complicated? I’m not sure but it could be worth while looking into their code or maybe doing a fiddle and see how theirs works on this type of step. They must have faced this issue and overcome it inside the JavaScript as it’s definitely not something that can be fixed using css... or they may not even have a solution.

IGreatlyDislikeJavascript commented 5 years ago

Getting the base functionality in Tourist is straight forward, it's the other cases I'm concerned about. The preventInteraction div will need fixing, all the modal handling changed, and all the animation capability we added (to the highlight etc) will be lost because we have to create and destroy a new set of divs so there's no transition anymore.

Let's take a look at intro and shepherd, see if they have a solution, because I'm a bit reluctant to change so much and lose functionality...

ibastevan commented 5 years ago

That's true, I think it will be ok for preventinteraction and modal dialogs but we should definitely get a fiddle working and check out the other tour plugins.

ibastevan commented 5 years ago

Intro seemed to have this issue - https://github.com/usablica/intro.js/issues/96 also here - https://github.com/usablica/intro.js/issues/50 I tried playing around with a fiddle and it doesnt seem to work out of the box on fixed position steps. I will look more into it tomorrow when i am at my desk.

I added position fixed to the div in this fiddle and it breaks intro.js - http://jsfiddle.net/umi/woxewfag/

IGreatlyDislikeJavascript commented 5 years ago

Good to know intro has the same issue, thanks for putting that together.

I'll look into the code more tomorrow too. I see what you mean that preventInteraction will still work. I'm not sure about objects in modals, think we need to work through that use case if there's a fixed in a modal... Or maybe it's handled in exactly the same way and solving the fixed issue will solve it everywhere.

I think we need to try and find a solution that doesn't require creating/destroying new divs. Coding it is fine but I think we lose a lot of functionality if the DOM elems (highlight etc) aren't consistent.

ibastevan commented 5 years ago

I think we need to try and find a solution that doesn't require creating/destroying new divs. Coding it is fine but I think we lose a lot of functionality if the DOM elems (highlight etc) aren't consistent.

Are we creating/destroying new divs though? Is it not just moving the already created divs to a different location?

IGreatlyDislikeJavascript commented 5 years ago

Then maybe I'm misunderstanding how to solve this. I thought the problem was an element in a parent container with a z index, which causes it to "ignore" the tourist z index css setting for the tour step. To solve it, we had to:

  1. adjust the z index of the element parent above the normal tourist backdrop divs
  2. create new highlight and backdrop divs as siblings to the element
  3. Adjust z index of element above the new sibling divs etc

Have I misunderstood? If not, step 2 means we duplicate divs on the step.

ibastevan commented 5 years ago

I don't think your misunderstanding it, i think that you can see all the potential issues that can arise from making adjustments, you're looking at the "bigger picture".

It depends how we go about it. Essentially if we want to keep the users from having to alter any of their existing html, then the best way going about it would be leaving the z-indexes as they are. We don't need to create a new highlight or backdrop div from scratch. We just need to change the location of where the highlight div goes (through the .insertBefore() function). And for the backdrop that is the only thing that needs the z-index altering for this step and cloning the div to show up next to the highlight div's position. Then on next/previous we would need to destroy the cloned backdrop div and move the highlight back to its original place.

I'm not sure on this but would we be best (now we know the bug) starting a new thread for this or pull request or continue it on here?

ibastevan commented 5 years ago

I havent tested my commit in any other scenarios but this should should do the job for @1Luc1. Just need to add this to the steps inside the navbar.

backdropOptions: {
                        backdropSibling: true
                    }

@IGreatlyDislikeJavascript I am sure you will need to more elegantly refactor this into tourist and we will need to test the other use cases.

1Luc1 commented 5 years ago

I havent tested my commit in any other scenarios but this should should do the job for @1Luc1. Just need to add this to the steps inside the navbar.

backdropOptions: {
                        backdropSibling: true
                    }

@IGreatlyDislikeJavascript I am sure you will need to more elegantly refactor this into tourist and we will need to test the other use cases.

Thanks for you commitment! I just tried it and it looks like in v0.2.0. Unfortunately after closing the help, the element <div class="tour-backdrop" id="tourBackdrop-temp"></div> is still there, which keeps the whole thing black.

ibastevan commented 5 years ago

This is a hacky way to fix that but until we have time to properly address this issue I would recommend adding this to the step so you are not held back

onEnd: function(tour) {
   $('#tourBackdrop-temp').remove();
   $('#tourHighlight-temp').remove();
}

Sorry this wont wont but i have just commited a fix.

IGreatlyDislikeJavascript commented 5 years ago

I'm not sure on this but would we be best (now we know the bug) starting a new thread for this or pull request or continue it on here?

I think a new thread/pull is a good idea, thanks. Let's take this over to your pull, I want to work through it a bit more.

IGreatlyDislikeJavascript commented 5 years ago

Ok can't see your pull :-) ...

We just need to change the location of where the highlight div goes (through the.insertBefore() function). And for the backdrop that is the only thing that needs the z-index altering for this step and cloning the div to show up next to the highlight div's position. Then on next/previous we would need to destroy the cloned backdrop div and move the highlight back to its original place.

Right sorry that's my misunderstanding. I forgot how insertBefore/insertAfter worked, I thought it created clones by default, but I checked docs and it only creates clones if there are multiple DOM elements.

Now I've worked it through in my head and seen your code, I think this is fixable easily.

Instead of the onEnd hack, we can just add the removal code into _updateBackdropElements() at the top of the func. This is called on every step to correctly set the state of background and highlight, and is the best place for it - because tour step flow could alter when other funcs are called.

So I think the answer here is:

  1. add the clone backdrop and move the highlight in exactly as you said
  2. give the clone backdrop a new ID so we can easily remove it, again like you said!
  3. in _updateBackdropElements(), destroy the cloned backdrop (top of func)
  4. let the rest of the logic continue, even if it recreates the clone because the next step is backdropSibling == true

Does that sound about right? If you've already got a pull waiting, do you want to fix in yours and I'll merge to master?

ibastevan commented 5 years ago

I tried moving the highlight but it breaks the animations so had to clone that one too and just hide the original using -1 zindex. I haven’t done a pull, just updated my fork today. Other than that all sounds about right! I won’t be back online till Monday

IGreatlyDislikeJavascript commented 5 years ago

Yeah, the animation etc was my concern. Does it look right when you clone it? Surely the animation would still break, because the cloned highlight is bounded to a container, so it can't seamlessly work.

If I get time, I'll grab your fork and test out some options over the weekend, otherwise we can pick this up next week. Thanks as always :-)

ibastevan commented 5 years ago

I think it’s working quite well cloning it. Should be ok!

IGreatlyDislikeJavascript commented 5 years ago

@ibastevan I've just merged tszulc's pull into master, because he's done a great job sorting out the npm structure stuff that I don't know about.

So I think the easiest thing to do is for me to manually add your changes into master. Have you made any changes other than the ones shown in the commits in this thread?

ibastevan commented 5 years ago

@IGreatlyDislikeJavascript i havent made any changes. Well done for all your hard work, have a nice holiday!

IGreatlyDislikeJavascript commented 4 years ago

I'm back :-) . @ibastevan I've incorporated your changes into repo & release v0.3.2 (on github) - thank you!

I'll do the npm update shortly as per the other thread.

IGreatlyDislikeJavascript commented 4 years ago

Updated on NPM, thanks for everyone's input and support!

1Luc1 commented 4 years ago

Thanks for your work. Sorry for my late response.

Just tested the new version 0.3.2 and unfortunately it doesn't seem to work as indented.

As of with version 0.2.1: https://jsfiddle.net/x0mda4z2/ As you can see, the blue icon is visible.

With version 0.3.2: https://jsfiddle.net/szkod0jn/ the blue icon is not visible Also there is a slightly delay where the backdrop is visible for quite long after the tourist shows up.

Bootstrap version 3.4.1

As I already said, maybe I set up something wrong or this is just the wrong way to use it.

Thanks again for your help!

IGreatlyDislikeJavascript commented 4 years ago

@1Luc1 thanks for your feedback. I'm happy it does work, so I'm not going to re-open this yet :-) . Have a look at this fiddle: https://jsfiddle.net/bL4s7z91/1/

To test it, simply run the fiddle and click the burger to show the hidden bar. The tour step will immediately show correctly.

Here's what I did.

  1. Added the backdropOptions object to tour step.
  2. Added backdropOptions.highlightColor: #000 - this fixes the visual appearance of the highlight by telling Tourist that your element is on a black background
  3. Added backdropOptions.backdropSibling: true - this enables @ibastevan 's fix for your issue reported in this thread
  4. Added delayOnElement.delayElement: "element" - this is not strictly required, but is needed for the fiddle to work. This instructs Tourist to wait for the step element (#tourHelp) to appear before showing the tour step. I'm assuming in your actual code, the previous step has onNext defined as a function and some code in there ensures #tourhelp is visible. See the next paragraph also.

Please note: your fiddle set up the tour on an element that is initially hidden. So, in addition to delayOnElement I've also had to add the following code in because otherwise there's no way to make your fiddle work - a tour step element must be visible for something to happen, so I just started the tour when you click the burger to make the element visible. This is unrelated to your issue.

$("#w31").click(function()
{
    tour.restart();
});
1Luc1 commented 4 years ago

Thanks for the clarification! Now it works quite well; one thing still confuses me a little bit and maybe thats as well some configuration problem.

If i set up the example without backdrop there is a short time where a black screen appears and disappears followed by the actual tour dialog. https://jsfiddle.net/q6eb27oj/ (click on the icon) tested with FF, Edge, Chrome

Within my understanding, within this example the tour dialog just should pop up, without the black screen flickering.

IGreatlyDislikeJavascript commented 4 years ago

If i set up the example without backdrop there is a short time where a black screen appears and disappears followed by the actual tour dialog

I think you found a bug, but it's not the bug you think :-)

So with your fiddle, you must use the delayOnElement option. Simply, the tour is started when you click the burger. But the transition effect to show the navbar, and therefore for $("#tourHelp").visible() === true , takes longer than the tour to initialize. You must tell Tourist that it should wait for the element to be visible.

However there is a bug here due to Tourist initialization, when the divs are created. The init creates the backdrop visible, and because there's a delay before the step is shown, the step.backdrop doesn't hide it quickly enough.

I'm going to open a new issue on this one. Thanks for reporting it!