Page MenuHomePhabricator

Support JS enhanced editing on the merge view (No. 7)
Closed, ResolvedPublic3 Estimated Story Points

Description

Motivation
Users should be able to merge versions when they encounter an edit conflict. This should be done in the selected paragraph. However, since people should always know that all paragraphs are in theory editable, both show the edit button, just in different states of activity

Acceptance Criteria

  • If users click on the activated edit icon,
    • The text box becomes a wikitext editor (without any editing toolbar)
    • The font changes to wikitext layout for both paragraphs (the selected and the non selected one), or for the unchanged text that is now in edit mode
    • The edit icon within the editor disappears. Instead, there appears
      • A checkmark button: When this is clicked, the boxes content is "pre-saved", and displayed as content of the text box. This also resets the editor to being a text box, and at that point the inline diff will disappear (for changed paragraphs).
      • A back arrow button: When this is clicked, a confirmation prompt appears (default OOui Popup) and asks you: "Discard changes? Your text will be reset to what it was when the edit conflict occurred. [cancel] [discard]" (see example for reference.) If the user agrees, the box’s content is reset to what it was when the edit conflict occurred, and back in text box mode (not editor), with the inline diff working again (for changed paragraphs)
  • When the user hits save at the end of the screen to resolve the edit conflict, please use the content of the open edit windows, and not of the previous version

Note
Please use the OOUI-icons, if they look very different to the mock, please talk to @Hanna_Petruschat_WMDE
or @Charlie_WMDE

Mock
Inactive and active edit buttons:

MVP 1.3-choose_a_version--en-6.2.png (516×3 px, 223 KB)

Editor:

MVP 1.3-choose_a_version--en-6.3.png (633×3 px, 246 KB)

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Lea_WMDE renamed this task from Show edit buttons for changed paragraphs - js version (No. 7) to Show edit buttons on text blocks - js version (No. 7) .Jul 5 2018, 8:50 AM
Lea_WMDE updated the task description. (Show Details)
JStrodt_WMDE subscribed.

@Lea_WMDE Will the back arrow also have a checkbox like "Don't notify me again"?

@Hanna_Petruschat_WMDE Do we need a design for the back arrow's confirmation prompt?

Discard changes?
Your text will be reset to what it was when the edit conflict occurred.
[cancel] [discard]

((see example for reference))

I don't think we need an extra mock, but thats also up to the dev developing it. If required we can prepare something. Otherwise your linked exapmle is fine.

Regarding the checkbox to not notify again: from a UX perspective, I think it would be useful to have one.

@JStrodt_WMDE The "don't ask me again if I really want to go back" feature is not part of this story, but could be a seperate ticket.

@thiemowmde and me are planing to split this up into two or more smaller tasks also including browser tests for each task just as hint before working on this.

WMDE-Fisch renamed this task from Show edit buttons on text blocks - js version (No. 7) to Support JS enhanced editing on the merge view (No. 7).Aug 30 2018, 7:21 AM

Change 460022 had a related patch set uploaded (by Andrew-WMDE; owner: Andrew-WMDE):
[mediawiki/extensions/TwoColConflict@master] Add save (checkmark) button

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

Change 460044 had a related patch set uploaded (by Andrew-WMDE; owner: Andrew-WMDE):
[mediawiki/extensions/TwoColConflict@master] Add reset (back) button

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

Change 461417 had a related patch set uploaded (by Andrew-WMDE; owner: Andrew-WMDE):
[mediawiki/extensions/TwoColConflict@master] Implement save/submit functionality in order to resolve conflicts

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

Change 460022 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Add save (checkmark) button

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

@Andrew-WMDE We failed to re-estimate this task after we added it to the sprint. Since you worked on the it mostly, what do you think would be left in regard of story points at the beginning of the week?

Also, since this task was worked on in the last sprint, I will try to take the point reduction into account there for the velocity calculation.

While reviewing the patch related to this ticket and checking the point Your text will be reset to what it was when the edit conflict occurred. I had a few thoughts:

  • When I change the text on the left side and (pre-) save it.
    • The colored diff on the right side might be totally inaccurate but there will still be coloring present.
  • When I change the text on the left side and (pre-) save it.
    • Then switch sides and change text on the right side but press reset.
    • I again get a colored diff on the right side that might be totally inaccurate.

Some ideas on that - although no hard feeling:

  • Should we remove coloring on both sides if changes were made on one side?
  • Should we reset both sides if a reset is done on one side?
  • Should we have a prompt when changing sides and one has already been edited, to (reset the side we "leave"?)
  • Should we keep it as it is?

This is no blocker I just wanted to get these thoughts out there. ( maybe we even talked about these things already and I forgot ) :-)

Change 464506 had a related patch set uploaded (by Andrew-WMDE; owner: Andrew-WMDE):
[mediawiki/extensions/TwoColConflict@master] Add browser tests for the reset button and access the betaPreferencesLink by it's Id

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

Change 461417 abandoned by WMDE-Fisch:
Implement save/submit functionality in order to resolve conflicts

Reason:
Saving the page is already working due to the non-js implementation. - The browser tests in here were extracted:

I583af143188cab965755bf2528131591cc4c55f7

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

The review of this is basically done but still waits for to green light that we can merged translations.

While reviewing the patch related to this ticket and checking the point Your text will be reset to what it was when the edit conflict occurred. I had a few thoughts:

  • When I change the text on the left side and (pre-) save it.
    • The colored diff on the right side might be totally inaccurate but there will still be coloring present.
  • When I change the text on the left side and (pre-) save it.
    • Then switch sides and change text on the right side but press reset.
    • I again get a colored diff on the right side that might be totally inaccurate.

Some ideas on that - although no hard feeling:

  • Should we remove coloring on both sides if changes were made on one side?
  • Should we reset both sides if a reset is done on one side?
  • Should we have a prompt when changing sides and one has already been edited, to (reset the side we "leave"?)
  • Should we keep it as it is?

For now let's keep it as is please. Without any more info how users react to this specific behavior, all changes are equally likely to be better or worse :)

WMDE-Fisch changed the point value for this task from 13 to 5.EditedOct 9 2018, 9:16 AM

Since there were already two closed subtickets before this ticket was added to the sprint and in addition to that patches that already implemented lots of the points I will reduce the points for this sprint to have a more realistic workload estimation.

While reviewing the browser tests for this, I run into a surprising UX issue. According to the specifications above the reset button (back arrow) only appears in edit mode. I totally expected the button to do what every other reset button I have seen so far does: discard the current edit. But it undoes all edits (to the same paragraph) I have done and saved before.

I see two ways to fix this confusion:

  1. Make the button only discard the current, unsaved edit.
  2. Make the button always visible after an edit was made, not only in edit mode. This makes it much more obvious that the button will affect all edits, not only the current unsaved one.

Pinging @Hanna_Petruschat_WMDE.

WMDE-Fisch changed the point value for this task from 5 to 3.

Change 460044 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Add reset (back) button

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

Change 464506 merged by WMDE-Fisch:
[mediawiki/extensions/TwoColConflict@master] Add browser tests for the reset button

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

Can't test, the test server is broken :/

Bildschirmfoto 2018-10-12 um 12.07.01.png (262×1 px, 41 KB)

Can't test, the test server is broken :/

Bildschirmfoto 2018-10-12 um 12.07.01.png (262×1 px, 41 KB)

Fixed, the core master needed to be updated on the tool.

Now I find this a bit confusing behavior:
When the conflict is in reading mode, the radion buttons are very very close together

Bildschirmfoto 2018-10-22 um 11.50.56.png (452×2 px, 200 KB)

In edit mode, their spacing is nice, but the width of both columns is nowhere near the same (which it should, because I want to be able to do line by line comparisons)

Bildschirmfoto 2018-10-22 um 11.51.02.png (554×2 px, 223 KB)

@Lea_WMDE: This sound awkward, almost like the page did not finished loading for you for some reason, and parts of the code have gone missing. I can not reproduce it at https://tools.wmflabs.org/wmde-editconflict-test/core/index.php?title=Spezial:SimulateTwoColEditConflict at the moment, neither with Firefox nor with Chrome. Can you please try again?

Lea_WMDE moved this task from Demo to Done on the WMDE-QWERTY-Sprint-2018-10-09 board.

@thiemowmde now I can't reproduce it anymore either. Weird.

My wild guess is that it appeared due to the kind of text in the diff, so we could probably try to reproduced it with developer console and that text.

Back to the original consern...

While reviewing the browser tests for this, I run into a surprising UX issue. According to the specifications above the reset button (back arrow) only appears in edit mode. I totally expected the button to do what every other reset button I have seen so far does: discard the current edit. But it undoes all edits (to the same paragraph) I have done and saved before.

I see two ways to fix this confusion:

  1. Make the button only discard the current, unsaved edit.
  2. Make the button always visible after an edit was made, not only in edit mode. This makes it much more obvious that the button will affect all edits, not only the current unsaved one.

I agree, this is an unexpected behavior which is why we put in the confirmation pop up. It explains, that everything is set back to the "right after edit conflict happened"-state. I also agree that this is not a user friendly behaovior. ("Who reads these?")
So the actual intention is to give the user the opportunity to set back single changes in the edit but also go back to the original state (mainly to be able to get the diffs highlighted).

@thiemowmde
Before diving into finding to icons to represent these functions: Is it even possible (and ressourcewise doable) to make the destinction between these two states of undo? If so I'm happy to find a solution for that, if not, w would only need to point out more clear, that this undos all edits in this paragraph.

It would be possible to just cancel the current edit, but keep others that have previously been "checked" and pre-saved. But this does not bring the colored diff-highlighting back, which was the whole reason for the "back" button.

We could just hide this "back" button for now, or keep it and wait for the users input. @Lea_WMDE, what do you prefer?

Let's keep it as is right now (aka bringing back the highlighting and resetting it to the original state), and see what user input will tell us

Change 476263 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Fix unescaped HTML injected into conflict resolution interface

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

Change 476263 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Fix unescaped HTML injected into conflict resolution interface

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

Change 476300 had a related patch set uploaded (by WMDE-Fisch; owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@wmf/1.33.0-wmf.6] Fix unescaped HTML injected into conflict resolution interface

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

Change 476300 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@wmf/1.33.0-wmf.6] Fix unescaped HTML injected into conflict resolution interface

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