Compare diff isn't working

[emteknetnz]

CWP-2.8.0-rc1

I can "compare" versions, though there is not a content diff i.e. the green/red diff of text changes between versions

Instead it seemed like it only showed the most recent version

I see this on /admin/pages/history/show/6132

The content 'diff text' is on the latest version, but not the older version that I'm comparing against

Tested in IE11 and Firefox

Acceptance Criteria

• Text added without a matching deleted sample is highlighted in green
• Deleted text style is retained

Notes

• This never worked in Safari before ... we don't expect it to.
• There's a weird code snippet that's been copied in our codebase to handle the diff generation. If it's possible to patch it, great! Otherwise we might need to look for an alternative library.

Pull Requests

• FIX Highlight paragraph changes in diff. #237

[maxime-rainville]

I'm only getting a diff when content is deleted/replace.

When brand new content is added that doesn't get highlighted.

Initial version

Updated version

Diff

[brynwhyman]

This was called out as a regression but I'm wondering if it indeed is... There are some related issues out there already:

• Diff doesn't show an entirely new line as an addition
• No colour indicating changes between blocks on pages

[emteknetnz]

Replicated in cms 4.7.x-dev - so it's not a regression, it's an existing issue

[GuySartorelli]

What's happening

This is a styling issue. It is caused by the <ins> tag surrounding the new <p> tag and its content. Deletions of entire paragraphs also don't get the appropriate background-color applied, though the deleted content does get strikethrough applied.
ins and del elements are allowed to contain p (and other block-level) tags, but then their display style must be changed, as it's inline by default.

Easy solution: Use display: inline-block

There's unfortunately no parent selector in css yet, so I can't add a style that says "style any ins or del elements that contain p or div tags with display:block. Setting all ins or del tags to display:inline-block will slightly change the appearance of the tags, but is definitely the easiest solution.

Current appearance:

Using inline-block:

Harder solution: Push the del and ins tags inside the block-level elements.

We could check for any del and ins tags that contain block-level elements, and invert them so the block-level element instead contains the del or ins tag. This will retain the current styling, but may look a little strange for longer paragraphs - not sure which is more desirable. This approach also has more edge cases (e.g. making sure we catch all block-level elements, plus what happens with nested block-level elements?)
This can be done either with an HTML parser in the php code or using javascript on the client-side.

[emteknetnz]

The inline-block solution seems fine and unlikely to cause regressions? Is there actually a downside to going with this?

[GuySartorelli]

I can't think of any downside provided there's no complaints about the slight style change.
I'm aware we used to have a designer in product to look at stuff like this so I figured in lieu of that I'd just give you and Max a chance to let me know if it's okay before I make the change.

[emteknetnz]

Go for it

[emteknetnz]

Linked PR has been merged