Fix bug where too many marks could be rendered on slider

KoolADE85 · KoolADE85 · commit bbcedf1bce90 · 2025-11-18T10:20:03.000-07:00
diff --git a/components/dash-core-components/src/utils/computeSliderMarkers.ts b/components/dash-core-components/src/utils/computeSliderMarkers.ts
@@ -1,3 +1,4 @@
+/* eslint-disable no-magic-numbers */
 import {pickBy, isEmpty, isNil} from 'ramda';
 import {formatPrefix} from 'd3-format';
 import {SliderMarks} from '../types';
@@ -38,7 +39,6 @@ const estimateBestSteps = (
     stepValue: number,
     sliderWidth?: number | null
 ) => {
-    // Base desired count for 330px slider with 0-100 scale (10 marks = 11 total including endpoints)
     let targetMarkCount = 11; // Default baseline
 
     // Scale mark density based on slider width
@@ -87,31 +87,30 @@ const estimateBestSteps = (
         idealInterval = range / (targetMarkCount - 1);
     }
 
-    // Find the best interval that's a multiple of stepValue
-    // Start with multiples of stepValue and find the one closest to idealInterval
-    const stepMultipliers = [
-        // eslint-disable-next-line no-magic-numbers
-        1, 2, 2.5, 5, 10, 20, 25, 50, 100, 200, 250, 500, 1000,
-    ];
-
-    let bestInterval = stepValue;
-    let bestDifference = Math.abs(idealInterval - stepValue);
-
-    for (const multiplier of stepMultipliers) {
-        const candidateInterval = stepValue * multiplier;
-        const difference = Math.abs(idealInterval - candidateInterval);
-
-        if (difference < bestDifference) {
-            bestInterval = candidateInterval;
-            bestDifference = difference;
-        }
-
-        // Stop if we've gone too far beyond the ideal
-        if (candidateInterval > idealInterval * 2) {
-            break;
-        }
+    // Calculate the multiplier needed to get close to idealInterval
+    // Round to a "nice" number for cleaner mark placement
+    const rawMultiplier = idealInterval / stepValue;
+
+    // Round to nearest nice multiplier (1, 2, 2.5, 5, or power of 10 multiple)
+    const magnitude = Math.pow(10, Math.floor(Math.log10(rawMultiplier)));
+    const normalized = rawMultiplier / magnitude; // Now between 1 and 10
+
+    let niceMultiplier;
+    if (normalized <= 1.5) {
+        niceMultiplier = 1;
+    } else if (normalized <= 2.25) {
+        niceMultiplier = 2;
+    } else if (normalized <= 3.5) {
+        niceMultiplier = 2.5;
+    } else if (normalized <= 7.5) {
+        niceMultiplier = 5;
+    } else {
+        niceMultiplier = 10;
     }
 
+    const bestMultiplier = niceMultiplier * magnitude;
+    const bestInterval = stepValue * bestMultiplier;
+
     // All marks must be at valid step positions: minValue + (n * stepValue)
     // Find the first mark after minValue that fits our desired interval
     const stepsInInterval = Math.round(bestInterval / stepValue);
@@ -208,38 +207,18 @@ export const autoGenerateMarks = (
     );
     let cursor = start;
 
-    // Apply a safety cap to prevent excessive mark generation while preserving existing behavior
-    // Only restrict when marks would be truly excessive (much higher than the existing UPPER_BOUND)
-    const MARK_WIDTH_PX = 20; // More generous spacing for width-based calculation
-    const FALLBACK_MAX_MARKS = 200; // High fallback to preserve existing behavior when no width
-    const ABSOLUTE_MAX_MARKS = 200; // Safety cap against extreme cases
-
-    const widthBasedMax = sliderWidth
-        ? Math.max(10, Math.floor(sliderWidth / MARK_WIDTH_PX))
-        : FALLBACK_MAX_MARKS;
-
-    const maxAutoGeneratedMarks = Math.min(widthBasedMax, ABSOLUTE_MAX_MARKS);
-
-    // Calculate how many marks would be generated with current interval
-    const estimatedMarkCount = Math.floor((max - start) / interval) + 1;
-
-    // If we would exceed the limit, increase the interval to fit within the limit
-    let actualInterval = interval;
-    if (estimatedMarkCount > maxAutoGeneratedMarks) {
-        // Recalculate interval to fit exactly within the limit
-        actualInterval = (max - start) / (maxAutoGeneratedMarks - 1);
-        // Round to a reasonable step multiple to keep marks clean
-        const stepMultiple = Math.ceil(actualInterval / chosenStep);
-        actualInterval = stepMultiple * chosenStep;
-    }
-
-    if ((max - cursor) / actualInterval > 0) {
-        do {
+    if ((max - cursor) / interval > 0) {
+        while (cursor < max) {
             marks.push(alignValue(cursor, chosenStep));
-            cursor += actualInterval;
-        } while (cursor < max && marks.length < maxAutoGeneratedMarks);
+            const prevCursor = cursor;
+            cursor += interval;
+
+            // Safety check: floating point precision could impact this loop
+            if (cursor <= prevCursor) {
+                break;
+            }
+        }
 
-        // do some cosmetic
         const discardThreshold = 1.5;
         if (
             marks.length >= 2 &&
diff --git a/components/dash-core-components/tests/integration/sliders/test_marks_density.py b/components/dash-core-components/tests/integration/sliders/test_marks_density.py
@@ -0,0 +1,89 @@
+from dash import Dash, dcc, html
+
+
+def test_slsl_extreme_range_marks_density(dash_dcc):
+    """
+    Test that extreme ranges don't generate too many overlapping marks.
+
+    With min=-1, max=480256671, and container width ~365px, we should have
+    no more than ~7 marks to prevent overlap (given the long labels).
+    """
+    app = Dash(__name__)
+    app.layout = html.Div(
+        style={"width": "365px"},
+        children=[
+            dcc.RangeSlider(
+                id="rangeslider-extreme",
+                min=-1,
+                max=480256671,
+                value=[-1, 480256671],
+            )
+        ],
+    )
+
+    dash_dcc.start_server(app)
+
+    # Wait for component to render
+    dash_dcc.wait_for_element("#rangeslider-extreme")
+
+    # Count the rendered marks
+    marks = dash_dcc.find_elements(".dash-slider-mark")
+    mark_count = len(marks)
+
+    print(f"Number of marks rendered: {mark_count}")
+
+    # Get the actual mark text to verify what's rendered
+    mark_texts = [mark.text for mark in marks]
+    print(f"Mark labels: {mark_texts}")
+
+    # Should have between 2 and 7 marks (min/max plus a few intermediate)
+    assert 2 <= mark_count <= 7, (
+        f"Expected 2-7 marks for extreme range, but found {mark_count}. "
+        f"This suggests overlapping marks. Labels: {mark_texts}"
+    )
+
+    # Verify min and max are included
+    assert "-1" in mark_texts, "Min value (-1) should be included in marks"
+    assert any(
+        "480" in text or "M" in text for text in mark_texts
+    ), "Max value should be included in marks"
+
+    assert dash_dcc.get_logs() == []
+
+
+def test_slsl_extreme_range_no_width(dash_dcc):
+    """
+    Test that extreme ranges work even before width is measured.
+
+    This simulates the initial render state where sliderWidth is null.
+    """
+    app = Dash(__name__)
+    app.layout = html.Div(
+        # No explicit width, so ResizeObserver will measure it
+        children=[
+            dcc.RangeSlider(
+                id="rangeslider-no-width",
+                min=-1,
+                max=480256671,
+                value=[-1, 480256671],
+            )
+        ],
+    )
+
+    dash_dcc.start_server(app)
+
+    # Wait for component to render
+    dash_dcc.wait_for_element("#rangeslider-no-width")
+
+    # Count the rendered marks
+    marks = dash_dcc.find_elements(".dash-slider-mark")
+    mark_count = len(marks)
+
+    print(f"Number of marks rendered (no explicit width): {mark_count}")
+
+    # Even without explicit width, should not have too many marks
+    assert (
+        mark_count <= 7
+    ), f"Expected <= 7 marks even without explicit width, but found {mark_count}"
+
+    assert dash_dcc.get_logs() == []
diff --git a/components/dash-core-components/tests/unit/computeSliderMarkers.test.ts b/components/dash-core-components/tests/unit/computeSliderMarkers.test.ts
@@ -338,4 +338,80 @@ describe('Dynamic Slider Mark Density', () => {
             expect(areAllMarksValidSteps(width660, 0, 5)).toBe(true);
         });
     });
+
+    describe('Extreme ranges with large numbers', () => {
+        test('should not create overlapping marks for range -1 to 480256671 WITHOUT width (initial render)', () => {
+            const marks = sanitizeMarks({
+                min: -1,
+                max: 480256671,
+                marks: undefined,
+                step: undefined, // Let it auto-calculate
+                sliderWidth: null, // Simulates initial render before width is measured
+            });
+
+            const positions = getMarkPositions(marks);
+
+            // Should have min and max
+            expect(positions[0]).toBe(-1);
+            expect(positions[positions.length - 1]).toBe(480256671);
+
+            // Should have reasonable number of marks to prevent overlap even without width
+            // With ~9-character labels (480256671), we need substantial spacing
+            // Labels like "45M", "95M" are ~3-4 chars, so reasonable mark count is 5-7
+            expect(positions.length).toBeGreaterThanOrEqual(2); // At least min and max
+            expect(positions.length).toBeLessThanOrEqual(11); // Not too many to cause overlap
+
+            // Even without explicit width, assume a reasonable default (330px baseline)
+            // and verify spacing would be sufficient
+            const estimatedSpacing = 330 / (positions.length - 1);
+            expect(estimatedSpacing).toBeGreaterThanOrEqual(30);
+        });
+
+        test('should not create overlapping marks for range -1 to 480256671 at 365px width', () => {
+            const marks = sanitizeMarks({
+                min: -1,
+                max: 480256671,
+                marks: undefined,
+                step: undefined, // Let it auto-calculate
+                sliderWidth: 365,
+            });
+
+            const positions = getMarkPositions(marks);
+
+            // Should have min and max
+            expect(positions[0]).toBe(-1);
+            expect(positions[positions.length - 1]).toBe(480256671);
+
+            // Should have reasonable number of marks to prevent overlap
+            // With 365px width and ~9-character labels (480256671), we need substantial spacing
+            // Estimate: 9 chars * 8px/char = 72px per label, so max ~5 marks for 365px
+            expect(positions.length).toBeGreaterThanOrEqual(2); // At least min and max
+            expect(positions.length).toBeLessThanOrEqual(7); // Not too many to cause overlap
+
+            // Verify spacing between marks is sufficient
+            // With 365px width, marks should be at least 50px apart for long labels
+            const estimatedSpacing = 365 / (positions.length - 1);
+            expect(estimatedSpacing).toBeGreaterThanOrEqual(50);
+        });
+
+        test('should handle very large ranges with appropriate step multipliers', () => {
+            const marks = sanitizeMarks({
+                min: 0,
+                max: 1000000000, // 1 billion
+                marks: undefined,
+                step: undefined,
+                sliderWidth: 330,
+            });
+
+            const positions = getMarkPositions(marks);
+
+            // Should have reasonable mark count
+            expect(positions.length).toBeGreaterThanOrEqual(2);
+            expect(positions.length).toBeLessThanOrEqual(15);
+
+            // Should include min and max
+            expect(positions[0]).toBe(0);
+            expect(positions[positions.length - 1]).toBe(1000000000);
+        });
+    });
 });
