Page MenuHomePhabricator

Empty line differences confuse TwoColConflict UI
Closed, ResolvedPublic13 Estimated Story PointsBUG REPORT

Description

This may be a regression in our newline handling, or just an edge case. When viewing a conflict between base revision https://test.wikipedia.org/w/index.php?diff=425275 , conflicting revision https://test.wikipedia.org/w/index.php?diff=425276 , and my edits which added a line at the end, I got this dialog:

The empty boxes are disconcerting although reasonable since there's a whitespace change. However, the difference isn't visible because the text boxes both contain no text. Newlines have been optimized out into the split-linefeeds hidden inputs, so there's no way for a user to meaningfully choose between sides.

A second bug: when the user edits and resets the text field, the newlines reappear but this time inside the text! Hidden linefeed counts are still present, so I would imagine the linefeeds will be saved double. The difference between actual text and stored "reset" text also throws off our change detection code on the client.

A third bug: selecting the side with the newlines, "\n\n" in the textarea and hidden field with value "2" linefeeds, and saving the page, I would expect to see 4 linefeeds. Instead, I see zero.


Fixed bugs

Leading newlines get lost

https://gerrit.wikimedia.org/r/589011
How to demo: Create a conflict, but make sure to add some newlines at the beginning of the page when you do the second edit. The merge UI will show these as two empty boxes (this is to be fixed later, see T250402). Pick the empty box on the right (it represents the newlines you added). Merge. Look at the diff. The newlines should appear there.

Reset warning pops up when it shouldn't

https://gerrit.wikimedia.org/r/589020
How to demo: Cause a conflict. Start editing in one of the blocks. Add or remove newlines at the end of this block. Accept this block. Now reset this block. No warning should be shown, because newlines at the end don't matter when the merge is done later.

Incomplete tracking of newlines at the beginning of text blocks

https://gerrit.wikimedia.org/r/589219
This is related to T248668: Edge case: newlines can break talk page use case. Before, newlines have been tracked as part of the previous block of text. This worked badly when there was no previous block, or the previous block was a conflicting one.
How to demo: Create a page with a few paragraphs. Make sure there are multiple empty lines between two of the paragraphs. Now cause a conflict in the text below the empty space you just created. Change some text and add one or more additional newlines. Old, bad behavior: This caused 2 conflicts, one in the paragraph where the text actually changed, and one in the previous paragraph. Fixed behavior: Only the paragraph with actual changes shows up. When you pick your side, the resulting page should contain both the text changes as well as the additional newlines.

Event Timeline

awight created this task.Apr 15 2020, 11:42 AM
Restricted Application added a project: archived--TCB-Team. · View Herald TranscriptApr 15 2020, 11:42 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
awight changed the subtype of this task from "Task" to "Bug Report".Apr 15 2020, 11:43 AM

To be honest I don't understand what the "3 bugs" are. Do you have enough information to be able to create separate tickets for each?

I replayed the situation you described. My observations:

  • Our conflict resolution UI tracks newlines in hidden fields, but only newlines at the end of each block.
  • In the example, the first block contains nothing but newlines.
  • There is currently no other way than showing these as empty boxes. We could introduce another hidden field that tracks the number of newlines at the beginning of a diff (not per row, but only 1 hidden field per diff). However, I don't think this is worth the effort. This should never happen in real-world situations.
  • In my test the hidden field is correctly set to 1. Remember, this is the number of extra newlines.
  • Bug 1: When saving, these newlines disappear. This is a bug I can confirm.
  • Bug 2: The reset buttons act weird. The popup to confirm the reset appears even if I have not touched the text. This should not happen. The reset feature should ignore all newlines at the end of the text.
  • When this is submitted, newlines at the end of each block are ignored, no matter how many have been added. This is intentional. The hidden counts are used instead.

So I can confirm 2 bugs.

awight triaged this task as Lowest priority.Apr 15 2020, 12:50 PM

Thanks for looking at this. The first bug (let's call it Bug 0) is that the user is presented with no information about the contents of the two columns. They will probably conclude that either there's a bug in the software, or both blocks are empty (and it still appears to a user that there's a bug in the software, because this should have been a copy row if the sides are the same). There's no way to tell that one side has newlines and the other is really empty.

I'm marking this very low priority because it's a ridiculous edge case. Feel free to increase again if there's a real situation where leading newlines are meaningful.

Ok, when you describe it this way it is a bug. The user does not have enough information to understand what's going on.

I suggest to focus on bug 0 in this ticket here, and create 2 new tickets for the other 2. Both number 1 and 2 should be fixed, in my opinion. Number 0 not so much.

Change 589011 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Fix merged trimming leading newlines

https://gerrit.wikimedia.org/r/589011

Change 589020 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Fix edit warning popping up because of irrelevant newlines

https://gerrit.wikimedia.org/r/589020

Change 589219 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Track linefeeds at the beginning of blocks as well

https://gerrit.wikimedia.org/r/589219

Change 589020 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Fix edit warning popping up because of irrelevant newlines

https://gerrit.wikimedia.org/r/589020

Change 589219 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Track linefeeds at the beginning of blocks as well

https://gerrit.wikimedia.org/r/589219

thiemowmde updated the task description. (Show Details)Apr 20 2020, 8:22 AM
thiemowmde moved this task from Doing to Review on the WMDE-QWERTY-Sprint-2020-04-15 board.

Change 589011 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Fix merger trimming leading newlines

https://gerrit.wikimedia.org/r/589011

Lena_WMDE set the point value for this task to 8.Apr 23 2020, 9:12 AM
Lena_WMDE removed the point value for this task.
thiemowmde set the point value for this task to 13.
awight added a subscriber: ecohen.Apr 23 2020, 11:00 AM

Leading newlines: Fixed! The newlines are almost invisible, apparently the width of a space character and especially hard to see in the light pastel of our color scheme. I can't think of a good suggestion to improve, however. I feel like there's already a paradigm in MediaWiki? If not, I would be happy with: "↩", U+21A9 LEFTWARDS ARROW WITH HOOK. Winderung what @ecohen thinks?
Screenshot:

Reset warning: Not fixed. Interestingly, it sometimes works, but depends on the order of interactions. If the page is freshly loaded, clicking pencil and close will incorrectly show the popup. If you "cancel" it will continue to pop up, with no real changes. Once you choose to "discard" changes, this behavior switches to correctly not showing the popup unless real changes are present. The bug only affects pure-whitespace cells.

Newlines dropped: Only fixed for initial newlines. Not fixed for internal pure-newline blocks. Steps to reproduce: create a conflict which includes added newlines between existing paragraphs. Select the side with extra newlines (the display is really nice now!). Result: no extra newlines are saved.

Moving back to "doing" for visibility.

Yea, the visibility of whitespace in diffs is bad. I even have a user script to fix exactly that (and more). But I think we should not deviate from the original diff design. If we are going to make changes, we might need to apply them to all diffs.

If the page is freshly loaded, clicking pencil and close will incorrectly show the popup. If you "cancel" it will continue to pop up, with no real changes. Once you choose to "discard" changes, this behavior switches […]

I can not reproduce this. Furthermore, what is the difference between "close", "cancel", and "discard"? Can you please describe the steps to reproduce using, I don't know, "pencil", "checkmark" and "x"?

I can confirm the "newlines dropped" bug. I know why it happens and will look into it.

If the page is freshly loaded, clicking pencil and close will incorrectly show the popup. If you "cancel" it will continue to pop up, with no real changes. Once you choose to "discard" changes, this behavior switches […]

I can not reproduce this. Furthermore, what is the difference between "close", "cancel", and "discard"? Can you please describe the steps to reproduce using, I don't know, "pencil", "checkmark" and "x"?

I can't reproduce any more, either. Magically solved!

Change 592607 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Separate tracking of whitespace-only paragraphs

https://gerrit.wikimedia.org/r/592607

Change 592607 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Separate tracking of whitespace-only paragraphs

https://gerrit.wikimedia.org/r/592607

Lena_WMDE closed this task as Resolved.Apr 29 2020, 10:11 AM
Lena_WMDE claimed this task.
Lena_WMDE moved this task from Demo to Done on the WMDE-QWERTY-Sprint-2020-04-15 board.