From 84dddccc353e3db70bbabf0b73f78f5b339a50fa Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 7 Sep 2026 23:01:44 +0200 Subject: [PATCH 1/2] feat(html): a sheet writes the same markup editable or not `translate_sheet` stamped `contenteditable` and `data-odr-path` on every run inside a cell when `HtmlConfig::editable` was set, asking the element and never the document. That is a half-working sheet editor - a string saved, a number desynced from `office:value` - and it laid the sheet out differently, since `plain_text` refuses to fold an editable run into its `td`. A sheet's editing is an overlay, so its markup states none of it: `WritingState` gains `editable_markup`, false for the state a sheet writes its content through. Text documents, which have no overlay, are unchanged. The reference output loses every `contenteditable` under `ods/` and the runs fold back into their cells. No page's text changes: all 221 files differ in markup alone. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J325TWocZ4iXBjvKv2ZVZi --- CHANGELOG.md | 4 ++++ docs/design/spreadsheet-editing.md | 18 ++++------------ src/odr/internal/html/common.hpp | 8 ++++++++ src/odr/internal/html/document_element.cpp | 23 +++++++++++++++------ test/src/html_output_test.cpp | 3 +-- test/src/html_test.cpp | 24 +++++++++++++++++----- 6 files changed, 53 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 940d6eecf..a2b8ae46f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- A sheet rendered with `HtmlConfig::editable` carries no `contenteditable` + and no `data-odr-path`: its editing is an overlay, so the markup states + none. A cell's runs fold into the `td` as they do read-only. + - **Breaking** (wire only) `Document::edit` takes an op envelope, `{"version": 1, "ops": [...]}`, with `setCell` writing a sheet cell by position and `setText` carrying what the `modifiedText` map carried. diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index 595713dbb..75036159f 100644 --- a/docs/design/spreadsheet-editing.md +++ b/docs/design/spreadsheet-editing.md @@ -52,15 +52,6 @@ results go stale the moment an input changes. | Addressing | `DocumentPath` | Already spells a cell by position: `/child:0/cell:A1/...` | | Capabilities | `file_type_table.cpp` | `ods` and `xlsx` declare `edit` and `save` (step 0.2, landed); `csv` declares neither. `odr_test` checks the declaration against `Document::is_editable` | -One inconsistency worth fixing on day one: `translate_sheet` stamps -`contenteditable` on every run inside an `.ods` cell when `config.editable` is -set — the writer asks the *element* (`element_is_editable`, true for a -non-repeated cell) and never the document. The reference output for -`style-color+fixed-1.ods` carries 594 of them. Two consequences: an app that -turns `editable` on gets a half-working sheet editor (strings save, numbers -desync), and the editable output lays out differently, because -`plain_text` refuses to fold an editable run into its `td`. - ## Decisions ### 1. A cell is the unit of editing, addressed by position @@ -308,11 +299,10 @@ Each step ships on its own. "Both" means `.ods` and `.xlsx`. sheet is 0.36 s today. 5. **Landed.** `Document::is_editable` true for both; capability rows gained `edit` (`xlsx` also `save`); `odr_test` keeps them honest. -6. Stop `translate_sheet` stamping `contenteditable` on a cell's runs at all. - It cannot be gated on `Document::is_editable`, which item 5 makes *true* for - a sheet: decision 3 puts a sheet's editing in an overlay, so the markup - carries none. Changes the reference output — a reference `.ods` loses 594 - attributes — so it lands with a regen, on its own. +6. **Landed.** `translate_sheet` writes its cells through a `WritingState` + whose `editable_markup` is false, so no run carries `contenteditable` and + `plain_text` folds it into the `td` as it does read-only. It could not be + gated on `Document::is_editable`, which item 5 makes *true* for a sheet. 7. **Landed.** Tests: set a number, a string, clear a cell, and each refusal, on both formats, from inline fixtures; save and reopen. The LibreOffice oracle (`soffice --convert-to`) stays a by-hand check — it is not in CI, and diff --git a/src/odr/internal/html/common.hpp b/src/odr/internal/html/common.hpp index 24a47c0bc..1aa81dbe5 100644 --- a/src/odr/internal/html/common.hpp +++ b/src/odr/internal/html/common.hpp @@ -44,6 +44,13 @@ struct WritingState { [[nodiscard]] TextDirection direction() const { return m_direction; } void set_direction(const TextDirection direction) { m_direction = direction; } + /// Whether an editable run says so in the markup - false under a sheet, + /// whose editing is an overlay. + [[nodiscard]] bool editable_markup() const { return m_editable_markup; } + void set_editable_markup(const bool editable) { + m_editable_markup = editable; + } + private: HtmlWriter *m_out; const HtmlConfig *m_config; @@ -51,6 +58,7 @@ struct WritingState { const Logger *m_logger; StyleRegistry *m_styles; TextDirection m_direction{TextDirection::left_to_right}; + bool m_editable_markup{true}; }; /// Writes the viewport meta tag. Precedence: `config.viewport_content` (raw, diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp index 4df5dcc2b..ee757657b 100644 --- a/src/odr/internal/html/document_element.cpp +++ b/src/odr/internal/html/document_element.cpp @@ -235,12 +235,19 @@ std::optional sheet_print_fit(const Sheet &sheet, /// A run whose style the box around it can carry instead. Not a background, a /// raised run or an editable one: each means something else on the box. +/// Whether @p element carries `contenteditable`: an editable run of a view +/// that writes its editing into the markup. +bool writes_editable(const Element &element, const html::WritingState &state) { + return state.editable_markup() && state.config().editable && + element.is_editable(); +} + std::optional plain_text(const Element &element, const html::WritingState &state) { if (element.type() != ElementType::text) { return {}; } - if (state.config().editable && element.is_editable()) { + if (writes_editable(element, state)) { return {}; } @@ -411,6 +418,10 @@ std::optional html::sheet_cut(const Sheet &sheet, } void html::translate_sheet(const Sheet &sheet, const WritingState &state) { + // a sheet's editing is an overlay, so its content carries none + WritingState sheet_state = state; + sheet_state.set_editable_markup(false); + const TableDimensions rendered = sheet_rendered_extent(sheet, state.config()); const std::uint32_t end_column = rendered.columns; const std::uint32_t end_row = rendered.rows; @@ -594,8 +605,8 @@ void html::translate_sheet(const Sheet &sheet, const WritingState &state) { } } - const std::optional folded = - fold_cell(cell, state, wraps, anchors_shapes, table_row_style.height); + const std::optional folded = fold_cell( + cell, sheet_state, wraps, anchors_shapes, table_row_style.height); state.out().write_element_begin( "td", @@ -624,13 +635,13 @@ void html::translate_sheet(const Sheet &sheet, const WritingState &state) { }())); if (column_index == 0 && row_index == 0) { for (const Element shape : sheet.shapes()) { - translate_element(shape, state); + translate_element(shape, sheet_state); } } if (folded.has_value()) { state.out().out() << folded->text; } else { - translate_cell_children(cell, state); + translate_cell_children(cell, sheet_state); } state.out().write_element_end("td"); @@ -695,7 +706,7 @@ void html::translate_text(const Element &element, const WritingState &state) { HtmlElementOptions() .set_inline(true) .set_attributes([&](const HtmlAttributeWriterCallback &clb) { - if (state.config().editable && element.is_editable()) { + if (writes_editable(element, state)) { clb("contenteditable", "true"); clb("data-odr-path", element.document_path().to_string()); } diff --git a/test/src/html_output_test.cpp b/test/src/html_output_test.cpp index f09d0092f..add3caff5 100644 --- a/test/src/html_output_test.cpp +++ b/test/src/html_output_test.cpp @@ -299,8 +299,7 @@ std::vector> list_variant_cases() { {"odr-public/docx/physics.docx", reflow}, // The output a reader gets rather than an editor. A sheet is pinned - // too: a cell holding one plain string drops the run around it only - // where nothing has to carry `contenteditable`. + // too, where the two agree: its editing is an overlay. {"odr-public/odt/style-various-1.odt", read_only}, {"odr-public/ods/file_example_ODS_100.ods", read_only}, }; diff --git a/test/src/html_test.cpp b/test/src/html_test.cpp index 517f2298b..c35963a4f 100644 --- a/test/src/html_test.cpp +++ b/test/src/html_test.cpp @@ -839,15 +839,29 @@ TEST(html, a_cell_holding_one_plain_string_writes_no_box_of_its_own) { EXPECT_EQ(page.find(" Date: Mon, 7 Sep 2026 23:20:34 +0200 Subject: [PATCH 2/2] test: advance the reference output pins The sheets lose their `contenteditable`, and the two resource files that no output change carried with them catch up. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J325TWocZ4iXBjvKv2ZVZi --- test/data.cmake | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/data.cmake b/test/data.cmake index e00c71919..f2e3f635a 100644 --- a/test/data.cmake +++ b/test/data.cmake @@ -17,9 +17,9 @@ odr_test_data( odr_test_data( PATH "reference-output/odr-public" URL "https://github.com/opendocument-app/OpenDocument.test.output.git" - REVISION "19392dfc4e3f4c5507dae231fe96a92fe0ca29f4") + REVISION "e8dd6b52062fa8efa82a90f3e31b6c5ea7eeb58a") odr_test_data( PATH "reference-output/odr-private" URL "https://github.com/opendocument-app/OpenDocument.test-private.output.git" - REVISION "ee3077eaf35b36e084f14b130c4e9236fc9be0f1") + REVISION "ed1be07780b703b5400accbae8ccb124e68ebb71")