Page MenuHomePhabricator

wikidiff2 creating ins and del elements with single empty character element
Closed, ResolvedPublic2 Story Points

Description

When viewing http://reading-web-staging.wmflabs.org/wiki/Special:MobileDiff/9?useformat=mobile#movedpara_1_1_lhs and working on T197491 I noticed the presence of

<div class="mw-diff-inline-deleted"><del>&nbsp;</del></div>

Since mobile diffs are by word, this character is problematic as it looks peculiar and like a bug. I believe it relates to a new line.

The presence of this seems to always be a sibling element of mw-diff-inline-moved and is problematic

Can the ins element be removed from the markup or marked up with a class so we can style it differently?

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterEmpty lines take up full width in diff view
mediawiki/php/wikidiff2 : masterMark empty lines as "​mw-diff-empty-line" in InlineDiff
mediawiki/php/wikidiff2 : masterMark empty lines as "​mw-diff-empty-line" in InlineDiff

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 19 2018, 11:05 PM

@jkroll @thiemowmde @WMDE-Fisch any thoughts?
@Jdlrobson changing this would take a few days (or rather weeks), since we will have to build the wikidiff2 library again and roll it out on production. Or @jkroll @thiemowmde @WMDE-Fisch could we provide changes to them in a faster way?

I'm not so sure these non-breaking spaces have anything to do with any of the changes we made recently. Random examples:

As far as I can tell these spaces represent an otherwise empty line that was removed or added. In the example screenshot above, there was actually an empty line removed (the red &nbsp; at the top), and one added (the green &nbsp;). I don't think we can remove these.

It's probably possible to add a dedicated CSS class for empty paragraphs, but I would like to first understand what exactly is "problematic" about these?

Jdlrobson added a subscriber: alexhollender.

@alexhollender thoughts on this? Is it an issue?

@thiemowmde from my POV https://en.m.wikipedia.org/wiki/Special:MobileDiff/846896818 doesn't seem as problematic as it tells me a blank line was replaced with some text, but in this case when I parse the diff I see the line was moved down below and I'd expect it to be part of the moved block.

In an ideal world, I guess new lines would be marked in the diff so that we can style them appropriately e.g. width 100%

The algorithm we introduced recently can detect moved lines, but does not know anything about "moved blocks". It does not and currently can't do anything about empty lines, because every empty line would match every other empty line. I understand that an intelligent human being looking at a diff like the one in the screenshot can find many, many more patterns the current diff algorithm can't find. This is certainly the case here. But this is not new and – to my knowledge – not in the scope of our current German-Community-Wishlist project T139603.

Can we please take a step back and clarify what exactly the problem is?

@thiemowmde thanks for looking into this. I understand now, that this is nothing new we introduced.

@Jdlrobson I am afraid we really can't do anything about how empty lines are being treated. We actually attempted to improve empty line handling by adding it to the next paragraphs already (see our report here), but with no success.
Since we ran into many situations in the past months where we thought "this should be done differently!" and then this seemingly easy change turned into a big source of trouble, and in the end we had to abort - because of this we are a bit hesitent now with taking on "easy" improvements of wikidiff2.

i am afraid we really can't do anything about how empty lines are being treated

I'm not sure i understand why this isn't easy. Given you are already adding a nbsp it seems trivial to add a class to this line which will empower us to either make it 100% width or change the color in css. Surely this is possible? If not can you go into technical details about why. This seems like a trivial change to me.

To be clear all we would need to handle this is:

<div>&nbsp;</div> => <div class=emptyline>&nbsp;</div>

i am afraid we really can't do anything about how empty lines are being treated

I'm not sure i understand why this isn't easy. Given you are already adding a nbsp it seems trivial to add a class to this line which will empower us to either make it 100% width or change the color in css. Surely this is possible? If not can you go into technical details about why. This seems like a trivial change to me.

I think @thiemowmde 's answer was more directed to this somewhat request in the comment before:

I see the line was moved down below and I'd expect it to be part of the moved block.

I am not entirely sure right now, but I think it's doable to add a specific class to these lines. What's not easily possible is, to do move detection on them or treating them as part of the upper block. We already went down that road with two exploratory patches and were not very successful.

Jdlrobson added a comment.EditedJun 27 2018, 2:03 PM

Got it with regards to merging the blocks but yes a class on the elements wrapping those new lines would give us the flexibility we need to address this! Please let me know and I can wrap up my styling changes.

Change 442307 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/php/wikidiff2@master] Mark empty lines as "diff-empty" in both TableDiff and InlineDiff

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

FYI In case this helps with prioritisation, this is blocking T197491 in case that wasn't clear.

We'd like to style the new lines differently when they appear above the moved paragraph, something very possible with CSS, but not possible in current form (unless we resort to JavaScript hacks which are obviously not ideal).

Vvjjkkii renamed this task from wikidiff2 creating ins and del elements with single empty character element to umaaaaaaaa.Jul 1 2018, 1:03 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from umaaaaaaaa to wikidiff2 creating ins and del elements with single empty character element.Jul 2 2018, 1:17 PM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.

Hi @Jdlrobson, @alexhollender , @ovasileva , sorry for the delay in answering.

I'm still not sure if we are all talking about the same thing. So let me try to phrase how I understand the situation right now:

  • We changed wikidiff2 so that paragraph moves are indicated. We did not change anything about newline creation or deletion. This means we are not adding any &bsp to the code.
  • Since it often happens that people delete and add empty lines when they move a paragraph, it happens that diffs look like the one in the task description. However, there is no guarantee that the addition and removal of newlines is semantically connected to the move. E.g. see this example.

Do I understand it right that with the new "moved paragraph" style the empty line indicators are not that visible anymore, and this is why you want to extend all of them to 100% width, independent of whether they are next to a move paragraph or not?

Change 444756 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Empty lines take up full width in diff view

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

@Lea_WMDE - we met yesterday to clear up some of the confusion. How does the following sound for next steps:

  • we add the class for empty lines and split off a separate task for addressing those (potentially post-deployment)
  • you move forward with the deployment as planned

To reiterate, we'll probably make some styling changes to empty lines in the short term to make them look a bit more cohesive with moves, but we don't consider those blockers to deployment of the moved paragraphs.

phuedx added a subscriber: phuedx.Jul 10 2018, 10:13 AM

@Jdlrobson gave additional context for T197729#4410014 in the parent task:

I've also written https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/444756/ to capture why T197729 would be helpful.
@ovasileva @alexhollender and I are chatting separately about options including revisiting the color choices separately.

AIUI @Jdlrobson's change shouldn't block any deployments on WMDE's side and is meant as an experimental base to tackle the design for this task in future.

@ovasileva sounds good! We'll finish off this patch and will prepare deployment then :)

WMDE-Fisch set the point value for this task to 2.
WMDE-Fisch assigned this task to jkroll.Jul 17 2018, 1:16 PM
WMDE-Fisch moved this task from Sprint Backlog to Doing on the WMDE-QWERTY-Sprint-2018-07-17 board.

Change 446745 had a related patch set uploaded (by WMDE-Fisch; owner: Thiemo Kreuz (WMDE)):
[mediawiki/php/wikidiff2@master] Mark empty lines as "​mw-diff-empty-line" in InlineDiff

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

Change 442307 abandoned by WMDE-Fisch:
Mark empty lines as "​mw-diff-empty-line" in InlineDiff

Reason:
Non WIP version here:
I7726c290b7dc94e8bbc5c9ea26c4c7ebe0288483

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

Change 446745 merged by jenkins-bot:
[mediawiki/php/wikidiff2@master] Mark empty lines as "​mw-diff-empty-line" in InlineDiff

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

We finished the patch and it's merged and deployed here:
https://wmde-wikidiff2-mobile.wmflabs.org/core/index.php/Special:MobileDiff/14626

The now current version 1.7.2 needs to be tagged and released now so we can schedule the deployment. I poked @Legoktm already if he would be so kind to handle the release part.

Ok that was quick, release is there deployment will be tracked in T199801 so if understood the conversation here correctly @ovasileva deploying the wikidiff2 version that adds the CSS classes to empty lines is not blocked by anyone from your side.

( I assume there are no styles deployed yet that would actually make use of these classes and so nothing needs to be communicated in that regard )

Sorry for asking again, just want to make double sure before giving the final go to deploy that new version.

@ovasileva and team will be working with what we implemented here, so there is no blocking from their side (it's rather the other way round ;) ) So we should deploy :)

Change 444756 abandoned by Jdlrobson:
Empty lines take up full width in diff view

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

perfect! thanks!

Awesome! Looks great. Shall we resolve this task? We can do the rest as part of T199307

Lea_WMDE closed this task as Resolved.Aug 3 2018, 6:47 AM

Done :)