diff --git a/docs/superpowers/specs/2026-07-25-guide-popup-support-design.md b/docs/superpowers/specs/2026-07-25-guide-popup-support-design.md new file mode 100644 index 0000000..95a465d --- /dev/null +++ b/docs/superpowers/specs/2026-07-25-guide-popup-support-design.md @@ -0,0 +1,107 @@ +# Obsługa popupów w `guidebot guide` (PDF) + +Data: 2026-07-25 + +## Problem + +`guide` odrzuca każdy scenariusz, w którym jakiś krok otwiera nowe okno: + +``` +BŁĄD: scenariusze z popupem nie są obsługiwane w `guide` v1 (krok otwiera nowe okno) +``` + +Blokada siedzi w `guide/prolog.py` (`scan_for_blockers`) i wyzwala ją zamrożona +flaga `opens_popup: true`. Dotyczy to całej klasy scenariuszy, których sednem +jest logowanie — a więc dokładnie tych, dla których przewodnik krok-po-kroku ma +największy sens. + +Blokada nie jest kaprysem: `guide` nie ma **żadnego** cyklu życia okien. +`_Capture.page` jest ustawiane raz z okna głównego i każde z pięciu wywołań +`_screenshot(cap.page, …)` (`replay.py:216, 268, 328, 396, 403`) fotografuje +właśnie je. Krok działający na popupie szukałby swojego celu w oknie głównym +i padał z niezgodnością tożsamości — kilka kroków za późno, w niezrozumiałym +miejscu. Odrzucenie z góry było uczciwsze niż taka awaria. + +## Rozwiązanie + +Zamiast dokładać wykrywanie okien, zmieniamy jedno pojęcie: `_Capture.page` +przestaje znaczyć „okno główne", a zaczyna znaczyć **okno aktywne**. Pętla +przechwytywania nie dowiaduje się o tym niczego — te same pięć wywołań +`_screenshot(cap.page, …)` fotografuje inną wartość. + +### Cykl życia sterowany sidecarem + +Popupu nie wykrywamy heurystycznie. `compile` już zamraża `opens_popup: true` +(`recorder/compile/run.py:365`), więc: + +* krok `click` z tą flagą owija kliknięcie w `context.expect_page()` + i po powrocie przełącza aktywną parę `(page, recorder)` na nowe okno; +* krok `closeWindow` zamyka popup i przywraca parę okna głównego. + +Świadomie **nie** sięgamy po `render/popup_detect.py` ani `render/popup_crop.py` +(789 linii). To maszyneria wideo: wykrywanie momentu z kwantem ciszy, przejścia +float/slide, kadrowanie klatek. PDF nie ma osi czasu, więc nie ma czego kadrować. + +Popup dostaje własny `Recorder` — bez `frame=`, bo nie żyje w iframie powłoki +chrome. Pasek chrome na popupie zostaje sterowany istniejącym `cfg.popup.is_bare`, +które `run_guide` już przekazuje do `Chrome(...)`. + +`closeWindow` przestaje być komendą wyłącznie narracyjną. Dziś siedzi +w `NARRATION_ONLY_KINDS` z komentarzem „popup bookkeeping the guide already +rejects" — dostaje własny rodzaj strony i własną fazę, bo od teraz **wykonuje** +pracę w przeglądarce. + +### Płótno — w CSS, nie w obrazie + +Zrzut popupu zostaje w naturalnym rozmiarze okna. Wyśrodkowanie na płótnie +o proporcjach okna głównego robi `layout.py`: bez przetwarzania obrazu i bez +nowej zależności. + +Jeden haczyk, i to on decyduje o kształcie zmiany. Dziś SVG z adnotacjami leży +`inset: 0` nad `.shot`, a `img` wypełnia ten box — więc oba układy współrzędnych +się pokrywają. Gdy obrazek przestanie wypełniać `.shot`, adnotacje rozjadą się +o wielkość marginesu. Dlatego `img` i `svg` trafiają do wewnętrznego `.plate` +o rozmiarze obrazka, wyśrodkowanego w `.shot`. Dla stron nieletterboxowanych +`.plate` ma `width: 100%` i renderuje się identycznie jak dotąd. + +`GuidePage` dostaje `canvas_size` obok istniejącego `screenshot_size`. +Letterboxing włącza się wtedy i tylko wtedy, gdy oba są znane i różne — czyli +sam fakt „to jest popup" nie jest nigdzie zapisywany jako flaga. Rozmiar +wystarcza. + +## Obsługa błędów + +Popup, który nie pojawi się mimo `opens_popup: true`, to twardy `GuideError` +z `plik:linia` i fragmentem YAML — tak jak każdy inny niespełniony namiar +w `guide`. Bez cichego pomijania: zamrożona flaga mówi, że okno *ma* się otworzyć, +a PDF bez tej strony byłby przewodnikiem, który gubi połowę instrukcji. + +Wywołanie `enter_popup` bez wstrzykniętej fabryki `Recorder`-ów jest błędem +programisty, nie scenariusza, więc leci `RuntimeError`. + +## Testy + +Pakiet ma własny strażnik szwów (`tests/unit/guide/test_capture_seams.py`), +który wymaga, by nazwa patchowana na `capture` była wołana z `capture`. Nowy +przełącznik okien mieszka na `_Capture`, więc go nie dotyczy — ale testy nie +mogą go obchodzić name-importem w siostrzanym module. + +Nowe przypadki: + +* przejście na popup po kroku z `opens_popup` i powrót na `closeWindow`; +* zrzuty po przełączeniu pochodzą z popupu, nie z okna głównego; +* ślad kursora zeruje się na każdej zmianie okna (strzałka między oknami nie ma sensu); +* brak popupu mimo flagi → `GuideError` z `plik:linia`; +* `render_html` letterboksuje wtedy i tylko wtedy, gdy `canvas_size ≠ screenshot_size`, + a adnotacje pozostają w układzie obrazka; +* `scan_for_blockers` przestaje odrzucać popupy, nadal odrzuca nieznane komendy + i nierozwiązane kroki obowiązkowe. + +## Zakres + +Zmiana dotyczy wyłącznie `guidebot_recorder/guide/`. Nie ruszamy `render`, +przejść float/slide ani kadrowania wideo. + +Pliki: `prolog.py`, `replay.py`, `capture.py`, `model.py`, `layout.py`, `guide.py`. +Wszystkie zostają grubo poniżej limitu 600 linii; żadna funkcja nie zbliża się +do CC 10. diff --git a/guidebot_recorder/guide/capture.py b/guidebot_recorder/guide/capture.py index e5fc8fc..e267256 100644 --- a/guidebot_recorder/guide/capture.py +++ b/guidebot_recorder/guide/capture.py @@ -37,6 +37,7 @@ _approach_target, _Capture, _click_or_hover_frame, + _close_window_page, _gate_page, _navigate_page, _scroll_page, @@ -95,6 +96,7 @@ "text": _text_page, "scroll": _scroll_page, "wait": _wait_page, + "closeWindow": _close_window_page, } @@ -161,6 +163,7 @@ def _append_step_page(run: _StepRun) -> None: bounds=bounds, ), screenshot_size=run.size, + canvas_size=cap.canvas, ) ) @@ -181,6 +184,11 @@ async def _action_page(run: _StepRun) -> None: return await cap.recorder.apply_readiness(run.action.expect) _append_step_page(run) + # Last, and the order is the point: this step's page shows the window the + # reader clicked *in*, photographed before the click. Only once that page is + # built does the popup become the window the following steps are read against. + if run.popup is not None: + cap.enter_popup(run.popup) async def capture_pages( @@ -195,6 +203,7 @@ async def capture_pages( pause_on_error: bool = False, sensitive_values: Iterable[str] = (), selects: Selects | None = None, + make_recorder: Callable[[Page], Recorder] | None = None, ) -> list[GuidePage]: """Replay the compiled scenario, keeping one annotated frame per step. @@ -203,6 +212,11 @@ async def capture_pages( — the guide's half of the readiness barrier compile and render also take. See :meth:`~guidebot_recorder.guide.replay._Capture.await_selects_ready` for where it is taken and why. + + ``make_recorder`` builds the ``Recorder`` for a popup window; omit it for a + scenario that never opens one. It is injected rather than built here because a + ``Recorder`` needs the caller's ``Overlay`` — see + :meth:`~guidebot_recorder.guide.replay._Capture.enter_popup`. """ cap = _Capture( @@ -216,6 +230,7 @@ async def capture_pages( pause_on_error=pause_on_error, sensitive_values=sensitive_values, selects=selects, + make_recorder=make_recorder, ) # Wrapping the iterator (rather than `bar.update(1)` as render does) is what # keeps the count honest here: this loop leaves through a dozen early returns @@ -239,7 +254,10 @@ async def capture_pages( except Exception as exc: if pause_on_error: await pause_for_inspection( - page, + # The window that failed, not the one the run started in. + # A step inside a popup that leaves the main window paused + # shows the developer a page with nothing wrong on it. + cap.page, "guide", index, run.kind, diff --git a/guidebot_recorder/guide/guide.py b/guidebot_recorder/guide/guide.py index 2b90e45..79af455 100644 --- a/guidebot_recorder/guide/guide.py +++ b/guidebot_recorder/guide/guide.py @@ -4,12 +4,12 @@ from pathlib import Path -from playwright.async_api import Browser, Frame +from playwright.async_api import Browser, Frame, Page from guidebot_recorder.chrome import SHELL_URL, Chrome from guidebot_recorder.chrome.framing import install_framing from guidebot_recorder.guide.capture import capture_pages -from guidebot_recorder.guide.layout import render_html +from guidebot_recorder.guide.layout import fold_narration, render_html from guidebot_recorder.guide.pdf import html_to_pdf from guidebot_recorder.guide.prolog import GuideError, scan_for_blockers from guidebot_recorder.overlay.overlay import Overlay @@ -92,6 +92,17 @@ async def run_guide( # otherwise the recorder drives the page directly (frame=None -> page). recorder = Recorder(page, overlay, frame=site_frame, type_delay_ms=None) + def make_popup_recorder(popup: Page) -> Recorder: + """The popup's recorder — deliberately without a ``frame``. + + The shell iframe belongs to the main window only; a popup is its own + top-level document, so the recorder drives the page directly. Its + chrome bar, if any, is the legacy in-DOM one that ``chrome.js`` mounts + on popup-site documents under ``bare_popups=False``. + """ + + return Recorder(popup, overlay, frame=None, type_delay_ms=None) + shots_dir = out_pdf.parent / (out_pdf.stem + "_shots") pages = await capture_pages( scenario, @@ -104,10 +115,15 @@ async def run_guide( pause_on_error=pause_on_error, sensitive_values=sensitive_values, selects=selects, + make_recorder=make_popup_recorder, ) finally: await context.close() - html = render_html(pages, title=cfg.title) + # Fold before rendering *and* before counting: narration with no still of its + # own rides on the previous picture, so it stops being a page. Counting the + # unfolded list here is what made `guide` report eight pages of a five-page PDF. + sheets = fold_narration(pages) + html = render_html(sheets, title=cfg.title) await html_to_pdf(browser, html, out_pdf) - return len(pages) + return len(sheets) diff --git a/guidebot_recorder/guide/layout.py b/guidebot_recorder/guide/layout.py index 7729d20..f7fd575 100644 --- a/guidebot_recorder/guide/layout.py +++ b/guidebot_recorder/guide/layout.py @@ -4,6 +4,7 @@ import html import math +from dataclasses import replace from pathlib import Path from guidebot_recorder.guide.model import Annotation, GuidePage @@ -16,12 +17,22 @@ page-break-after: always; align-items: start; } .page:last-child { page-break-after: auto; } .shot { position: relative; width: 100%; border: 1px solid #ddd; border-radius: 6px; - overflow: hidden; } -.shot img { width: 100%; display: block; } -.shot svg { position: absolute; inset: 0; width: 100%; height: 100%; } + overflow: hidden; display: flex; align-items: center; justify-content: center; + background: #eceef2; } +/* The still and its annotation layer share one box, so the SVG's viewBox + coordinates and the image's pixels line up. That box is `.plate`, not `.shot`: + a popup still does not fill `.shot`, and an overlay pinned to `.shot` would sit + off the picture by the width of the letterbox margin. At `width: 100%` (every + main-window page) the two boxes coincide and this renders as it always did. */ +.plate { position: relative; width: 100%; } +.plate img { width: 100%; display: block; } +.plate svg { position: absolute; inset: 0; width: 100%; height: 100%; } .side { padding-top: 2mm; } .side .heading { font-size: 20px; font-weight: 700; margin: 0 0 4mm; } .side .body { font-size: 16px; line-height: 1.5; white-space: pre-wrap; } +/* Separates this step's own description from narration folded in from steps + that had nothing to photograph — see `_fold_narration`. */ +.side hr.fold { border: 0; border-top: 1px solid #d6d6d6; margin: 4mm 0; } .slide { grid-column: 1 / -1; display: flex; flex-direction: column; justify-content: center; height: 100vh; text-align: center; } .slide .title { font-size: 40px; font-weight: 800; } @@ -91,15 +102,53 @@ def _svg(anns: list[Annotation], size: tuple[int, int]) -> str: return "".join(parts) +def _letterbox(page: GuidePage) -> tuple[str, str]: + """Inline styles that centre a smaller still on the page's canvas. + + Returns ``(shot_style, plate_style)``, both empty when the still already fills + the canvas — which is every main-window page, so the common case adds no + markup at all. A popup opens at its own size, and the two sizes differing is + the only signal that it is one (see :class:`GuidePage`). + + ``.shot`` gets the canvas aspect ratio rather than a pixel height so it keeps + scaling with the print column, and ``.plate`` is sized as a percentage of it — + the same ratio the still has to the canvas, so the picture lands unscaled + relative to its neighbours and a 600px-wide popup reads as narrower than the + 1376px window it came from, which is the honest depiction. + """ + + shot, canvas = page.screenshot_size, page.canvas_size + if not shot or not canvas or shot == canvas or not all(canvas): + return "", "" + width = round(100 * shot[0] / canvas[0], 3) + height = round(100 * shot[1] / canvas[1], 3) + return ( + f' style="aspect-ratio:{canvas[0]}/{canvas[1]}"', + f' style="width:{width}%;height:{height}%"', + ) + + +def _side(page: GuidePage) -> str: + """The right-hand column: this step's description, then anything folded in.""" + + heading = f'
{html.escape(page.heading)}
' if page.heading else "" + body = f'
{html.escape(page.text)}
' if page.text else "" + folded = "".join( + f'
{html.escape(text)}
' + for text in page.folded_text + ) + return f"{heading}{body}{folded}" + + def _shot_page(page: GuidePage) -> str: uri = Path(page.screenshot).absolute().as_uri() svg = _svg(page.annotations, page.screenshot_size or (1, 1)) - heading = f'
{html.escape(page.heading)}
' if page.heading else "" - body = f'
{html.escape(page.text)}
' if page.text else "" + shot_style, plate_style = _letterbox(page) return ( '
' - f'
{svg}
' - f'
{heading}{body}
' + f'
' + f'
{svg}
' + f'
{_side(page)}
' "
" ) @@ -115,14 +164,72 @@ def _slide_page(page: GuidePage) -> str: def _text_page(page: GuidePage) -> str: - heading = f'
{html.escape(page.heading)}
' if page.heading else "" + """Narration that found no picture to join — full width, still its own sheet. + + Reached only when :func:`_fold_narration` declined to fold it: nothing came + before it, or what came before was a slide. + """ + return ( '
' - f'{heading}
{html.escape(page.text)}
' + f"{_side(page)}" + ) + + +def _folds_into_previous(page: GuidePage, previous: GuidePage | None) -> bool: + """Whether ``page`` is narration that belongs on the picture before it. + + Three conditions, and each excludes a page that must keep its own sheet: a + narration page with nothing before it has no picture to join; a page with a + still of its own is the thing being described, not a description; and a slide + is a deliberate full-page interstitial, so it neither absorbs nor is absorbed. + """ + + return ( + previous is not None + and previous.screenshot is not None + and page.screenshot is None + and page.kind != "slide" ) +def fold_narration(pages: list[GuidePage]) -> list[GuidePage]: + """Move picture-less narration onto the page of the still it was spoken over. + + A `say:` between two actions describes what the reader is looking at — it did + not earn a sheet of paper with a sentence marooned on it. Folding happens here + and not in the capture pass on purpose: one step still produces one + :class:`GuidePage`, and pagination is the document's question alone. + + Empty narration folds to nothing rather than to a bare rule, which is also how + a text page that never had any text stops printing a blank sheet. + + **Public, and called by ``run_guide`` rather than by :func:`render_html`.** + Folding inside the renderer would leave the caller holding the *unfolded* list + as its only measure of the document — and `guide` reports that length as the + page count, so the CLI would cheerfully announce eight pages of a five-page + PDF. One list, counted and rendered, is the only shape that cannot drift. + """ + + folded: list[GuidePage] = [] + for page in pages: + previous = folded[-1] if folded else None + if not _folds_into_previous(page, previous): + folded.append(page) + continue + carried = [*previous.folded_text, page.text] if page.text else previous.folded_text + folded[-1] = replace(previous, folded_text=carried) + return folded + + def render_html(pages: list[GuidePage], *, title: str) -> str: + """Render exactly the pages given — one ``
`` each. + + Does **not** fold; run :func:`fold_narration` first if you want that. Keeping + the renderer free of pagination is what lets the caller count the same list it + prints. + """ + body_parts: list[str] = [] for page in pages: if page.kind == "slide": diff --git a/guidebot_recorder/guide/model.py b/guidebot_recorder/guide/model.py index 5804332..4e59ce3 100644 --- a/guidebot_recorder/guide/model.py +++ b/guidebot_recorder/guide/model.py @@ -38,7 +38,23 @@ class Annotation: @dataclass class GuidePage: - """One PDF page: a screenshot (or none) plus its description and annotations.""" + """One PDF page: a screenshot (or none) plus its description and annotations. + + ``screenshot_size`` is the still's own pixel size — the coordinate system every + :class:`Annotation` on this page is expressed in. ``canvas_size`` is the size + the page is *presented* at, which for the main window is the same thing and for + a popup window is not: a popup opens at whatever size the site asked for. + + A popup page is not flagged as one anywhere; the two sizes differing IS the + flag. That is deliberate — a separate boolean could disagree with the sizes, + and then the layout would have two sources of truth for one question. + + ``folded_text`` is narration from later steps that had nothing of their own to + photograph, and so rides along in this page's side panel under a rule. It is + filled by the layout, never by the capture pass: one step still produces one + :class:`GuidePage`, and how those pages are composed onto sheets of paper is + a question only the document asks. + """ kind: Literal["step", "navigate", "slide", "text"] screenshot: Path | None @@ -46,6 +62,8 @@ class GuidePage: heading: str | None annotations: list[Annotation] = field(default_factory=list) screenshot_size: tuple[int, int] | None = None + canvas_size: tuple[int, int] | None = None + folded_text: list[str] = field(default_factory=list) def page_text(step: Step) -> str: diff --git a/guidebot_recorder/guide/prolog.py b/guidebot_recorder/guide/prolog.py index 7d4dff1..537fce3 100644 --- a/guidebot_recorder/guide/prolog.py +++ b/guidebot_recorder/guide/prolog.py @@ -9,22 +9,27 @@ class GuideError(Exception): - """A scenario the guide cannot render (popup, unresolved mandatory step).""" + """A scenario the guide cannot render (unresolved mandatory step, broken sidecar).""" -PageKind = Literal["gate", "navigate", "slide", "action", "scroll", "text", "wait"] +PageKind = Literal["gate", "navigate", "slide", "action", "scroll", "text", "wait", "closeWindow"] #: Commands the guide replays in the browser off a frozen target. `highlight` is #: one of them even though it performs nothing: the guide still has to resolve the #: target to know where to draw its ellipse. ACTION_KINDS = frozenset({"click", "hover", "enterText", "teach", "select", "highlight"}) #: Commands with no browser work in a still-image pass: a film-only flourish -#: (`desktop`), popup bookkeeping the guide already rejects (`closeWindow`), -#: and a bare `say`. All that survives into the PDF is their narration. -NARRATION_ONLY_KINDS = frozenset({"desktop", "closeWindow", "say"}) +#: (`desktop`) and a bare `say`. All that survives into the PDF is their narration. +#: +#: `closeWindow` used to sit here, back when a popup was rejected outright and the +#: command therefore had nothing to close. It now drives the window switch back to +#: the main page, so it is browser work and has a page kind of its own. +NARRATION_ONLY_KINDS = frozenset({"desktop", "say"}) #: Every command `classify` knows how to place. A command outside this set is a #: gap in the guide, not a no-op — see :func:`scan_for_blockers`. -SUPPORTED_KINDS = ACTION_KINDS | NARRATION_ONLY_KINDS | {"navigate", "slide", "scroll", "wait"} +SUPPORTED_KINDS = ( + ACTION_KINDS | NARRATION_ONLY_KINDS | {"navigate", "slide", "scroll", "wait", "closeWindow"} +) def classify(flat_step: FlatStep) -> PageKind: @@ -38,6 +43,8 @@ def classify(flat_step: FlatStep) -> PageKind: return "slide" if kind == "scroll": return "scroll" + if kind == "closeWindow": + return "closeWindow" if kind in ACTION_KINDS: return "action" if kind == "wait": @@ -47,7 +54,7 @@ def classify(flat_step: FlatStep) -> PageKind: def scan_for_blockers(flat: list[FlatStep], actions: list) -> None: - """Raise GuideError for popups, unsupported commands, or a mandatory unresolved step.""" + """Raise GuideError for unsupported commands or a mandatory unresolved step.""" for flat_step, action in zip(flat, actions, strict=True): kind = flat_step.step.command_kind() @@ -58,10 +65,6 @@ def scan_for_blockers(flat: list[FlatStep], actions: list) -> None: # browser never saw the action, and the failure surfaced steps later # as an identity mismatch against a page the compiler never saw. raise GuideError(f"komenda `{kind}` nie jest obsługiwana w `guide`") - if isinstance(action, CachedAction) and action.opens_popup: - raise GuideError( - "scenariusze z popupem nie są obsługiwane w `guide` v1 (krok otwiera nowe okno)" - ) pending = action is not None and not isinstance(action, CachedAction) mandatory = ( flat_step.branch is None diff --git a/guidebot_recorder/guide/replay.py b/guidebot_recorder/guide/replay.py index fbfe922..c139b3a 100644 --- a/guidebot_recorder/guide/replay.py +++ b/guidebot_recorder/guide/replay.py @@ -23,6 +23,7 @@ from playwright.async_api import Error as PlaywrightError from playwright.async_api import Page +from playwright.async_api import TimeoutError as PlaywrightTimeoutError from tqdm import tqdm from guidebot_recorder.diagnostics import step_banner @@ -65,8 +66,14 @@ class _Capture: """What outlives one step of the capture pass. :attr:`pages` is the document being built, :attr:`skipped_branch` the gate - that turned out to be absent, and :attr:`trail` the cursor memory. The rest - is settled before the loop starts and only read. + that turned out to be absent, and :attr:`trail` the cursor memory. + + :attr:`page` and :attr:`recorder` are the **active window**, not the main one. + A step that opens a popup swaps both (:meth:`enter_popup`) and ``closeWindow`` + swaps them back (:meth:`leave_popup`); every ``_screenshot(cap.page, ...)`` in + this module photographs whichever window is active at the time, which is the + whole of the popup support. The main pair is kept aside in ``__post_init__`` + so leaving never has to reconstruct it. """ scenario: object @@ -79,9 +86,62 @@ class _Capture: pause_on_error: bool sensitive_values: Iterable[str] selects: Selects | None + #: Builds the popup's own ``Recorder``. Injected rather than constructed here + #: because a ``Recorder`` needs the ``Overlay``, which belongs to the caller's + #: browser context — and because a test can hand over a fake without a browser. + #: ``None`` in a capture that never meets a popup. + make_recorder: Callable[[Page], Recorder] | None = None pages: list[GuidePage] = field(default_factory=list) trail: _CursorTrail = field(default_factory=_CursorTrail) skipped_branch: int | None = None + popup: Page | None = None + + def __post_init__(self) -> None: + self._main_page = self.page + self._main_recorder = self.recorder + + @property + def canvas(self) -> tuple[int, int]: + """The size every page is presented at — the main window's viewport. + + A popup still is smaller than this and gets centred on it by the layout; + see :func:`~guidebot_recorder.guide.layout._letterbox`. + """ + + viewport = self.scenario.config.viewport + return (viewport.width, viewport.height) + + def enter_popup(self, popup: Page) -> None: + """Make ``popup`` the active window for the steps that follow.""" + + if self.make_recorder is None: + raise RuntimeError( + "guide nie ma fabryki Recordera, a krok otworzył popup — " + "to błąd wywołania `capture_pages`, nie scenariusza" + ) + self.popup = popup + self.page = popup + self.recorder = self.make_recorder(popup) + # A new window is a new coordinate system: an arrow drawn from the last + # target in the other window would point at nothing on this picture. + self.trail.reset() + + async def leave_popup(self) -> None: + """Close the popup, if one is open, and go back to the main window. + + A no-op when no popup is open: `closeWindow` in a scenario that never + opened one is bookkeeping the author wrote for the film, and the guide has + nothing to undo. + """ + + if self.popup is None: + return + if not self.popup.is_closed(): + await self.popup.close() + self.popup = None + self.page = self._main_page + self.recorder = self._main_recorder + self.trail.reset() def banner(self, entry: FlatStep, entry_index: int, message: str) -> str: """Komunikat kroku z `plik:linia` i fragmentem YAML; sekrety zredagowane.""" @@ -170,6 +230,11 @@ class _StepRun: row_box: dict | None = None row_center: _Point | None = None mark: object = None + #: The window this step's click opened, filled by :func:`_click_or_hover_frame` + #: for a frozen ``opens_popup`` click. The switch itself happens one phase + #: later, in ``capture._action_page``: this step's own page still belongs to + #: the window the reader was looking at when they clicked. + popup: Page | None = None @property def step(self) -> Step: @@ -222,6 +287,7 @@ async def _navigate_page(run: _StepRun) -> None: heading=f"Otwórz adres: {url}", annotations=[], screenshot_size=size, + canvas_size=cap.canvas, ) ) cap.trail.reset() @@ -274,6 +340,7 @@ async def _scroll_page(run: _StepRun) -> None: heading=None, annotations=[], screenshot_size=size, + canvas_size=cap.canvas, ) ) @@ -397,17 +464,67 @@ async def _highlight_frame(run: _StepRun) -> bool: return True +def _opens_popup(action: CompiledAction | None) -> bool: + """Whether ``compile`` froze this action as the one that opens a new window.""" + + return isinstance(action, CachedAction) and action.opens_popup + + +async def _click_into_popup(run: _StepRun) -> Page: + """Click, and hand back the window that click opened. + + The wait wraps the click rather than following it: a popup that opens fast + would have fired its event before a bare ``wait_for_event`` started listening, + and the guide would then blame the site for its own race. + """ + + cap = run.capture + try: + async with cap.page.context.expect_page(timeout=cap.timeout * 1000) as opened: + await run.locator.click() + popup = await opened.value + except PlaywrightTimeoutError as exc: + # The frozen flag says a window opens here, so its absence is a broken + # guide, not a branch to skip: a PDF missing the popup's steps would read + # as a complete set of instructions while omitting half of them. + raise GuideError( + run.banner("krok miał otworzyć nowe okno, a żadne się nie pojawiło") + ) from exc + await popup.wait_for_load_state("domcontentloaded") + return popup + + async def _click_or_hover_frame(run: _StepRun) -> bool: cap = run.capture # frame BEFORE click/hover run.shot, run.size = await _screenshot(cap.page, cap.shots_dir, run.index) if run.act == "hover": await run.locator.hover() + elif _opens_popup(run.action): + run.popup = await _click_into_popup(run) else: await run.locator.click() return True +async def _close_window_page(run: _StepRun) -> None: + """Close the popup, go back to the main window, and narrate if the step does. + + No screenshot of its own: the closing is not a thing the reader does, it is + the film's way of getting back — and the next step photographs the main window + anyway. + """ + + cap = run.capture + await cap.leave_popup() + text = page_text(run.step) + if not text: + return + cap.pages.append( + GuidePage(kind="text", screenshot=None, text=text, heading=None, annotations=[]) + ) + + #: How each frozen action gets photographed. `click` and `hover` share the #: default because they differ only in the call that follows the frame. _FRAMES: dict[str, Callable[[_StepRun], Awaitable[bool]]] = { diff --git a/tests/unit/guide/test_capture_popup.py b/tests/unit/guide/test_capture_popup.py new file mode 100644 index 0000000..0f9fdc3 --- /dev/null +++ b/tests/unit/guide/test_capture_popup.py @@ -0,0 +1,332 @@ +"""Popup windows in the PDF guide: the active window switches, and switches back. + +Driven with fakes, no browser. The fakes here stand in for the one Playwright API +the feature leans on — ``BrowserContext.expect_page`` — and stay in this file +rather than travelling to ``_capture_helpers.py`` because nothing else uses them. + +What is actually being pinned is an *ordering*: the step that opens a popup is +photographed in the window the reader clicked in, and only the steps after it are +read against the popup. Get that backwards and every guide with a login flow +shows the reader a picture of the wrong window at the moment they act. +""" + +from __future__ import annotations + +import pytest +from playwright.async_api import TimeoutError as PlaywrightTimeoutError + +import guidebot_recorder.guide.capture as capture +from guidebot_recorder.guide.capture import capture_pages +from guidebot_recorder.guide.prolog import GuideError +from guidebot_recorder.models.action import CachedAction +from guidebot_recorder.models.scenario import Scenario, Step + +from ._capture_helpers import ( + FakeRecorder, + _async_none, + _cfg, + _compiled, + _fp, + _target, +) + + +class _Opened: + """Stand-in for the object ``expect_page()`` yields. + + ``None`` for the popup means the event never fired, and the refusal is raised + from ``__aexit__`` — which is where Playwright raises it too, so the + production code's ``try`` has to span the whole ``async with``, not just the + ``await opened.value`` after it. + """ + + def __init__(self, popup): + self._popup = popup + + async def __aenter__(self): + return self + + async def __aexit__(self, *_exc): + if self._popup is None: + raise PlaywrightTimeoutError('Timeout exceeded while waiting for event "page"') + return False + + @property + def value(self): + return self._resolve() + + async def _resolve(self): + return self._popup + + +class FakeContext: + def __init__(self, popup): + self.popup = popup + self.expect_timeouts: list[float | None] = [] + + def expect_page(self, timeout=None): + self.expect_timeouts.append(timeout) + return _Opened(self.popup) + + +class NamedPage: + """A page that says which window a screenshot came from.""" + + def __init__(self, name: str, events: list[str], size=(1280, 720), context=None): + self.name = name + self.events = events + self.viewport_size = {"width": size[0], "height": size[1]} + self.context = context + self.closed = False + + async def screenshot(self, path): + from pathlib import Path + + Path(path).write_bytes(b"fake") + self.events.append(f"shot:{self.name}") + + def is_closed(self): + return self.closed + + async def close(self): + self.closed = True + self.events.append(f"close:{self.name}") + + async def wait_for_load_state(self, _state): + return None + + +def _click(opens_popup=False): + return CachedAction( + action="click", + target=_target(), + expect="none", + opens_popup=opens_popup, + fingerprint=_fp(command_kind="click"), + ) + + +def _windows(events, popup_size=(600, 700)): + """Main window plus the popup its context will hand over.""" + + popup = NamedPage("popup", events, size=popup_size) + main = NamedPage("main", events, context=FakeContext(popup)) + popup.context = main.context + return main, popup + + +async def test_popup_click_switches_the_active_window_and_close_switches_back( + tmp_path, monkeypatch +): + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, popup = _windows(events) + scenario = Scenario( + config=_cfg(), + steps=[ + Step(click="ikona profilu"), + Step(click="pole w wyskakującym oknie"), + Step(close_window=True, say="wracamy do serwisu"), + Step(click="menu w oknie głównym"), + ], + ) + popup_recorders: list[FakeRecorder] = [] + + def make_recorder(page): + assert page is popup + recorder = FakeRecorder(events) + popup_recorders.append(recorder) + return recorder + + await capture_pages( + scenario, + _compiled([_click(opens_popup=True), _click(), None, _click()]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + make_recorder=make_recorder, + ) + + # The click that opens the popup is photographed in the MAIN window: that is + # what the reader is looking at when they perform it. + assert [e for e in events if e.startswith(("shot:", "close:"))] == [ + "shot:main", + "shot:popup", + "close:popup", + "shot:main", + ] + assert len(popup_recorders) == 1 + assert popup.closed is True + + +async def test_popup_page_keeps_its_own_size_and_the_main_windows_canvas(tmp_path, monkeypatch): + """Zrzut popupu zostaje w swoim rozmiarze; płótno pozostaje okna głównego. + + Te dwie liczby są jedynym sygnałem „to jest popup" — `layout._letterbox` + włącza się dokładnie na ich różnicy, więc gdyby capture zapisał rozmiar + płótna jako rozmiar zrzutu, letterboxing zniknąłby bez żadnego czerwonego + testu w warstwie układu. + """ + + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, popup = _windows(events) + scenario = Scenario( + config=_cfg(), + steps=[Step(click="ikona profilu"), Step(click="pole w wyskakującym oknie")], + ) + pages = await capture_pages( + scenario, + _compiled([_click(opens_popup=True), _click()]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + make_recorder=lambda page: FakeRecorder(events), + ) + + assert pages[0].screenshot_size == (1280, 720) + assert pages[1].screenshot_size == (600, 700) + # `_cfg()` viewport — the canvas never follows the active window. + assert [p.canvas_size for p in pages] == [(1280, 720), (1280, 720)] + + +async def test_first_popup_step_draws_no_arrow_from_the_other_window(tmp_path, monkeypatch): + """Ślad kursora zeruje się przy zmianie okna. + + Strzałka prowadzi wzrok od poprzedniego celu do bieżącego. Poprzedni cel + został w innym oknie i na innym obrazku, więc narysowana tu strzałka + wskazywałaby współrzędne, które na tej stronie nie znaczą nic. + """ + + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, popup = _windows(events) + scenario = Scenario( + config=_cfg(), + steps=[Step(click="ikona profilu"), Step(click="pole w wyskakującym oknie")], + ) + pages = await capture_pages( + scenario, + _compiled([_click(opens_popup=True), _click()]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + make_recorder=lambda page: FakeRecorder(events), + ) + + assert not [a for a in pages[1].annotations if a.kind == "arrow"] + + +async def test_a_popup_that_never_opens_fails_loudly_with_the_yaml_location(tmp_path, monkeypatch): + """Brak okna mimo zamrożonej flagi to twardy błąd, nie krok do pominięcia. + + Cicha tolerancja dałaby PDF, który wygląda na komplet instrukcji, a gubi + wszystko, co dzieje się w wyskakującym oknie — czyli zwykle sedno scenariusza. + """ + + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, _popup = _windows(events) + main.context.popup = None # click opens nothing + scenario = Scenario(config=_cfg(), steps=[Step(click="ikona profilu")]) + + with pytest.raises(GuideError, match="nowe okno"): + await capture_pages( + scenario, + _compiled([_click(opens_popup=True)]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + make_recorder=lambda page: FakeRecorder(events), + ) + + +async def test_entering_a_popup_without_a_recorder_factory_blames_the_caller(tmp_path, monkeypatch): + """Brak fabryki to błąd wywołania `capture_pages`, nie wada scenariusza. + + Dlatego `RuntimeError`, a nie `GuideError`: `GuideError` niesie `plik:linia` + i mówi autorowi, co poprawić w YAML-u — a tu nie ma czego poprawiać. + """ + + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, _popup = _windows(events) + scenario = Scenario(config=_cfg(), steps=[Step(click="ikona profilu")]) + + with pytest.raises(RuntimeError, match="fabryki Recordera"): + await capture_pages( + scenario, + _compiled([_click(opens_popup=True)]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + ) + + +async def test_pause_on_error_stops_on_the_window_that_failed(tmp_path, monkeypatch): + """`--pause-on-error` zostawia otwarte okno, w którym krok padł. + + Zanim `_Capture.page` stało się „oknem aktywnym", była tylko jedna możliwa + odpowiedź i pauza dostawała parametr `page` funkcji. Teraz błąd w popupie + zostawiłby dewelopera przed oknem głównym, na którym nic złego nie widać. + """ + + paused: list[object] = [] + + async def _pause(page, *_args, **_kwargs): + paused.append(page) + + monkeypatch.setattr(capture, "pause_for_inspection", _pause) + monkeypatch.setattr(capture, "reuse_failure", _async_none) + events: list[str] = [] + main, popup = _windows(events) + + class ExplodingRecorder(FakeRecorder): + async def point(self, target, ripple=False): + raise RuntimeError("krok padł w popupie") + + scenario = Scenario( + config=_cfg(), + steps=[Step(click="ikona profilu"), Step(click="pole w wyskakującym oknie")], + ) + with pytest.raises(RuntimeError, match="padł w popupie"): + await capture_pages( + scenario, + _compiled([_click(opens_popup=True), _click()]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + pause_on_error=True, + make_recorder=lambda page: ExplodingRecorder(events), + ) + + assert paused == [popup] + + +async def test_close_window_without_a_popup_is_a_no_op(tmp_path): + """`closeWindow` w scenariuszu bez popupu nie ma czego zamykać. + + Autor pisze go dla filmu; przewodnik nie ma powodu ani padać, ani zamykać + okna głównego — zostaje sama narracja. + """ + + events: list[str] = [] + main, _popup = _windows(events) + scenario = Scenario(config=_cfg(), steps=[Step(close_window=True, say="wracamy")]) + pages = await capture_pages( + scenario, + _compiled([None]), + main, + FakeRecorder(events), + tmp_path / "shots", + timeout=15.0, + ) + + assert [p.text for p in pages] == ["wracamy"] + assert "close:main" not in events diff --git a/tests/unit/guide/test_layout.py b/tests/unit/guide/test_layout.py index dbffa5f..25ba3c3 100644 --- a/tests/unit/guide/test_layout.py +++ b/tests/unit/guide/test_layout.py @@ -1,6 +1,6 @@ from pathlib import Path -from guidebot_recorder.guide.layout import render_html +from guidebot_recorder.guide.layout import fold_narration, render_html from guidebot_recorder.guide.model import Annotation, GuidePage @@ -94,6 +94,164 @@ def test_a_select_steps_marks_render_through_the_one_shared_arrowhead(): assert '' in html +def test_main_window_page_is_not_letterboxed(): + """Zrzut wypełniający płótno renderuje się dokładnie jak przed zmianą. + + Letterboxing włącza się wyłącznie przy różnicy rozmiarów, więc każda strona + okna głównego musi zostać bez stylów inline — inaczej zmiana „dla popupów" + po cichu przesunęłaby wszystkie dotychczasowe strony. + """ + + page = GuidePage( + kind="step", + screenshot=Path("/tmp/shot.png"), + text="t", + heading=None, + annotations=[], + screenshot_size=(1376, 800), + canvas_size=(1376, 800), + ) + html = render_html([page], title="x") + assert 'class="shot"' in html + assert "aspect-ratio" not in html + assert 'class="plate"' in html + + +def test_popup_page_is_centred_on_the_main_window_canvas(): + """Mniejszy zrzut dostaje płótno o proporcjach okna głównego i własną skalę. + + 600/1376 ≈ 43.605%, 700/800 = 87.5% — obie liczby muszą wyjść z jednego + stosunku, bo inaczej obrazek zostałby rozciągnięty względem sąsiednich stron. + """ + + page = GuidePage( + kind="step", + screenshot=Path("/tmp/popup.png"), + text="t", + heading=None, + annotations=[], + screenshot_size=(600, 700), + canvas_size=(1376, 800), + ) + html = render_html([page], title="x") + assert 'style="aspect-ratio:1376/800"' in html + assert 'style="width:43.605%;height:87.5%"' in html + + +def test_annotations_stay_in_the_stills_own_coordinate_system(): + """Adnotacje popupu opisują piksele zrzutu, nie płótna. + + To jest powód, dla którego `img` i `svg` siedzą we wspólnym `.plate`, a nie + w `.shot`: viewBox musi zostać rozmiarem zrzutu (600×700), a warstwa SVG musi + leżeć na obrazku, nie na całym płótnie. Gdyby SVG został przypięty do `.shot`, + każda strzałka i gwiazdka przesunęłaby się o szerokość marginesu. + """ + + page = GuidePage( + kind="step", + screenshot=Path("/tmp/popup.png"), + text="t", + heading=None, + annotations=[Annotation(kind="frame", x=10.0, y=20.0, w=300.0, h=40.0)], + screenshot_size=(600, 700), + canvas_size=(1376, 800), + ) + html = render_html([page], title="x") + assert 'viewBox="0 0 600 700"' in html + assert ".plate svg {" in html + assert ".shot svg {" not in html + + +def _text(text): + return GuidePage(kind="text", screenshot=None, text=text, heading=None, annotations=[]) + + +def _slide(heading): + return GuidePage(kind="slide", screenshot=None, text="", heading=heading, annotations=[]) + + +def test_narration_after_a_picture_folds_onto_it_under_a_rule(): + """`say:` między akcjami opisuje to, na co czytelnik patrzy. + + Nie zasłużyło na osobną kartkę z jednym zdaniem na środku — ląduje w panelu + bocznym poprzedniego kadru, oddzielone poziomą kreską. + """ + + html = render_html( + fold_narration([_shot_page([]), _text("Okno otwiera się osobno.")]), title="x" + ) + assert html.count('class="page"') == 1 + assert '
' in html + assert "Okno otwiera się osobno." in html + + +def test_narration_with_no_picture_before_it_keeps_its_own_page(): + """Nie ma czego dosiąść — zostaje pełnowymiarowa strona tekstowa.""" + + html = render_html(fold_narration([_text("Zanim zaczniemy.")]), title="x") + assert html.count('class="page"') == 1 + assert '
' not in html + assert "Zanim zaczniemy." in html + + +def test_a_slide_neither_absorbs_narration_nor_is_absorbed(): + """Slajd to celowa plansza pełnoekranowa, więc nie zwija się z niczym. + + Slajd tytułowy z narracją zaraz za nim dawałby inaczej stronę tytułową + z doklejonym zdaniem — a to jest przerywnik, nie kadr do opisania. + """ + + html = render_html(fold_narration([_slide("Logowanie"), _text("Witaj.")]), title="x") + assert html.count('class="page"') == 2 + assert '
' not in html + + +def test_several_narrations_fold_onto_one_picture_each_under_its_own_rule(): + pages = [_shot_page([]), _text("Pierwsze zdanie."), _text("Drugie zdanie.")] + html = render_html(fold_narration(pages), title="x") + assert html.count('class="page"') == 1 + assert html.count('
') == 2 + + +def test_empty_narration_folds_to_nothing_rather_than_a_bare_rule(): + """Pusta narracja nie ma czego wnieść, a kreska bez tekstu to śmieć na stronie. + + To zarazem koniec pustych kartek: krok narracyjny bez tekstu drukował dotąd + stronę z niczym. + """ + + html = render_html(fold_narration([_shot_page([]), _text("")]), title="x") + assert html.count('class="page"') == 1 + assert '
' not in html + + +def test_fold_narration_is_what_the_document_is_counted_by(): + """Zwinięta lista to jedna i ta sama lista, którą się drukuje i liczy. + + `run_guide` zwraca długość listy jako liczbę stron, więc gdy zwijanie siedziało + wewnątrz `render_html`, CLI ogłaszało osiem stron pięciostronicowego PDF-a. + Ten test pilnuje, że zwijanie jest krokiem wywołującego, a nie efektem ubocznym + renderowania. + """ + + pages = [_slide("Tytuł"), _shot_page([]), _text("dopisek"), _text("i jeszcze")] + sheets = fold_narration(pages) + html = render_html(sheets, title="x") + + assert len(sheets) == 2 + assert html.count('class="page"') == len(sheets) + # A gdyby renderer nadal zwijał sam, podwójne zwinięcie byłoby niewykrywalne. + assert render_html(pages, title="x").count('class="page"') == 4 + + +def test_folding_does_not_mutate_the_caller_s_pages(): + """`render_html` nie może przepisywać wejścia — to lista wywołującego.""" + + shot, text = _shot_page([]), _text("dopisek") + fold_narration([shot, text]) + assert shot.folded_text == [] + + def test_text_page_has_no_svg(): pages = [ GuidePage(kind="text", screenshot=None, text="tylko tekst", heading=None, annotations=[]) diff --git a/tests/unit/guide/test_prolog.py b/tests/unit/guide/test_prolog.py index 472a1fc..18022b8 100644 --- a/tests/unit/guide/test_prolog.py +++ b/tests/unit/guide/test_prolog.py @@ -1,6 +1,11 @@ import pytest -from guidebot_recorder.guide.prolog import GuideError, classify, scan_for_blockers +from guidebot_recorder.guide.prolog import ( + NARRATION_ONLY_KINDS, + GuideError, + classify, + scan_for_blockers, +) from guidebot_recorder.models.action import CachedAction, Fingerprint, PendingAction from guidebot_recorder.models.config import Config, TtsConfig, Viewport from guidebot_recorder.models.scenario import FlatStep, Scenario, Step, WhenBlock @@ -79,10 +84,29 @@ def test_scan_allows_a_visual_only_command(): scan_for_blockers(scen.flat_steps(), [None]) # no raise -def test_scan_raises_on_popup(): +def test_scan_allows_a_click_that_opens_a_popup(): + """Popup nie jest już blokadą — `guide` przełącza się na nowe okno. + + Ten przypadek odwraca dawne `test_scan_raises_on_popup`. Zamrożona flaga + `opens_popup` przestała być powodem odmowy, a stała się instrukcją: krok + otwiera okno, a kolejne kroki czyta się względem niego. + """ + scen = Scenario(config=_cfg(), steps=[Step(click="opens something")]) - with pytest.raises(GuideError, match="popup"): - scan_for_blockers(scen.flat_steps(), [_cached(opens_popup=True)]) + scan_for_blockers(scen.flat_steps(), [_cached(opens_popup=True)]) # no raise + + +def test_close_window_is_its_own_kind_not_narration(): + """`closeWindow` wykonuje pracę w przeglądarce, więc ma własny rodzaj strony. + + Dopóki popup był odrzucany, `closeWindow` nie miał czego zamykać i siedział + w `NARRATION_ONLY_KINDS`. Gdyby tam został, przełączenie z powrotem na okno + główne nigdy by nie nastąpiło, a dalsze kroki fotografowałyby zamknięty popup. + """ + + assert classify_step_of(Step(close_window=True)) == "closeWindow" + assert classify_step_of(Step(close_window=True, say="wracamy")) == "closeWindow" + assert "closeWindow" not in NARRATION_ONLY_KINDS def test_scan_raises_on_mandatory_pending():