Skip to content

Commit cf12865

Browse files
committed
[FIX] borders: preserve side borders when combining External and All
When applying an External border on a range and then applying All borders on an inner/overlapping range, the side borders (left/right) of the inner cells were lost after re-opening the spreadsheet. The exported border data was correct, but the BordersPlugin recomputation logic in addBorder incorrectly cleared adjacent borders on import/update. The adjacency check (adjacent(existingBorder.zone, zone)) reported the side of the existing zone, but we were deciding whether to clear it based on the same side key in the new border, instead of the opposite side (shared edge) on the new zone. As a result, importing the combination of: C2:C4 with All borders B2:B4 with left/top/bottom D2:D4 with right/top/bottom ended up clearing the left and right borders of C2:C4. This commit adjusts the adjacent clearing logic to: Map the adjacent side of the existing zone to the opposite side on the new zone (i.e. left → right, right → left, top → bottom, bottom → top). Only clear the existing side if the corresponding opposite side is actually being written on the new border. closes #7513 Task: 5270171 X-original-commit: 1a9de79 Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
1 parent d535744 commit cf12865

File tree

2 files changed

+74
-7
lines changed

2 files changed

+74
-7
lines changed

packages/o-spreadsheet-engine/src/plugins/core/borders.ts

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -344,22 +344,35 @@ export class BordersPlugin extends CorePlugin<BordersPluginState> implements Bor
344344
) {
345345
const borders: ZoneBorder[] = [];
346346
const plannedBorder = newBorder ? { zone, style: newBorder } : undefined;
347-
const sideToClear = {
348-
left: force || !!newBorder?.left,
349-
right: force || !!newBorder?.right,
350-
top: force || !!newBorder?.top,
351-
bottom: force || !!newBorder?.bottom,
347+
348+
// For each side, decide if we must clear the border on the *adjacent*
349+
// existing cell when we draw on the opposite side of the new zone.
350+
//
351+
// Example:
352+
// - newBorder.right is set → we draw border on the RIGHT side of `zone`
353+
// - the cell on the right may already have a LEFT border on that edge
354+
// In that case we clear that LEFT border, so only the new RIGHT border
355+
// remains on the shared edge.
356+
//
357+
// existingBorderSideToClear[side] = true means we should clear the border on that
358+
// side of the existing adjacent zone before adding the new border.
359+
const existingBorderSideToClear = {
360+
left: force || !!newBorder?.right,
361+
right: force || !!newBorder?.left,
362+
top: force || !!newBorder?.bottom,
363+
bottom: force || !!newBorder?.top,
352364
};
353365
let editingZone: Zone[] = [zone];
354366
for (const existingBorder of this.borders[sheetId] ?? []) {
355367
const inter = intersection(existingBorder.zone, zone);
356368
if (!inter) {
357-
// Clear adjacent borders on which you write
369+
// Check if the existing border is adjacent to the new zone
358370
const adjacentEdge = adjacent(existingBorder.zone, zone);
359-
if (adjacentEdge && sideToClear[adjacentEdge.position]) {
371+
if (adjacentEdge && existingBorderSideToClear[adjacentEdge.position]) {
360372
for (const newZone of splitIfAdjacent(existingBorder.zone, zone)) {
361373
const border = this.computeBorderFromZone(newZone, existingBorder);
362374
const adjacentEdge = adjacent(newZone, zone);
375+
// Clear the existing border on the side that touches the new zone
363376
switch (adjacentEdge?.position) {
364377
case "left":
365378
border.style.left = undefined;

tests/borders/border_plugin.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,60 @@ describe("borders", () => {
147147
});
148148
});
149149

150+
test("Preserves side borders when combining external and all via command", () => {
151+
const model = new Model();
152+
const defaultBorder = DEFAULT_BORDER_DESC;
153+
154+
setZoneBorders(model, { position: "all" }, ["C3"]);
155+
setBordersOnTarget(model, ["C2"], {
156+
top: defaultBorder,
157+
right: defaultBorder,
158+
left: defaultBorder,
159+
});
160+
setBordersOnTarget(model, ["C4"], {
161+
bottom: defaultBorder,
162+
left: defaultBorder,
163+
right: defaultBorder,
164+
});
165+
setBordersOnTarget(model, ["B3"], {
166+
top: defaultBorder,
167+
bottom: defaultBorder,
168+
left: defaultBorder,
169+
});
170+
setBordersOnTarget(model, ["D3"], {
171+
top: defaultBorder,
172+
bottom: defaultBorder,
173+
right: defaultBorder,
174+
});
175+
176+
expect(getBorder(model, "C3")).toEqual({
177+
top: defaultBorder,
178+
bottom: defaultBorder,
179+
left: defaultBorder,
180+
right: defaultBorder,
181+
});
182+
expect(getBorder(model, "C2")).toEqual({
183+
top: defaultBorder,
184+
left: defaultBorder,
185+
right: defaultBorder,
186+
});
187+
expect(getBorder(model, "C4")).toEqual({
188+
bottom: defaultBorder,
189+
left: defaultBorder,
190+
right: defaultBorder,
191+
});
192+
expect(getBorder(model, "B3")).toEqual({
193+
top: defaultBorder,
194+
left: defaultBorder,
195+
bottom: defaultBorder,
196+
});
197+
expect(getBorder(model, "D3")).toEqual({
198+
top: defaultBorder,
199+
bottom: defaultBorder,
200+
right: defaultBorder,
201+
});
202+
});
203+
150204
test("can set all borders in a zone", () => {
151205
const model = new Model();
152206

0 commit comments

Comments
 (0)