Timeline.add(data) will cause 0 distance between ticks when the timeline is hidden when .add() is called

[Tyfui90]

When an embedded timeline is rendered but is currently hidden, then when when Timeline.add is called, the distance between timeMakers is set to 0 as the current width of the hidden Timeline is calculated as 0 and therefore the distance between each marker is 0.

The Tab the timeline embed is rendered in

Then navigating to a different tab (Add a memory) hides the timeline, and it is while the timeline is hidden that Timeline.add is called and the width is calculated to be zero.

so when navigating back to the Timeline the markers are no longer visible as the distance between them is now 0

Timeline.js

// Add an event
add(data) {
    var unique_id = this.config.addEvent(data);

    var n = this._getEventIndex(unique_id);
    var d = this.config.events[n];

    this._storyslider.createSlide(d, this.config.title ? n + 1 : n);
    this._storyslider._updateDrawSlides();

    this._timenav.createMarker(d, n);
    this._timenav._updateDrawTimeline(false);

    this.fire("added", { unique_id: unique_id });
}

Calls this._timenav._updateDrawTimeline(false);

TimeNav.js

_updateDrawTimeline(check_update) {
    var do_update = false;

    // Check to see if redraw is needed
    if (check_update) {
        /* keep this aligned with _getTimeScale or reduce code duplication */
        var temp_timescale = new TimeScale(this.config, {
            display_width: this._el.container.offsetWidth,
            screen_multiplier: this.options.scale_factor,
            max_rows: this.max_rows

        });

        if (this.timescale.getMajorScale() == temp_timescale.getMajorScale() &&
            this.timescale.getMinorScale() == temp_timescale.getMinorScale()) {
            do_update = true;
        }
    } else {
        do_update = true;
    }

    // Perform update or redraw
    if (do_update) {
        this.timescale = this._getTimeScale();
        this.timeaxis.positionTicks(this.timescale, this.options.optimal_tick_width);
        this.positionMarkers();
        this._assignRowsToMarkers();
        this._positionGroups();
        if (this.has_eras) {
            this._positionEras();
        }
        this.updateDisplay();
    } else {
        this._drawTimeline(true);
    }

    return do_update;

}

Which then calls this.timeaxis.positionTicks(this.timescale, this.options.optimal_tick_width);

TimeAxis.js

_positionTickArray(tick_array, timescale, optimal_tick_width) {
    // Poition Ticks & Handle density of ticks
    if (tick_array[1] && tick_array[0]) {
        var distance = (timescale.getPosition(tick_array[1].date.getMillisecond()) - timescale.getPosition(tick_array[0].date.getMillisecond())),
            fraction_of_array = 1;

        if (distance < optimal_tick_width) {
            fraction_of_array = Math.round(optimal_tick_width / timescale.getPixelsPerTick());
        }

        var show = 1;

        for (var i = 0; i < tick_array.length; i++) {

            var tick = tick_array[i];

            // Poition Ticks
            tick.tick.style.left = timescale.getPosition(tick.date.getMillisecond()) + "px";
            tick.tick_text.innerHTML = tick.display_date;

            // Handle density of ticks
            if (fraction_of_array > 1) {
                if (show >= fraction_of_array) {
                    show = 1;
                    tick.tick_text.style.opacity = 1;
                    tick.tick.className = "tl-timeaxis-tick";
                } else {
                    show++;
                    tick.tick_text.style.opacity = 0;
                    tick.tick.className = "tl-timeaxis-tick tl-timeaxis-tick-hidden";
                }
            } else {
                tick.tick_text.style.opacity = 1;
                tick.tick.className = "tl-timeaxis-tick";
            }

        }
    }
}

The main issue is that

display_width: this._el.container.offsetWidth,

this._el.container.offsetWidth can be equal to 0 when the timeline is not currently on screen. Which causes the downstream calculation in the TimeAxis to have a display_width of 0 and end up calculating for timescale.getPosition incorrectly

tick.tick.style.left = timescale.getPosition(tick.date.getMillisecond()) + "px";

As the display_width is used in the Timescale.js constructor, it will create a _pixel_width of 0

this._pixel_width = this._screen_multiplier * this._display_width;

which is what _pixels_per_milli is calculated from, and will also be 0

 this._pixels_per_milli = this.getPixelWidth() / this._span_in_millis;

As updates call the this.timeaxis.positionTicks for getting the Ticks positions, this will always be 0 when _pixels_per_milli as display_width is 0

getPosition(time_in_millis) {
    // be careful using millis, as they won't scale to cosmological time.
    // however, we're moving to make the arg to this whatever value
    // comes from TLDate.getTime() which could be made smart about that --
    // so it may just be about the naming.
    return (time_in_millis - this._earliest) * this._pixels_per_milli
}

I suggest that TimeNav.js be updated in 2 locations when it creates a new TimeScale in _getTimeScale(), and in_updateDrawTimeline(check_update)

Changing their respective display_width to either be display_width: this._el.container.offsetWidth, or if that is 0 then default to the this.options.width value so the last known value of the width is then used.

so replace

display_width: this._el.container.offsetWidth,

with

display_width: this._el.container.offsetWidth === 0 ? this.options.width : this._el.container.offsetWidth,

Eg, below is are the functions with this change. I can confirm that this change works for me when implemented for my use case TimeNav.js

_getTimeScale() {
    /* maybe the establishing config values (marker_height_min and max_rows) should be
    separated from making a TimeScale object, which happens in another spot in this file with duplicate mapping of properties of this TimeNav into the TimeScale options object? */
    // Set Max Rows
    var marker_height_min = 0;
    try {
        marker_height_min = parseInt(this.options.marker_height_min);
    } catch (e) {
        trace("Invalid value for marker_height_min option.");
        marker_height_min = 30;
    }
    if (marker_height_min == 0) {
        trace("marker_height_min option must not be zero.")
        marker_height_min = 30;
    }
    this.max_rows = Math.round((this.options.height - this._el.timeaxis_background.offsetHeight - (this.options.marker_padding)) / marker_height_min);
    if (this.max_rows < 1) {
        this.max_rows = 1;
    }
    return new TimeScale(this.config, {
        display_width: this._el.container.offsetWidth === 0 ? this.options.width : this._el.container.offsetWidth,
        screen_multiplier: this.options.scale_factor,
        max_rows: this.max_rows

    });
}

TimeNav.js

_updateDrawTimeline(check_update) {
    var do_update = false;

    // Check to see if redraw is needed
    if (check_update) {
        /* keep this aligned with _getTimeScale or reduce code duplication */
        var temp_timescale = new TimeScale(this.config, {
            display_width: this._el.container.offsetWidth === 0 ? this.options.width : this._el.container.offsetWidth,
            screen_multiplier: this.options.scale_factor,
            max_rows: this.max_rows

        });

        if (this.timescale.getMajorScale() == temp_timescale.getMajorScale() &&
            this.timescale.getMinorScale() == temp_timescale.getMinorScale()) {
            do_update = true;
        }
    } else {
        do_update = true;
    }

    // Perform update or redraw
    if (do_update) {
        this.timescale = this._getTimeScale();
        this.timeaxis.positionTicks(this.timescale, this.options.optimal_tick_width);
        this.positionMarkers();
        this._assignRowsToMarkers();
        this._positionGroups();
        if (this.has_eras) {
            this._positionEras();
        }
        this.updateDisplay();
    } else {
        this._drawTimeline(true);
    }

    return do_update;

}

Please let me know if theres anything thats unclear, or if you see any concerns with what im proposing

If you are OK with the proposed change I can submit a PR created that includes these changes

[JoeGermuska]

Timeline typically does not do well when manipulated without being visible onscreen, because it uses screen dimensions to compute all sorts of details about its own layout. This comes up more frequently when people initialize a timeline on a hidden tab, but it seems to be the same problem.

Our recommended approach in these cases is to add a call to timeline.updateDisplay() as part of whatever javascript makes the timeline visible.

~To be honest, I am having trouble understanding the details of your suggested fix, perhaps in part because the code formatting moves in and out of monospace code, etc. (Best practice is to use ``` to wrap code blocks, and using ```js at the beginning also gets nicer syntax highlighting.)~ (I see you fixed this while I was writing!)

That said, I think I get the gist. Trying to sort through your code is making me regret some of the code factoring decisions in the original TimelineJS. Still, this may not be the time to change it.

Here are a couple of adjustments I'd like to suggest, in the spirit of nudging this old code towards some better practices:

• for the timeline object to be responsible for choosing the dimensions, including any fallback strategy between real-display-dimensions vs cached-best-known-values-from-options. Then the timeline would just pass the chosen dimensions to all child display elements, reducing any replication of logic. I guess in some cases this would involve changing the method signatures on child elements, but especially the ones with _ prefixes are considered "private" so changing those shouldn't have outside impact .
• for the timeline to stop treating options as read/write, which I never liked, but inherited. Perhaps there should be a state object which holds any things that had been written to options, and could be initialized by copying them from options in the constructor. I'd be fine if we don't comb through all of the code to fix this now, but we could start in that direction by doing it for these values now.

It would also be great to add a test case so we can more easily check for regressions. I'll be the first to admit we don't have a very robust UI testing system, but perhaps we can discuss ideas. The simplest would probably be to add another NPM script like disttest and compare, although I'd love to hear a suggestion that didn't keep propagating single-use scripts if anyone has one.

Does this make sense?

PS upon further consideration, I realize that the child elements may need their own container widths instead of something which can be passed in from the containing timeline object... but for right now I don't have time to sort through the code details more ....

[Tyfui90]

I accidentally posted this while I was still formatting the issue, It should be a little less stream of consciousness now (hopefully). But it sounds like you understand what I was trying to describe.

Our recommended approach in these cases is to add a call to timeline.updateDisplay() as part of whatever javascript makes the timeline visible. I have tried this approach to invoke timeline.updateDisplay() using a tabChange event. It calls the updateDisplay(), but it doesnt cause and update to the tick width, and the display is still broken for them even after it is called this way. It may be that the tab change event calls the updateDisplay before the container width is back to being non zero. But this is why I went for the approach of finding out why the ticks were being set to 0.

for the timeline object to be responsible for choosing the dimensions, including any fallback strategy between real-display-dimensions vs cached-best-known-values-from-options.

Based on this, I do think I have over-complicated what I suggested as the fix. I have just checked and replacing

display_width: this._el.container.offsetWidth,

with

display_width: this.options.width,

This will default the width value to the timeline calculated value from the last time updateDisplay was invoked. and based on your comment, though this may still be 0 if timeline.updateDisplay() is called when the timeline isnt visible; we can do a the 0 width check and handle it appropriately in the _updateDisplay() inside Timeline..js, allowing for this.options.width to always default to a non zero value when used by any child container etc. I think that makes sense.

[Tyfui90]

Regarding Tests.

I've looked at the compare.html, and index.html thats being tested via disttest and compare, and im not familiar with how they are setting up and testing the UI elements. I'd be better able to write functional tests around the public functions i'd be making related changes to. I see there is src/js/tests/Timeline.test.js. I could look at adding tests around my use case, and the expected use case, to make sure the at the updateDisplay() should keep the options values related to height and width to be non-zero if the updateDisplay is called with 0 values for the current hidden container. I would also think of ways to test the add as that is the public api i was calling when i initially ran into the above issue