Page MenuHomePhabricator

Balance diff display more evenly
Closed, ResolvedPublic

Description

Author: contactbox

Description:
The column widths applied to DIFF need addressing.

Sometimes one DIFF column is 40 pixels and the other 800+.


Version: 1.4.x
Severity: minor
OS: other
Platform: PC

Details

Reference
bz1229

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 8:07 PM
bzimport set Reference to bz1229.
bzimport added a subscriber: Unknown Object (MLST).
  • Bug 1438 has been marked as a duplicate of this bug. ***

ui2t5v002 wrote:

This really needs to be fixed. It makes diff pages completely unusable.

Here is a short discussion:

http://en.wikipedia.org/w/index.php?title=Wikipedia:Village_pump_%28technical%29&oldid=14043658#Wiiiiiiide_diff_pages

Four proposed solutions:

  • Leave it the way it is - Unacceptable
  • Put the long strings in boxes with automatic scrollbars that only appear with

wide text - My favorite, if implemented for each td /td separately - related to
Bug 414

  • Break long text strings (which slashdot does, to avoid page-widening attacks)
  • My second favorite
  • Clip long strings to the rect of the two diff columns - Destroys information

Good examples:

http://en.wikipedia.org/w/index.php?title=RTFM&diff=12625368&oldid=12536687
http://en.wikipedia.org/w/index.php?title=Nikola_Tesla&diff=0&oldid=13515391

ui2t5v002 wrote:

Ok, I think that all you have to do to fix this for Firefoxers is change some
lines in DifferenceEngine.php:

  1. HTML-escape parameter before calling this

function addedLine( $line ) {

		return "<td>+</td><td class='diff-addedline'>{$line}</td>";

}

  1. HTML-escape parameter before calling this

function deletedLine( $line ) {

		return "<td>-</td><td class='diff-deletedline'>{$line}</td>";

}

  1. HTML-escape parameter before calling this

function contextLine( $line ) {

		return "<td> </td><td class='diff-context'>{$line}</td>";

}

Should become:

  1. HTML-escape parameter before calling this

function addedLine( $line ) {

		return "<td>+</td><td class='diff-addedline'><div

class="diffblock">{$line}</div></td>";
}

  1. HTML-escape parameter before calling this

function deletedLine( $line ) {

		return "<td>-</td><td class='diff-deletedline'><div

class="diffblock">{$line}</div></td>";
}

  1. HTML-escape parameter before calling this

function contextLine( $line ) {

		return "<td> </td><td class='diff-context'><div

class="diffblock">{$line}</div></td>";
}

Or class="diffparagraph" or something like that. Then add:

.diffblock {overflow: auto;}

to the css. I don't have a way to try this myself.

ui2t5v002 wrote:

(In reply to comment #4)

Ok, I think that all you have to do to fix this for Firefoxers is change some
lines in DifferenceEngine.php:

Apparently DIVs inside TDs is bad HTML.

Oh well.

  • Bug 2992 has been marked as a duplicate of this bug. ***

ui2t5v002 wrote:

(In reply to comment #5)

Apparently DIVs inside TDs is bad HTML.

I have now been told that DIVs inside TDs is perfectly valid HTML:

http://www.w3.org/TR/html4/sgml/dtd.html

To test Omegatron's proposed solution, you can use the following in your user
js: http://en.wikipedia.org/w/index.php?title=User:CesarB/monobook.js&oldid=19891279

For some reason, it does not work always; some extreme cases where it fails
badly are at http://en.wikipedia.org/wiki/User_talk:CesarB/monobook.js (the
first one is particularly bad).

zigger wrote:

*** Bug 4494 has been marked as a duplicate of this bug. ***

ui2t5v002 wrote:

The javascript workaround rewrites the diff display in css instead of a table,
and is known to work in Firefox, Konqueror, Opera and at least some versions of
Internet Explorer.

I'd like it if Mediawiki output code like this instead of the table it currently
outputs. It just has to be changed to output spans and divs instead of the tds
it uses now.

Also, using tables for visual layout is bad. Just ask a google search.

I'm going to change the Severity away from enhancement, since the super wide
pages can be considered a bug.

Hmmm... diffs might qualify as a proper usage of tables, though; after all, the
contents of each line are data, and we're not just putting it in the table to
make it look nice. (We can't say the same for so many other uses of tables at
Wikipedia.) One thing I'll miss about the table-based diffs is the ability to
Ctrl-click a cell and copy the contents of an entire line to the clipboard.
There's really no easy way of doing that with CSS.

ui2t5v002 wrote:

(In reply to comment #13)

Hmmm... diffs might qualify as a proper usage of tables, though; after all, the
contents of each line are data, and we're not just putting it in the table to
make it look nice. (We can't say the same for so many other uses of tables at
Wikipedia.)

You're right. With css turned off it just becomes a mess of text. Is there any
way to use tables but still render correctly?

gangleri wrote:

(In reply to comment #10)

javascript workaround here:

http://en.wikipedia.org/wiki/Wikipedia:WikiProject_User_scripts/Scripts/Fix_diff_width

Thanks Omegatron for this note about the workaround!

However the colors for added / deleted do not show properly at
http://yi.wiktionary.org/w/index.php?title=%D7%91%D7%90%D6%B7%D7%A0%D7%99%D7%A6%D7%A2%D7%A8:Gangleri/monobook.js&diff=prev&oldid=7478
using Windows 2000 SP 2, FF 1.5 and IE 6.0
but they show properly at
http://yi.wiktionary.org/w/index.php?title=%D7%91%D7%90%D6%B7%D7%A0%D7%99%D7%A6%D7%A2%D7%A8:Gangleri/monobook.js&diff=next&oldid=7478

best regards reinhardt [[user:gangleri]]

ui2t5v002 wrote:

(In reply to comment #15)

However the colors for added / deleted do not show properly at

I didn't write it. :-) Ask on the Wikiproject page.

I will ask both there and here:

Is there a way to get this formatting at the same time as the table markup?

gangleri wrote:

(In reply to comment #16)
Thanks Omegatron!


Please see also bug 4801 comment 0
Bug 4801: user:foo/monobook.js does not render in preview

re: Bug 1229: Please change "OS" and "Hardware" if you experience this behaviour
at other
operating systems and / or hardware. Thanks in advance!

best regards reinhardt [[user:gangleri]]

gangleri wrote:

*addendum*

RTL wikies should show the current version on the left side; see
http://yi.wiktionary.org/w/index.php?title=user:Gangleri/monobook.js&diff=7485&oldid=7479

Behaviour depends on the wikies. Please make the tests at commons:, meta, b:,
n:, s:, q:, wikt; w:.

Thanks in advance!

best regards reinhardt [[user:gangleri]]

(In reply to comment #14)

You're right. With css turned off it just becomes a mess of text. Is there any
way to use tables but still render correctly?

I haven't found one, though I just realized that it might be possible with some
creative application of display:block. I'll have to test it to see if browsers
will actually support it.

With the current version, turning off CSS will put each cell on one line, making
the result look almost like a unified context diff (diff -u) except that each
context line is duplicated. I haven't found any way around that either, but at
least it should be more or less readable in browsers like lynx.

(In reply to comment #18)

RTL wikies should show the current version on the left side; see

http://yi.wiktionary.org/w/index.php?title=user:Gangleri/monobook.js&diff=7485&oldid=7479

The columns can easily be swapped by setting the float property of .xdiff-col to
"right". However, the signs will still be on the left side of the columns. I
think I know how to fix it, but it requires changing the DOM structure again.
I'll try to address the issue in the next revision; ideally changing the display
from LTR to RTL should be as easy as uncommenting one line of CSS.

(In reply to comment #19)

(In reply to comment #14)

You're right. With css turned off it just becomes a mess of text. Is there any
way to use tables but still render correctly?

I haven't found one, though I just realized that it might be possible with some
creative application of display:block. I'll have to test it to see if browsers
will actually support it.

With the current version, turning off CSS will put each cell on one line, making
the result look almost like a unified context diff (diff -u) except that each
context line is duplicated. I haven't found any way around that either, but at
least it should be more or less readable in browsers like lynx.

Still, tables seem like the most appropriate way to handle diffs, and it's a
pity that we have to come with workarounds for functionality that tables already
offer (albeit with annoyingly uneven columns).

ui2t5v002 wrote:

(In reply to comment #21)

Still, tables seem like the most appropriate way to handle diffs, and it's a
pity that we have to come with workarounds for functionality that tables already
offer (albeit with annoyingly uneven columns).

Ilmari, did you see my first implementation where the DIVs were inside TDs? I
don't know if it's a "proper" way to do it, and it got slow on mine on large
pages, but it *did* work in Firefox, and combined the logicalness of tables with
the 50% width of css formatting... Just a thought. I'm not a CSS guru or anything.

smjg wrote:

(In reply to comment #11)

The javascript workaround rewrites the diff display in css instead of a table,
and is known to work in Firefox, Konqueror, Opera and at least some versions of
Internet Explorer.

I'd like it if Mediawiki output code like this instead of the table it currently
outputs. It just has to be changed to output spans and divs instead of the tds
it uses now.

Also, using tables for visual layout is bad. Just ask a google search.

This isn't using tables for visual layout. It's a perfectly legitimate use of
tables for marking up tabular data. Each row denotes a paragraph that has
changed (or is next to one that has), and each column denotes a version of the
document.

And while I'm at it, I shall remind you all that this bug is about the widths of
the diff columns relative to each other. One erroneous dupe seems to have made
the topic of discussion drift....

r22204 for bug 1438 will have fixed this as well, using fixed table layout and
forcing overflow to scroll.

  • Bug 6798 has been marked as a duplicate of this bug. ***