Page MenuHomePhabricator

Diff: Empty context lines not showing
Closed, ResolvedPublic

Description

When viewing a diff that contains empty context lines (gray), those lines are not shown. This is due to all cells in that row having no content, which reverts the row height to zero. It used to show a thin gray line, but a change in diff.css (table.diff td {padding: 0px;}) made that disappear as well. Example in URL should show an empty line directly above "==Professional acting career==".

Having the following CSS in your own stylesheet restores the empty line:
td.diff-marker {height: 1.5em;}

However, that is a hack; A better solution is not to have rows with all empty cells. At least one cell needs to have content, and all rows start with a cell with a diff marker; these are either empty, or contain a + or - sign. I suggest putting a non-breaking space ( ) in the empty diff marker cells. This will cause the row to have content and force the proper height automatically.


Version: unspecified
Severity: normal
URL: http://en.wikipedia.org/w/index.php?title=Matt_Smith_(actor)&diff=next&oldid=392393612

Details

Reference
bz25697

Event Timeline

bzimport raised the priority of this task from to High.Nov 21 2014, 11:17 PM
bzimport set Reference to bz25697.
bzimport added a subscriber: Unknown Object (MLST).

( ) shoud be ( ).

M8R-cyc3n3 wrote:

yuck. inserting meaningless invisible placeholder content is clearly
the hack. here you would be using brute force to prevent individual
sites and users from customizing the appearance of diff.

recommend immediate closure WONTFIX.

M8R-cyc3n3 wrote:

specifying a reasonable default minimum td height in diff.css would be best, as
this can be over-ridden by anyone who wants to.

for example, see diff view highlighted by browser's "select all" command:
http://i55.tinypic.com/vpiyxd.png

spaces impart specific meaning as seen in the vicinity of the cursor, to wit:
the nbsp on line 652 implies i have vandalized your comment and replaced one
line of it by a single non-breaking space. this is different from replacing
it by a totally blank line.

the diff view should remain faithful to this distinction in all cases.

Perhaps just a min-height: 1.5em; then ?

@Thana: Adding a non-visible character is a common way to force table cells to behave as intended. It prevents nothing in the way customizing its appearence, on the contrary.

Also, you misunderstand where I want to place the space; it should go in the first cell of the row, which is normally occupied by the plus- or minus sign (td.diff-marker), *not* in the cell containing the actual diff-text. So you would see that "unwanted" select-block above and below the minus sign in the most-left column in your example.

@Derk-Jan Hartman: The height is skin-specific, and should match each ckin, that means 1.5em for the modern (sans-serif based) skins and 1em for the classic (serif based) skins. So that is not an ideal solution. By giving the empty cell content, it would scale to the right size automatically for every skin.

Also, it doesn't have to be a   necessarely; it could also be another neutral character (in addition to the plus- and minus sign) denoting a no-change in that line.

I just found out that the unclassed cells (the white space after a deleted line or before a added line) contain this code: <td colspan="2">&nbsp;</td>
So adding a &nbsp; in empty diff-marker cells should not be that controverial.

Thank you! When is this this revision expected to go live? If it going to take a while, I can apply the other fix temporarely.

(In reply to comment #9)

Thank you! When is this this revision expected to go live? If it going to take
a while, I can apply the other fix temporarely.

I have no idea what the current status of code deployment is other than "way behind". However with that said the answer is, probably not going live for a little while (I'd guess a couple months, but these are just random guesses that don't have any bearing on reality).

Given it looks like a small independant change, it might be merged and scaped live quickly :-)

How do I know when a particular revision is live?

At the moment this is the deployment branch: http://svn.wikimedia.org/viewvc/mediawiki/branches/wmf/1.16wmf4/

Any code that is not there, is not deployed. I don't think this will be deployed before 1.17, so it's likely that even when it does get deployed, it will be on a 1.17wmf branch.

Since bug 25725 mentions that the PHP diff engine is no longer used, should this fix not be applied in /trunk/extensions/wikidiff2/wikidiff2.cpp instead?

Created attachment 8748
Patch for wikidiff.cpp

Attached:

Created attachment 8749
Patch for wikidiff2.cpp

Attached:

(In reply to comment #17)

Marking as fixed; see
http://en.wikipedia.org/w/index.php?title=Matt_Smith_(actor)&diff=next&oldid=392393612

It works only because of a local fix, see http://en.wikipedia.org/w/index.php?title=MediaWiki:Vector.css&diff=393682546&oldid=392862614.

Wikidiff2 still needs to be adapted, so reopening and changing component.

Bumping this... no peep in five months. This kinda block bug 33335 (new diff color scheme). With this patch, diffs will finally be 'complete' and all local hacks can be removed once deployed.

Modified patch applied in r107875.