MediaWiki r87173 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r87172‎ | r87173 (on ViewVC)‎ | r87174 >
Date:19:49, 30 April 2011
Author:diebuche
Status:reverted (Comments)
Tags:
Comment:
Bug 27047: Nicer design for pre elements in Vector
Modified paths:

Diff [purge]

Loading diff…

Follow-up revisions

Rev.Commit summaryAuthorDate
r89712Revert r87173: adds 'wrap: break-word' which seems opposite to what pre block...brion23:55, 7 June 2011
r91548Redoing r87173 with scrollbars instead of word-wrap per CRdiebuche12:39, 6 July 2011

Comments

#Comment by Fomafix (talk | contribs)   08:53, 1 May 2011

pre { word-wrap: break-word } generates line wrap in Internet Explorer and Opera, but no line wrap in Firefox. General line wrap for pre is bad for source code and for ASCII art.

The author can activate line wrap with <pre style="word-wrap: break-word;/* IE 5.5-7 */ white-space: -moz-pre-wrap; /* Firefox 1.0-2.0 */ white-space: pre-wrap; /* current browsers */"> [1].

A Firefox user can activate line wrap with Toggle Word Wrap [2].

#Comment by DieBuche (talk | contribs)   16:33, 1 May 2011

word-wrap: break-word is CSS3 and supported in FF since version 3.5 and Chrome & Safari since v1.0

#Comment by Fomafix (talk | contribs)   19:24, 1 May 2011

Of course Firefox supports word-wrap: break-word. But pre { white-space: pre-wrap; } is the right CSS3 syntax to wrap lines. With pre { white-space: pre-wrap; word-wrap: break-word; } very long words get also wrapped. To support all browsers you should use

pre {
      word-wrap: break-word;      /* IE 5.5-7 */
      white-space: -moz-pre-wrap; /* Firefox 1.0-2.0 */
      white-space: pre-wrap;      /* current browsers */
}

But wrapping pre generally is bad for source code and ASCII art.

#Comment by DieBuche (talk | contribs)   19:40, 1 May 2011

There obviously are downsides, but imo their weighted up by the pros. A typical MW installation is neither an ASCII art collection nor a code repo. A quickly pasted long URL can easily break the whole layout, and before we used this fix at commons the village pump's layout was broken more often than not. break-word was purposefully since i breaks very long urls as well

#Comment by Fomafix (talk | contribs)   07:15, 4 May 2011

1. (Fixme): pre { word-wrap: break-word; } does not activate text wrapping for pre on all browsers. The correct CSS3 definition to activate text wrapping for pre is pre { white-space: pre-wrap; } [1]. To support all browsers you should use [2]:

pre {
      word-wrap: break-word;      /* IE 5.5-7 */
      white-space: -moz-pre-wrap; /* Firefox 1.0-2.0 */
      white-space: pre-wrap;      /* current browsers */
}

2. (Fixme): This commit activates text wrapping just on skin Vector. The other skins still don't have text wrapping for pre.

3. (Contra): pre is for pre-formatted text to preserve white space. It is especially used for source code and ASCII art. Text wrapping is bad for source code and ASCII art. If you just want to have a text box with border and with text wrapping and without preserving white spaces then don't use pre, use div with a special style or a class.

#Comment by Peachey88 (talk | contribs)   07:20, 4 May 2011
pre {
+	word-wrap: break-word;

Pre's are designed to keep their whitespace and line breaks how they were, we shouldn't be breaking it like this.

#Comment by Fomafix (talk | contribs)   07:47, 4 May 2011

Exactly

#Comment by Nikerabbit (talk | contribs)   08:31, 4 May 2011

Well it kind of isn't broken. Copying still functions like before. Other alternative would be to enable scrollbars.

#Comment by Peachey88 (talk | contribs)   08:39, 4 May 2011

It's broken for people viewing by a screen :p, pre is fundamentally meant to display text how it was contained within the pre tags, we shouldn't be changing really unless we have a very good reason to do it.

#Comment by Nikerabbit (talk | contribs)   08:42, 4 May 2011

Breaking the page layout and having part of the pre text unreadable is quite a good reason imho :)

#Comment by DieBuche (talk | contribs)   08:56, 4 May 2011

Scrollbars look like a good middle-ground between breaking the layout & changing pre formatted text. Compare them here:

http://www.mediawiki.org/wiki/User:DieBuche/pre Current behaviour, note how the page arbitrarily extends to the right, and the text swaps over the pre-borders

http://www.mediawiki.org/wiki/User:DieBuche/pre2 Scrollbars Linebreaks are intact, Layout as well

http://www.mediawiki.org/wiki/User:DieBuche/pre3 Linebreaks get changed, not scrollbars needed though

#Comment by Krinkle (talk | contribs)   02:35, 3 June 2011

I've been using overflow:auto; in my global.css for almost a year now (to unbreak pages with long <pre> contents)

#Comment by Brion VIBBER (talk | contribs)   23:56, 7 June 2011

Reverted in r89712; the word wrapping looks like not what we would want on pre areas, which are meant to exactly preserve the formatting of even very long lines.

Expected 'make it pretty' handling on these is to do things like use overflow & scrollbar settings.

Status & tagging log