| Schnark | |
| Feb 24 2018, 10:45 AM |
| F34923460: Screenshot 2022-01-19 at 19.18.36.png | |
| Jan 19 2022, 6:26 PM |
| F34914952: image.png | |
| Jan 12 2022, 12:33 AM |
| F34913691: image.png | |
| Jan 10 2022, 11:26 PM |
| F34913684: image.png | |
| Jan 10 2022, 11:25 PM |
Example: https://de.wikipedia.org/w/index.php?diff=174338623&oldid=174328623&diffmode=visual&visualdiff=1
In this particular case, the problem is caused by the diff timing out, at which point it just gives up and says everything in the old version was removed and everything in the new version was inserted... Once T180842 is fixed, we will at least show a message to explain that this has happened.
You're right that the containers are causing the problem here - without the containers, the diff doesn't time out. The reason is that there is a speed-up that is applied to top level elements in the document, where we use a fast check to see if any of them are exactly the same. If all of those lists/templates were top-level, then we'd quickly match up the ones that are the same using the fast check; however, in this case the <div> is the only top-level element, and once the fast check finds that the old <div> isn't exactly the same as the new <div>, it then applies the slow differ to everything inside.
The reason everything inside is slow to diff is because of all the lists - the slow part of the differ is particularly slow for lists. Once we special-case lists (see T187632) I would expect that this wouldn't time out any more...
@Tchanders Given the complexity, maybe we should put this back into the backlog? Displaying the timeout at least makes it more obvious what's happened, which might be good enough for now.
The reason I put it here is because the work I'm doing on lists diffs happens to solve this too, but happy to move it if that makes more sense...
Change 750777 had a related patch set uploaded (by Bartosz Dziewoński; author: Esanders):
[VisualEditor/VisualEditor@master] Diff some nodes as if they are documents, e.g. <div>s
Change 750777 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Diff some nodes as if they are documents, e.g. <div>s
The patch significantly improves this, although it also reveals new issues – apparently the background on the container hides all of the diff context (this is similar to T292491). But if you overlook that, the changes are highlighted perfectly.
Change 752764 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (2db326345)
Change 752764 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (2db326345)
Change 753179 had a related patch set uploaded (by Esanders; author: Esanders):
[VisualEditor/VisualEditor@master] DiffElement: Change order of z-index fixes
Change 753198 had a related patch set uploaded (by Esanders; author: Esanders):
[VisualEditor/VisualEditor@master] DiffElement: Collapse context to spacers in isDiffedAsDocument nodes
The above patches fix the z-index issue, and then handle context collapsing in document wrappers (the 3 vertical dots). The diff looks like this now:
Change 753179 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] DiffElement: Change order of z-index fixes
Change 753198 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] DiffElement: Collapse context to spacers in isDiffedAsDocument nodes
Change 754004 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (57e82187b)
Change 754004 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (57e82187b)
The above patches fix the z-index issue, and then handle context collapsing in document wrappers (the 3 vertical dots). The diff looks like this now:
On a closer look, the diff in the second list is incorrect – the inserted "Template:J" is shown at the end, but it should be shown immediately after the removed list item. I wonder if that's because there's another "Template:J" there? Or maybe this is related to T198529? Anyway, I think we can ignore this for now, and treat it as a separate issue.
Wrapped the page content in a <div> container and made changes to the list. This behaves as expected:
