incorrect isOrphan Step detection under certain conditions

[angek]

firstly, thank you for bootstrap-tourist. It looks very promising and I cant wait to play with it properly.
I've just set it up on one of my sites just to see how much I can break it and have come across what I believe is an incorrect isOrphan detection.
Created the tour and added 2 steps with both steps intentionally assigned to non-existent elements.

jQuery(document).ready( function() {
	var tour = new Tour({
		framework: 'bootstrap3',
		sanitizeFunction: function(stepContent) {
			//return DOMPurify.sanitize(stepContent);
			return stepContent;
		},
		onPreviouslyEnded: function (tour) {
			tour.restart();
		},
		steps: [{
			placement: 'right',
			showIfUnintendedOrphan: true,
			//orphan:true,
			showProgressBar: false,
			showProgressText: false,
			backdrop: true,
			element: '#img1', //intentionally set to non-existent page (DOM) element
			title: 'Step One',
			content: '<p>This is the preview of your popover</p><p>Set your options at the left so your popovers will match natively with your product</p>'
		},
		{
			placement: 'left',
			showIfUnintendedOrphan: true,
			//orphan: true,
                         backdrop: true,
                         showProgressBar: false,
			showProgressText: false,
		        element: '#imgxx', //intentionally set to non-existent page (DOM) element
			title: 'Step Two',
			content: '<p>This is the preview of your popover</p><p>Set your options at the left so your popovers will match natively with your product</p>',
		}]
	});
	
	tour.start();
});

I added some further debugging in the code to check the isOrphan function and then launched the tour.
when the tour starts, the first step is positioned correctly in the center of the page and the debugging output shows:

checking if step is an orphan....
     - step.orphan: false
     - step.element: #img1
     - step.placement: right
     - $(step.element).length:  0
     - $(step.element) hidden:  false
 orphan status is: true

I click the next button and step 2 displayed as intended (ie, it satisfied all the conditions to be declared an orphan).
I then click the back button and this is where the strange behaviour comes in. The step (step 1) is again displayed however it is now positioned at the very top of the page and the step.element, instead of being '#img1' is 'body'
debugging output is as follows:

checking if step is an orphan....
     - step.orphan: false
     - step.element: body
     - step.placement: top
     - $(step.element).length:  1
     - $(step.element) hidden:  false
 orphan status is: false

from here on in, moving through the tour, whether forwards or backwards, the steps stay positioned at the top of the page and the step.element remains assigned to 'body'.

I hope I've made sense :)

[IGreatlyDislikeJavascript]

Thanks for your very clear report! I've been caught up with other things recently so haven't had time to look at this properly (or the other couple of reports currently open).

I'm starting to think that something has been missed in the step reload flow, possibly because of the way the step.element has to be handled due to the legacy Tour code. For info, the legacy Tour code mangled the step.element, switching it back and forth between string literal and jq object. I had to work around this when implementing the "step element as function" feature, so it's possible I've introduced a bug.

Your report is for a different issue but I think yours and this report are symptoms of the same root cause: #44

I'll try to take a look soon, unfortunately it may be a little time before I can get around to it. If you'd like to try fixing it I'll be happy to help guide you, I just don't have access to a machine and the code at this min.

[angek]

Thank you for getting back to me. I'm more than happy to try to resolve the issue and appreciate your offer for guidance. I'll be in touch.

[angek]

I've had some trouble identifying where exactly the step.element is being mangled due to string literal and jq object switching.
It did however occur to me that, given the tour steps are initially correctly identified as orphans, why dont we just explicitly set step.orphan to true.

Propose to change _showPopover function (at around line 1236) from:

if (isOrphan)
{
	// Note: BS4 popper.js requires additional fiddling to work, see below where popOpts object is created
	step.element = 'body';
	step.placement = 'top';

	// If step is an intended or unintended orphan, and reflexOnly is set, show a warning.
	if(step.reflexOnly)
	{
		this._debug("Step is an orphan, and reflexOnly is set: ignoring reflexOnly");
	}
}

to:

if (isOrphan)
{
	// Note: BS4 popper.js requires additional fiddling to work, see below where popOpts object is created
        
        //successfully identified the step as being an orphan so lets explicitly set it here.
	step.orphan = true;

	step.element = 'body';
	step.placement = 'top';

	// If step is an intended or unintended orphan, and reflexOnly is set, show a warning.
	if(step.reflexOnly)
	{
		this._debug("Step is an orphan, and reflexOnly is set: ignoring reflexOnly");
	}
}

Thoughts?

[angek]

Ive also taken a quick look at issue #44 since you indicated it may be somewhat related. I'll comment further directly in that issue.

[IGreatlyDislikeJavascript]

Thank you for working on this. I'll need to take a quick look at the code to give you exact locations and feedback on this, so a quick signpost for now and hopefully some more specific feedback tomorrow.

where exactly the step.element is being mangled due to string literal and jq object switching.

In the step show flow, after the step is loaded the step.element is checked for typeof function and evaluated. The result is then put back into the temporary "live" step.element. This was necessary because (I believe, off the top of my head) the original showStepHelper and other areas of code switches between using $(element) and step.element = $element = $(element) , I.e.: depending on where you are in the show step flow, the legacy code could be using a jq object or string literal - and the only reason the code doesn't break is because jq elegantly handles $(already a jq collection object). Implementing the function for step.element broke this, so I had to work around it.

On your suggestion for setting orphan, that makes sense but let me double check the code flow please. I think there might be a case where keeping step.orphan intact is necessary so that a comparison between step.orphan (the intended setting for the step) and the result of isOrphan() (the real current orphan status of the step) is needed.

Thanks again for your work on this and #44

[IGreatlyDislikeJavascript]

Apologies, this took a bit longer than expected. Thanks again for your feedback.

First, step.element mangling. For info, the logical flow for step management is as follows:

1. Step is changed (first load, user presses button, code evaluates a step change etc). Let's assume "next" is clicked.
2. Next button triggers a call to Tour.prototype.next()
3. Function hides current step, and creates a promise to call _showNextStep()
4. _showNextStep() calls getStep(), to "load" the appropriate next step
5. _showNextStep() calls showStep(), the general purpose function that shows a step and handles all the various options (modal management, delayOnElement etc)

Part 1: in Tour.prototype.getStep() you'll see the code that tests for step.element === function, and evaluates it if so (line 316-ish). getStep() is called at every time a step is "loaded" or "changed", i.e.: if the tour flow is moved by the user clicking a button, or by code, or by any other action such as delayOnElement timing out. So I added this piece of code to enable the element by function feature.

Of course, this has a side effect - the code doesn't check that the returned value of the step.element == func() is actually a valid DOM element (string literal) or jq object. There are 2 reasons for this. First, no validation on the element (returned value) is done because the remaining code in showStep handles unintended orphans and other error conditions. Second, the legacy Tour code doesn't hygienically handle the step.element, as per next part:

Part 2: in showStep you'll see code like $(step.element) and most importantly this $element = $(step.element); . The legacy Tour code made implicit assumptions about the contents of step.element that may not be true (not just the element by function feature I added, but orphan steps and similar), hence my comment about "mangling".

Ideally I want to work through all the step management code and hold step.element in its original form as per step option, and manage the "live" element at runtime through a specific, validated $element object. That will clear up some confusion and make future features easier, but it's a bit of a painstaking process to check how it's used in every case.

On your orphan suggestion, as I suspected it's not quite as straight forward as we'd hope (again because of legacy code). Line 1221 has isOrphan = this._isOrphan(step);. _isOrphan is a live, runtime check on whether the step is an orphan.

Orphan status is checked in multiple ways unfortunately - by step option, by the isOrphan() function , and by a variable that's held following an isOrphan check (like line 1221). Changing the step option to reflect orphan status I think will cause some unintended side effects and prevent unintended orphan detection.

My principle going forward is to not alter the step options. Instead I want to track things like this through the step flags (see line 385 _setStepFlag() and an example of its use with modals on line 644.

There's clearly an underlying bug here though that I need to work on.

Let me know your thoughts - and I'll take a look at the other issue later. Thank you for your feedback!