From 0f86030f5f7e1511b0a2e4e79bfe5a766903cfee Mon Sep 17 00:00:00 2001 From: dobby-d-elf Date: Fri, 15 May 2026 09:33:29 -0600 Subject: [PATCH] =?UTF-8?q?fix:=20single=20close=20button=20on=20workspace?= =?UTF-8?q?=20panel,=20tooltip=20=E2=86=92=20'Close'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove duplicate mobile-close-btn from HTML - Remove dead .mobile-close-btn CSS rules; unhide .close-preview at all viewports - Change btnClearPreview tooltip from 'Hide workspace panel' to 'Close' - Update tests across test_sprint41.py, test_sprint44.py, test_issue781.py, and test_mobile_layout.py to match new single-button model --- static/boot.js | 2 +- static/index.html | 1 - static/style.css | 4 - tests/test_issue781.py | 99 +++++--------------- tests/test_mobile_layout.py | 2 - tests/test_sprint41.py | 57 +----------- tests/test_sprint44.py | 180 +++++++++++++++--------------------- 7 files changed, 100 insertions(+), 245 deletions(-) diff --git a/static/boot.js b/static/boot.js index afd0f576..af545ef1 100644 --- a/static/boot.js +++ b/static/boot.js @@ -201,7 +201,7 @@ function syncWorkspacePanelUI(){ const clearBtn=$('btnClearPreview'); if(clearBtn){ clearBtn.disabled=!isOpen; - _setButtonTooltip(clearBtn, hasPreview?'Close preview':'Hide workspace panel'); + _setButtonTooltip(clearBtn, hasPreview?'Close preview':'Close'); if(!isCompact) clearBtn.style.display=''; } } diff --git a/static/index.html b/static/index.html index 669f33a5..592e6243 100644 --- a/static/index.html +++ b/static/index.html @@ -1190,7 +1190,6 @@ - diff --git a/static/style.css b/static/style.css index 156d3f55..bce4b9ff 100644 --- a/static/style.css +++ b/static/style.css @@ -1299,7 +1299,6 @@ @container rightpanel (max-width: 160px){ .workspace-panel-title-group{display:none;} } - .mobile-close-btn{display:none;} .panel-icon-btn{width:24px;height:24px;background:none;border:none;color:var(--muted);cursor:pointer;border-radius:5px;font-size:13px;display:flex;align-items:center;justify-content:center;transition:all .15s;} .panel-icon-btn:hover{background:var(--hover-bg);color:var(--text);} .panel-icon-btn:disabled{opacity:.35;cursor:not-allowed;} @@ -1435,8 +1434,6 @@ @media(max-width:900px){ .rightpanel{display:none} .workspace-toggle-btn,.mobile-files-btn{display:inline-flex!important;} - .mobile-close-btn{display:flex;} - .close-preview{display:none;} } @container composer-footer (max-width: 700px){ @@ -1538,7 +1535,6 @@ .rightpanel .resize-handle{display:none;} .rightpanel .panel-header{row-gap:8px;} .rightpanel .panel-actions{align-items:center;gap:8px;} - .rightpanel .mobile-close-btn{margin-left:auto;min-width:36px;min-height:36px;} /* Topbar adjustments */ .topbar{padding:8px 12px;gap:8px;} .topbar-title{font-size:14px;} diff --git a/tests/test_issue781.py b/tests/test_issue781.py index ea5f7106..93cf9cfd 100644 --- a/tests/test_issue781.py +++ b/tests/test_issue781.py @@ -1,12 +1,12 @@ """ -Tests for issue #781 — duplicate X close button in workspace preview header -on window resize below 900px breakpoint. +Tests for issue #781 — duplicate X close button in workspace preview header. + +The fix: a single btnClearPreview (.close-preview) is the only close button, +visible on all devices. The mobile-close-btn element was removed entirely. Verifies that: - - .close-preview is hidden (display:none) inside the @media (max-width:900px) block - - .mobile-close-btn is shown (display:flex) inside the same @media block -Both rules must appear inside the same @media(max-width:900px) block so that -at mobile widths only the mobile-close-btn is visible. + - .close-preview is NOT hidden by any media query (visible everywhere) + - .mobile-close-btn has no CSS rules remaining (element removed from HTML) """ import re @@ -44,88 +44,33 @@ def _extract_media_block(css, media_query_pattern): raise AssertionError("Unmatched brace in CSS after @media block") -def _strip_media_blocks(css): - """Remove all @media {...} blocks from CSS, returning base rules only.""" - result = [] - i = 0 - while i < len(css): - # Look for @media keyword - m = re.search(r"@media\b", css[i:]) - if not m: - result.append(css[i:]) - break - # Append everything before this @media - result.append(css[i : i + m.start()]) - # Find the opening brace of this @media block - brace_start = css.index("{", i + m.start()) - depth = 0 - j = brace_start - while j < len(css): - if css[j] == "{": - depth += 1 - elif css[j] == "}": - depth -= 1 - if depth == 0: - i = j + 1 - break - j += 1 - else: - break - return "".join(result) - - _MEDIA_900_PATTERN = r"@media\s*\(\s*max-width\s*:\s*900px\s*\)" -def test_mobile_close_btn_displayed_in_900px_block(): - """mobile-close-btn must be display:flex inside the 900px media query.""" +def test_close_preview_not_hidden_in_900px_block(): + """The single close button (.close-preview) must NOT be hidden in any media query.""" css = _load_css() block = _extract_media_block(css, _MEDIA_900_PATTERN) - assert ".mobile-close-btn" in block, ( - ".mobile-close-btn rule is missing from @media(max-width:900px) block" - ) - rule_match = re.search(r"\.mobile-close-btn\s*\{([^}]*)\}", block) - assert rule_match, ".mobile-close-btn rule body not found in 900px block" - assert "display:flex" in rule_match.group(1).replace(" ", ""), ( - ".mobile-close-btn should have display:flex in the 900px media query" + assert ".close-preview" not in block, ( + ".close-preview must not appear in @media(max-width:900px) block — " + "the single X button should be visible on all devices" ) -def test_close_preview_hidden_in_900px_block(): - """.close-preview must be display:none inside the 900px media query (fix for #781).""" +def test_mobile_close_btn_not_in_css(): + """mobile-close-btn CSS rules should have been removed entirely.""" css = _load_css() - block = _extract_media_block(css, _MEDIA_900_PATTERN) - assert ".close-preview" in block, ( - ".close-preview rule is missing from @media(max-width:900px) block — " - "the duplicate-button fix (#781) may have been reverted" - ) - rule_match = re.search(r"\.close-preview\s*\{([^}]*)\}", block) - assert rule_match, ".close-preview rule body not found in 900px block" - assert "display:none" in rule_match.group(1).replace(" ", ""), ( - ".close-preview should have display:none in the 900px media query to hide " - "the duplicate desktop X button at mobile widths" + assert ".mobile-close-btn" not in css, ( + ".mobile-close-btn CSS rule still present — the element was removed " + "from HTML so its styles should be cleaned up too" ) -def test_both_rules_in_same_media_block(): - """Both .close-preview and .mobile-close-btn must appear in the same 900px block.""" +def test_close_preview_visible_in_base_css(): + """Outside media queries, .close-preview must NOT be display:none.""" css = _load_css() - block = _extract_media_block(css, _MEDIA_900_PATTERN) - assert ".mobile-close-btn" in block, ( - ".mobile-close-btn missing from @media(max-width:900px) block" - ) - assert ".close-preview" in block, ( - ".close-preview missing from @media(max-width:900px) block" - ) - - -def test_close_preview_visible_outside_media_query(): - """Outside the media query, .close-preview must NOT be display:none - (it should remain visible on desktop).""" - css = _load_css() - base_css = _strip_media_blocks(css) - close_rules = re.findall(r"\.close-preview\s*\{([^}]*)\}", base_css) - for rule_body in close_rules: - assert "display:none" not in rule_body.replace(" ", ""), ( - ".close-preview must not be hidden in base (desktop) CSS" + # Simple check: find all .close-preview rules and ensure none set display:none + for m in re.finditer(r"\.close-preview\s*\{([^}]*)\}", css): + assert "display:none" not in m.group(1).replace(" ", ""), ( + ".close-preview must not be hidden by any CSS rule" ) diff --git a/tests/test_mobile_layout.py b/tests/test_mobile_layout.py index 8c8321f5..46ac1972 100644 --- a/tests/test_mobile_layout.py +++ b/tests/test_mobile_layout.py @@ -207,8 +207,6 @@ def test_rightpanel_mobile_slide_over_css(): "open mobile rightpanel should keep the edge shadow" assert re.search(r'\.rightpanel\s+\.panel-header\{[^}]*row-gap:\s*8px', rightpanel_block), \ "mobile workspace header should keep comfortable row spacing" - assert re.search(r'\.rightpanel\s+\.mobile-close-btn\{[^}]*margin-left:\s*auto', rightpanel_block), \ - "mobile workspace close button should align to the far right" def test_workspace_panel_inline_width_is_desktop_only(): diff --git a/tests/test_sprint41.py b/tests/test_sprint41.py index da757cf9..eee0898c 100644 --- a/tests/test_sprint41.py +++ b/tests/test_sprint41.py @@ -1,15 +1,11 @@ """ -Sprint 41 Tests: Title auto-generation fix + mobile close button CSS (PR #333). +Sprint 41 Tests: Title auto-generation fix (PR #333). Covers: - streaming.py: sessions titled 'New Chat' trigger auto-title generation - streaming.py: sessions with empty/falsy title trigger auto-title generation - streaming.py: sessions titled 'Untitled' (original guard) still trigger - streaming.py: sessions with a user-set title do NOT trigger auto-title -- style.css: .mobile-close-btn is hidden by default (desktop rule present) -- style.css: .mobile-close-btn shown in <=900px media query -- style.css: #btnCollapseWorkspacePanel hidden in <=900px media query -- index.html: both .mobile-close-btn and #btnCollapseWorkspacePanel buttons exist """ import pathlib import re @@ -59,57 +55,6 @@ class TestTitleAutoGenerationCondition(unittest.TestCase): "Expected at least 3 OR-joined sub-conditions (Untitled, New Chat, not s.title)") -# ── style.css: mobile close button visibility ───────────────────────────── - -class TestMobileCloseButtonCSS(unittest.TestCase): - """Verify CSS rules that control the duplicate close button on mobile.""" - - def test_mobile_close_btn_hidden_by_default(self): - """Desktop default: .mobile-close-btn must be display:none outside any media query.""" - # Find the rule before the first @media block that contains mobile-close-btn - # We look for the pattern in the desktop (non-media-query) section - self.assertIn( - ".mobile-close-btn{display:none;}", - CSS.replace(" ", ""), - ".mobile-close-btn should be hidden by default (desktop) — rule missing or wrong" - ) - - def test_mobile_close_btn_shown_in_900px_query(self): - """Inside max-width:900px media query, .mobile-close-btn must be display:flex.""" - # Extract the 900px media block - m = re.search(r'@media\s*\(max-width\s*:\s*900px\)\s*\{([^{}]*(?:\{[^{}]*\}[^{}]*)*)\}', - CSS) - self.assertIsNotNone(m, "@media(max-width:900px) block not found in style.css") - block = m.group(1).replace(" ", "") - self.assertIn(".mobile-close-btn{display:flex;}", - block, - ".mobile-close-btn must be display:flex inside the 900px media query") - - def test_900px_query_retains_existing_rules(self): - """Ensure the PR didn't accidentally drop existing rules from the 900px block.""" - m = re.search(r'@media\s*\(max-width\s*:\s*900px\)\s*\{([^{}]*(?:\{[^{}]*\}[^{}]*)*)\}', - CSS) - self.assertIsNotNone(m) - block = m.group(1) - self.assertIn("rightpanel", block, ".rightpanel rule missing from 900px block") - self.assertIn("mobile-files-btn", block, ".mobile-files-btn rule missing from 900px block") - - -# ── index.html: button presence ─────────────────────────────────────────── - -class TestWorkspacePanelButtons(unittest.TestCase): - """Verify the workspace panel close control remains present.""" - - def test_mobile_close_button_exists(self): - self.assertIn("mobile-close-btn", HTML, - ".mobile-close-btn button must exist in index.html") - - def test_mobile_close_button_has_aria_label(self): - """Accessibility: mobile close button must have an aria-label.""" - m = re.search(r'class="[^"]*mobile-close-btn[^"]*"[^>]*>', HTML) - self.assertIsNotNone(m, "Could not find mobile-close-btn element") - self.assertIn("aria-label", m.group(0), - "mobile-close-btn must have aria-label for accessibility") class TestIssue495TitleStreaming(unittest.TestCase): diff --git a/tests/test_sprint44.py b/tests/test_sprint44.py index 63e6e3fc..91001096 100644 --- a/tests/test_sprint44.py +++ b/tests/test_sprint44.py @@ -1,15 +1,13 @@ """ -Sprint 44 Tests: Workspace panel close button fixes (PR #413). +Sprint 44 Tests: Workspace panel close button (PR #413). Covers: -- index.html: mobile-close-btn now calls handleWorkspaceClose() instead of - closeWorkspacePanel(), so hitting X while a file is open returns you to the - file browser rather than collapsing the whole panel. -- boot.js: syncWorkspacePanelUI() hides #btnClearPreview (the X icon) on - desktop when no file preview is open, eliminating the duplicate X that - appeared alongside the chevron collapse button. -- boot.js: handleWorkspaceClose() logic — clears preview when one is visible, - closes panel otherwise (existing function, confirmed wired to both buttons). +- btnClearPreview is the single close button for all devices. Clicking it calls + handleWorkspaceClose(), which clears a file preview if open, or closes the + entire workspace panel otherwise. +- The mobile-close-btn element was removed — only one X button remains. +- Tooltip text updated to "Close" (when no preview) and "Close preview" + (when a file is being viewed). """ import pathlib import re @@ -20,105 +18,79 @@ HTML = (REPO / "static" / "index.html").read_text(encoding="utf-8") BOOT_JS = (REPO / "static" / "boot.js").read_text(encoding="utf-8") -class TestMobileCloseButtonBehavior(unittest.TestCase): - """mobile-close-btn must call handleWorkspaceClose(), not closeWorkspacePanel().""" +class TestSingleCloseButton(unittest.TestCase): + """btnClearPreview is the only close button, visible on all devices.""" - def test_mobile_close_btn_calls_handle_workspace_close(self): - """mobile-close-btn onclick must be handleWorkspaceClose(), not closeWorkspacePanel().""" - m = re.search(r'class="[^"]*mobile-close-btn[^"]*"[^>]*>', HTML) - self.assertIsNotNone(m, "mobile-close-btn element not found in index.html") - btn_html = m.group(0) - self.assertIn( - 'onclick="handleWorkspaceClose()"', - btn_html, - "mobile-close-btn must call handleWorkspaceClose() so that hitting X " - "while a file is open closes the file first, not the whole panel", - ) - - def test_mobile_close_btn_does_not_call_close_workspace_panel_directly(self): - """mobile-close-btn must NOT call closeWorkspacePanel() directly.""" - m = re.search(r'class="[^"]*mobile-close-btn[^"]*"[^>]*>', HTML) - self.assertIsNotNone(m, "mobile-close-btn element not found in index.html") - btn_html = m.group(0) - self.assertNotIn( - 'onclick="closeWorkspacePanel()"', - btn_html, - "mobile-close-btn must not call closeWorkspacePanel() directly — " - "it would bypass the two-step close logic and collapse the panel even " - "when a file is being viewed", - ) - - def test_handle_workspace_close_defined_in_boot_js(self): - """handleWorkspaceClose() must be defined in boot.js.""" - self.assertIn( - "function handleWorkspaceClose()", - BOOT_JS, - "handleWorkspaceClose() is missing from boot.js", - ) - - def test_handle_workspace_close_clears_preview_first(self): - """handleWorkspaceClose() must call clearPreview() when a preview is visible.""" - # The function must check for visible preview and call clearPreview - self.assertIn( - "clearPreview()", - BOOT_JS, - "handleWorkspaceClose() must call clearPreview() when preview is visible", - ) - def test_handle_workspace_close_falls_back_to_close_panel(self): - """handleWorkspaceClose() must call closeWorkspacePanel() as fallback.""" - # Find the function start and extract until the closing brace by scanning - start = BOOT_JS.find("function handleWorkspaceClose()") - self.assertNotEqual(start, -1, "handleWorkspaceClose() not found in boot.js") - # Extract a generous window after the function start - fn_window = BOOT_JS[start : start + 400] - self.assertIn( - "closeWorkspacePanel()", - fn_window, - "handleWorkspaceClose() must call closeWorkspacePanel() as its fallback path", - ) - - -class TestDesktopNoDuplicateXButton(unittest.TestCase): - """On desktop, only one X/close control should appear at a time.""" - - def test_sync_workspace_panel_ui_hides_clear_preview_on_desktop(self): - """syncWorkspacePanelUI() must set display:none on btnClearPreview when no preview and desktop.""" - self.assertIn( - "clearBtn.style.display", - BOOT_JS, - "syncWorkspacePanelUI() must control clearBtn.style.display to hide it " - "on desktop when no file preview is open", - ) - - def test_clear_preview_hidden_when_no_preview(self): - """The display toggle for btnClearPreview must key off hasPreview.""" - # Expect something like: clearBtn.style.display=hasPreview?'':'none' - # or clearBtn.style.display = hasPreview ? '' : 'none' - pattern = r"clearBtn\.style\.display\s*=\s*hasPreview" - self.assertRegex( - BOOT_JS, - pattern, - "btnClearPreview display must be conditioned on hasPreview in " - "syncWorkspacePanelUI() to avoid a duplicate X on desktop", - ) - - def test_clear_preview_toggle_only_applied_on_desktop(self): - """The display toggle must be guarded by !isCompact so mobile is unaffected.""" - # Expect: if(!isCompact) clearBtn.style.display=... - pattern = r"isCompact.*clearBtn\.style\.display|clearBtn\.style\.display.*isCompact" - self.assertRegex( - BOOT_JS, - pattern, - "btnClearPreview display toggle must be guarded by isCompact so the " - "mobile X button visibility is not accidentally affected", - ) - - def test_btnclearpreview_exists_in_html(self): - """#btnClearPreview must still exist in the HTML (not removed).""" + def test_btn_clear_preview_exists_in_html(self): + """#btnClearPreview must exist in index.html.""" self.assertIn( 'id="btnClearPreview"', HTML, - "#btnClearPreview must remain in index.html", + "#btnClearPreview must be present in index.html", + ) + + def test_mobile_close_btn_removed_from_html(self): + """mobile-close-btn element should no longer exist in index.html.""" + self.assertNotIn( + "mobile-close-btn", + HTML, + "mobile-close-btn was removed — only btnClearPreview remains as close control", + ) + + def test_btn_clear_preview_wired_to_handle_workspace_close(self): + """btnClearPreview onclick must be handleWorkspaceClose.""" + self.assertRegex( + BOOT_JS, + r"btnClearPreview.*onclick\s*=\s*handleWorkspaceClose", + "btnClearPreview must call handleWorkspaceClose so that clicking X " + "either clears a preview or closes the panel", + ) + + +class TestHandleWorkspaceCloseLogic(unittest.TestCase): + """handleWorkspaceClose() clears preview first, falls back to close panel.""" + + def test_function_defined(self): + self.assertIn( + "function handleWorkspaceClose()", + BOOT_JS, + "handleWorkspaceClose() must exist in boot.js", + ) + + def test_clears_preview_when_open(self): + idx = BOOT_JS.find("function handleWorkspaceClose()") + self.assertGreater(idx, 0, "handleWorkspaceClose() not found") + body = BOOT_JS[idx:idx + 500] + self.assertIn( + "clearPreview()", + body, + "handleWorkspaceClose() must call clearPreview() when a preview is open", + ) + + def test_falls_back_to_close_panel(self): + idx = BOOT_JS.find("function handleWorkspaceClose()") + self.assertGreater(idx, 0, "handleWorkspaceClose() not found") + body = BOOT_JS[idx:idx + 500] + self.assertIn( + "closeWorkspacePanel()", + body, + "handleWorkspaceClose() must call closeWorkspacePanel() as fallback", + ) + + +class TestTooltipText(unittest.TestCase): + """The X button tooltip says 'Close' when no preview, 'Close preview' otherwise.""" + + def test_tooltip_uses_close(self): + """syncWorkspacePanelUI() sets tooltip to 'Close' (not 'Hide workspace panel').""" + idx = BOOT_JS.find("function syncWorkspacePanelUI()") + self.assertGreater(idx, 0, "syncWorkspacePanelUI() not found") + body = BOOT_JS[idx:idx + 2000] + # The tooltip line should contain 'Close' and NOT 'Hide workspace panel' + self.assertIn( + "'Close'", + body, + "btnClearPreview tooltip must use 'Close' instead of 'Hide workspace panel'", )