Skip to content

[BUGFIX] move container after itself to next sibling - #772

Open
engineintern wants to merge 1 commit into
b13:masterfrom
engineintern:bugfix/move-container-after-self
Open

engineintern wants to merge 1 commit into
b13:masterfrom
engineintern:bugfix/move-container-after-self

Conversation

@engineintern

Copy link
Copy Markdown

Page module drop-after-container sends target=-uid of the moved
container. Rewriting that to after the last child keeps the record
in the same column slot, so move-down is a no-op. Retarget to the
next sibling in the same page column instead.

Page module drop-after-container sends target=-uid of the moved
container. Rewriting that to after the last child keeps the record
in the same column slot, so move-down is a no-op. Retarget to the
next sibling in the same page column instead.
@achimfritz

Copy link
Copy Markdown
Contributor

Hi @engineintern , sorry, i do not understand when this bug can occure, can you a description, how to see the bug? e.g. having ..., clicking ..., expecting ..., got ....,

@engineintern

Copy link
Copy Markdown
Author

Hi Achim,

the bug shows up in the Page module when you try to move a container one slot down. Move-up works; move-down does not persist.

We hit this with our own CTypes registered via ContainerConfiguration: a 2-column (left / right) and a 3-column (left / middle / right). Both are normal b13 containers with children in those columns — not Flux grids. First seen on the 3-column, same behaviour on the 2-column. Column count does not matter; any container that already has children is enough.

Having

  • a page with at least two records in the same backend column (colPos)
  • the first record is a container (2-col, 3-col, or any other registered container) that already has children
  • after it, at least one other content element (container or regular CE)

Clicking

  • Page module → drag the container by its header
  • drop it on the “after this container” zone (below its children / one position down)
  • that drop sends cmdmap move with target = -uid of the moved container itself
  • reload the Page module (and check the frontend)

Expecting

  • the container (with its children) sits one position lower, after the next sibling
  • the new order is stored and still there after reload

Got (current master)

  • the UI may flash the new position after drag&drop -> but that change has not saved to DB.
  • after reload the container is back where it was; frontend unchanged / backend unchanged
  • nothing useful is written: rewriteCommandMapTargetForAfterContainer() treats “after this container” as “after the last child of this container”
  • for a self-move that is the same column slot (children already sort between the parent and the next sibling), so the move is a no-op

What this PR changes

  • if move and $id === $target (drop after itself), retarget to the next sibling in the same page column instead of the last child
  • if that sibling is itself a container, use getAfterContainerElementTarget() of that sibling so we land after its children

Move to the top of the column is a different command (target is another uid or the page) and was already fine. This only fixes “drop this container after itself”.

Hope that description gives u a clue...

@achimfritz

Copy link
Copy Markdown
Contributor

Thanks for the detailed steps,

Dropping a container on its own "after" dropzone sends target=-, i.e. "insert after myself". That's a no-op, and I think that's the correct behavior. It's the same for regular content elements in core: dropping an element right below itself doesn't move it either.

To move the container one position down, drop it on the dropzone after the next element (target=-). This case (moving a container after the next element, including its children) is already covered by moveContainerAfterElementMovesChildren in Tests/Functional/Datahandler/DefaultLanguage/ContainerTest.php.

With this change, the container's own dropzone would mean "move one down" for containers only. That diverges from core behavior and would be surprising for editors.

Could you check whether dropping after the next element works for you? If it doesn't, please describe that case (setup / action / expected / actual), because that would be a real bug we should fix. Otherwise I'd suggest closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants