Page MenuHomePhabricator

Extra line being added elsewhere on page
Closed, ResolvedPublic

Description

Expected behavior

  1. Go to: https://nl.wikipedia.org/wiki/Overleg_Wikipedia:Verzoekpagina_voor_moderatoren/RegBlok
  2. Post a comment using the Reply tool
  3. Notice your comment is posted to the page without affecting any other part of it

Actual behavior

  1. Go to: https://nl.wikipedia.org/wiki/Overleg_Wikipedia:Verzoekpagina_voor_moderatoren/RegBlok
  2. Post a comment using the Reply tool
  3. ⚠️Notice an extra line is being added elsewhere on the page. Specifically, above the comment beginning with, "Zeker wel: daarom is het account ook buiten gebruik gesteld...." [i]

i. https://nl.wikipedia.org/w/index.php?title=Overleg_Wikipedia:Verzoekpagina_voor_moderatoren/RegBlok&curid=500939&diff=55990506&oldid=55990470

Event Timeline

ppelberg created this task.Apr 10 2020, 9:57 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 10 2020, 9:57 PM

Due to the line starting <small> not being indented, there is a break in the lists and the line starting :::Zeker wel: is the first item in a new list. As we've seen already[1], Parsoid adds newlines before new lists and there is currently no way to prevent this.

  1. This is the same as the bug that always adds a newline after the original comment:
Original comment ~~~~
<empty>
:First reply ~~~~
::Second reply ~~~~

and the bug that adds a newline after a section if the first comment is indented:

=== Section ===
: Comment ~~~~

becomes:

=== Section ===
<empty>
: Comment ~~~~
cscott added a subscriber: cscott.Apr 21 2020, 4:18 PM

Presumably we need to ensure SOL context, but is there any case where SOL : *won't* trigger a DL unless we add an extra newline?

IE, are we adding the newline as a cheap fix to enable SOL (even if we're already at SOL), or is there a specific reason why two consecutive newlines would be needed?

ssastry added a subscriber: ssastry.May 4 2020, 7:42 PM

This is just the default expectation (wikitext) of whitespace before lists -- specifically this line. See the follow (non-selser) roundtripping behaviour.

[subbu@earth:~/work/wmf/parsoid] echo -e "==h==\n*x" | php bin/parse.php --wt2wt
==h==

*x

So, anytime a list 's first item is edited (either by editing the first list item, or adding a new list item), it inserts a new line before the list even with selser.

Given that wikis probably have expectations around whitespace before lists, we cannot change the default behaviour now, but, we could see if we can make selser smarter. So, the before constraints for ListHandler would need tweaking to be smarter about min-newline-constraints when the list is not a new list.

Change 594286 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] WIP: Don't add unneeded extra newlines before/after existing lists

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

ssastry triaged this task as Medium priority.May 4 2020, 7:48 PM
ssastry added a project: Parsoid.

I uploaded a quick fix for evaluation + testing. That still needs new tests to be added / existing tests to be adjusted. I'll meanwhile sift through the other bugs.

cscott added a comment.May 5 2020, 4:25 PM

@Esanders pointed out that even if we fix the selser issue we'll still have a new line inserted in the first reply make to a comment (which starts a new list). An issue here is that "code style" expectations on Talk pages (no spaces before lists) differ from "code style" expectations elsewhere. Is this significant enough to warrant some sort of "talk page mode"? (My feeling is that it's not enough by itself, but if there are enough other style changes to merit a mode, then this would be an easy thing to add to it.)

Change 594286 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Don't add unneeded extra newlines before/after existing lists

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

@Esanders pointed out that even if we fix the selser issue we'll still have a new line inserted in the first reply make to a comment (which starts a new list). An issue here is that "code style" expectations on Talk pages (no spaces before lists) differ from "code style" expectations elsewhere. Is this significant enough to warrant some sort of "talk page mode"? (My feeling is that it's not enough by itself, but if there are enough other style changes to merit a mode, then this would be an easy thing to add to it.)

If needed, we can fake the "talk page mode" by using the heuristic that ":foo" type lists only every show up on talk pages and so have a different newline heuristic for these lists. But, if that feels like a hack (which it is), we might as well inspect the namespace and do it right.

ssastry moved this task from Needs Triage to Feature requests on the Parsoid board.May 7 2020, 9:33 PM

For now, I moved this to the "Feature requests" column on the phab board since the "bug" part of it has been addressed and we are now discussing whether we want to introduce a "talk page mode" or not.

Change 595599 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/vendor@master] Bump Parsoid to 0.12.0-a12

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

Change 595599 merged by jenkins-bot:
[mediawiki/vendor@master] Bump Parsoid to 0.12.0-a13

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

For testing, here's a minimal page that triggers this problem: https://en.wikipedia.org/w/index.php?title=User:Matma_Rex/sandbox&oldid=956297462 – editing it using VE to change the "c" or add another item after it currently adds an extra newline, this should be fixed when the Parsoid patch is deployed:

ssastry added a comment.EditedMay 12 2020, 3:59 PM

If you move it / copy it to mediawiki.org, you can test it after group0 rolls out today instead of waiting till group2.

Ryasmeen edited projects, added Verified; removed Editing QA.May 26 2020, 8:13 PM
ppelberg closed this task as Resolved.Jun 5 2020, 1:36 AM
ppelberg claimed this task.