Page MenuHomePhabricator

Phase out legacy error, warning and success classes for usage outside the parser
Open, Needs TriagePublic

Description

These were identified to be one of the few remaining legacy classes.

They are extensively used within MediaWiki and extensions, making deprecation or removal more intensive than other styles in legacy.less.

These currently appear unstyled in the mobile site and will soon appear unstyled in the desktop site.

Consumers relying on styling from MediaWiki should be updated to either

  1. Provide their own styles for these classes via MediaWiki:Common.css/template style rules
.error,
.warning,
.success {
        font-size: larger;
}

.error {
        color: #d33;
}

.warning {
        color: #ac6600;
}

.success {
        color: #14866d;
}
  1. Using the .messagebox,.errorbox,.warningbox,.successbox classes

Identified usages

.error

Usages of .error: https://codesearch.wmcloud.org/search/?q=class%3D%22(%3F%3A%5B%5E%22%5Cs%3E%5D*%5Cs%2B)*%3Ferror%5B%5Cs%22%5D&i=nope&files=&excludeFiles=&repos=

  • Parser/ ParserFunctions (Also Parsoid?) - export <strong class="error"></strong> for parsing errors
    • These aren't going away, they'll be moved instead (T281228), but this shouldn't have any user-facing impact.
  • EditPage::handleStatus - <div class="error"></div> for hooks.

.success

Usages of .success: https://codesearch.wmcloud.org/search/?q=class%3D%22(%3F%3A%5B%5E%22%5Cs%3E%5D*%5Cs%2B)*%3Fsuccess%5B%5Cs%22%5D&i=nope&files=&excludeFiles=&repos=

.warning

Usages of .warning: https://codesearch.wmcloud.org/search/?q=class%3D%22(%3F%3A%5B%5E%22%5Cs%3E%5D*%5Cs%2B)*%3Fwarning%5B%5Cs%22%5D&i=nope&files=&excludeFiles=&repos=

This one is rare.

Event Timeline

error at least is used onwiki. Please do a wider search for the above. Representative search for en.wp. success looks much less used; almost every hit is a false positive. Overall, warning has the fewest hits. All of that however is just one wiki.

@Jdlrobson You marked this for inclusion in Tech News. Not sure I get the context here, so feel free to correct me, but something like this?

The [[:w:en:CSS|CSS]] classes <code>.error</code>, <code>.warning</code> and <code>.success</code> do not work for mobile readers if they have not been specifically defined on your wiki. They will soon not work on desktop. They can be defined in [[MediaWiki:Common.css/template]] instead. You can also use <code>.messagebox,</code> <code>.errorbox,</code> <code>.warningbox</code> or <code>.successbox</code> instead.

Who should act on this?

Who should act on this?

Gadget developers, template editors should review any code which depends on these classes and flag any potential problems. At time of writing we are aiming to remove support for these in all skins by June 1st.

Tacsipacsi added a subscriber: Tacsipacsi.

ParserFunctions not only emits, but also consumes .error: rEPFN includes/ParserFunctions.php:169 (at b31c726461de) finds other ParserFunctions functions returning errors, but not only those, anything else that contains strong.error, span.error, p.error, div.error, like another extension’s/core’s error message, or even a template that uses the same error report mechanism. It’s also true the other way round, e.g. Lua modules may count on PF functions returning an element with class="error" on error. Even changing other extensions/core may break things, so I’m not sure how (or even whether) to proceed here. (Dropping .warning and .success is harmless in this regard.)

By the way, the literal search values were way too strict, I’ve updated them with regexes inspired by the above PF regex. There are no magnitude changes, but it finds for example more than twice as many .error usages as before.

Given how PF functions (I assume that's #iferror without looking at the context) I think that's definitely in the realm of "must keep" error. That might not preclude removing the CSS, but I think it would be an error [heh jokes] if that was removed even after an indication of such general use.

The [[:w:en:CSS|CSS]] classes <code>.error</code>, <code>.warning</code> and <code>.success</code> do not work for mobile readers if they have not been specifically defined on your wiki. They will soon not work on desktop. They can be defined in [[MediaWiki:Common.css/template]] instead. You can also use <code>.messagebox,</code> <code>.errorbox,</code> <code>.warningbox</code> or <code>.successbox</code> instead.

Why is [[MediaWiki:Common.css/template]] suggested? Does it automatically load by system?

Because I misread the comment! It's been fixed.

Given the prevalence, perhaps we're better off moving these styles to the feature content-parser-output for 1.37 to give more time to transfer.

ParserFunctions not only emits, but also consumes .error: rEPFN includes/ParserFunctions.php:169 (at b31c726461de) finds other ParserFunctions functions returning errors, but not only those, anything else that contains strong.error, span.error, p.error, div.error, like another extension’s/core’s error message, or even a template that uses the same error report mechanism. It’s also true the other way round, e.g. Lua modules may count on PF functions returning an element with class="error" on error. Even changing other extensions/core may break things, so I’m not sure how (or even whether) to proceed here. (Dropping .warning and .success is harmless in this regard.)

I think it would help a lot if things that do not need to use class="error" (e.g. AbuseFilter/CentralAuth special pages) can be moved to class="errorbox". Parser output related styles should be handled with more care.

By the way, the literal search values were way too strict, I’ve updated them with regexes inspired by the above PF regex. There are no magnitude changes, but it finds for example more than twice as many .error usages as before.

Thank you. I hadn't managed to get that right, and just used the one that gave me the least false positives.

error at least is used onwiki. Please do a wider search for the above. Representative search for en.wp. success looks much less used; almost every hit is a false positive. Overall, warning has the fewest hits. All of that however is just one wiki.

I wanted to use https://global-search.toolforge.org/, but the amount of false positives is staggering to the point of uselessness. Once I find out how to make it behave, I'll add the results (but I should've mentioned that in the task description).

I think it would help a lot if things that do not need to use class="error" (e.g. AbuseFilter/CentralAuth special pages) can be moved to class="errorbox". Parser output related styles should be handled with more care.

While this should be done where possible, some contexts, like inline errors from various extensions (ParserFunctions named above, also can name Scribunto and Cite) will always require using .error or some other name for it (since .errorbox will not render correctly in a paragraph), so I don’t think removing it is wise. .success and .warning are remnants of (I think?) Special:Preferences not using .warningbox/.successbox before, so they can be removed, but I would like to join voices saying that .error should be kept.

I mean, .error is one line of CSS, I don't think adding it to content-parser-output should hurt. Keeping it wouldn't be a maintenance burden then. Scope it to .mw-parser-output, if we must - that doesn't affect extensions.

My first move would be to change things that can safely use .errorbox and friends.

@Jdlrobson Will Minerva be using content-parser-output? If so, we could just move .error there (font-size: larger optional) and phase out everything else.

Let's track this in a new task. We could use content-parser-output provided we scope the rules. Yes Minerva will be using it too.

Is there anyway to replicate this locally so I can see how they look?

Change 682715 had a related patch set uploaded (by Yaron Koren; author: Yaron Koren):

[mediawiki/extensions/PageForms@master] Replace deprecated "error", "warning" CSS classes

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

Change 682715 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Replace deprecated "error", "warning" CSS classes

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

Let's track this in a new task. We could use content-parser-output provided we scope the rules. Yes Minerva will be using it too.

I've created T281228: Provide styling for parser-outputted .error class in content-parser-output for that and I will re-scope this task to focus on the non-parser parts.

Is there anyway to replicate this locally so I can see how they look?

If you have ParserFunctions installed, just use {{#expr:garbage}}. I haven't found anything that works without it yet. The parser tests suggest that it should be possible, so I'll look in that next.

Mainframe98 renamed this task from Phase out legacy error, warning and success classes to Phase out legacy error, warning and success classes for usage outside the parser.Tue, Apr 27, 9:19 AM
Mainframe98 updated the task description. (Show Details)

Change 682915 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/core@master] Don't use classes error, success and warning outside parser

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

Change 682960 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/extensions/AbuseFilter@master] Don't use p class="success" for success messages

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

Er, should these even be used inside the parser? Like what are they even for that a successbox wouldn't cover? Why have this variation?

Change 682973 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/extensions/CentralAuth@master] Don't use .success and .error for success and error messages

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

Er, should these even be used //inside// the parser? Like what are they even for that a successbox wouldn't cover? Why have this variation?

It's a good question and it depends on who the errors are for. If the errors are meant for readers, then definitely should use errorBox. If they should be considered internal users, and not be shown to readers then they probably need different handling and dedicated styles in the parser.

Er, should these even be used //inside// the parser? Like what are they even for that a successbox wouldn't cover? Why have this variation?

It's a good question and it depends on who the errors are for. If the errors are meant for readers, then definitely should use errorBox. If they should be considered internal users, and not be shown to readers then they probably need different handling and dedicated styles in the parser.

For .error, the parser outputs <p> (and sometimes (through extensions) <strong>) inline, wherever they appear. Using .errorbox (optionally on a <div>) might break the flow.

.warning and .success aren't used in the parser.

Change 682915 merged by jenkins-bot:

[mediawiki/core@master] Don't use classes error, success and warning outside parser

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

Hey @ssastry we are auditing skin styles, and looking to deprecate/repurpose use of the error class and consistently style them across skins. Could you provide some guidance on parser errors (or delegate to somebody that can)? Are these meant for end users?

Currently if I create a parser error I see this in Minerva:

and this in Vector:

Who are these for? Readers? Editors? Developers?

If users, could we make use of the errorbox class and improve the error message to make it more actionable e.g. "There was an error parsing this page. The error is Expression error: Unrecognized word "garbage"." ?

If developers, perhaps these should he hidden by default rather than confuse users?

Good questions.

Hey @ssastry we are auditing skin styles, and looking to deprecate/repurpose use of the error class and consistently style them across skins. Could you provide some guidance on parser errors (or delegate to somebody that can)? Are these meant for end users?
...

Who are these for? Readers? Editors? Developers?

If users, could we make use of the errorbox class and improve the error message to make it more actionable e.g. "There was an error parsing this page. The error is Expression error: Unrecognized word "garbage"." ?
...

If developers, perhaps these should he hidden by default rather than confuse users?

We should chat about it. ( cc @cscott @Arlolra @Sbailey). I am not familiar with what guidance exists for this. But, certain kinds of errors from templates and extensions are only shown on preview (so, editors usually). Although, in some cases, I've noticed bright-red Cite errors are displayed during read views as well -- I suppose the incentive is to get someone to notice and fix them? As for other wikitext markup, there is no such thing as "markup error" right now since the wikitext -> HTML transform generates *some* output. But, we have various lint errors that Parsoid flags for editors to fix up out-of-band.

So, overall, this is probably more of a product / UX level of discussion that need to be had as to how warnings / lints / errors need to be handled in various components (markup, templates, extensions, installed gadgets, JS modules).

At least on the Parsoid output end, certain kinds of errors are flagged in the HTML via https://www.mediawiki.org/wiki/Specs/HTML/2.2.0#Error_handling and we are beginning to consolidate behind that mechanism so different clients can take appropriate actions suitable to their context (reading and editing clients might do different things). We could potentially start stuffing linter errors within some such framework as well.

But, I know I didn't answer the very specific question you asked. I don't think that your typical reader might be able to fix these errors, so that argues for suppressing them or displaying them in differently than they are now. But, that said, there is a case to be made about making broken stuff obvious on a page so someone sees it and fixes it. We will chat about this in one of our meetings, but we welcome any guidance / recommendations the web team might have based on end-user UX considerations. But, whatever the decision, I think it is useful to think about it all coherently across all content producing components (top-levle page markup, templates, extensions, gadgets, JS modules, etc.) so there is more coherent handling for them.

(This was excluded from last Tech News, since it didn't seem to be ready to be announced. Please let me know when it is.)

@Johan this can be announced. The main questions are around the future of this styling for parser errors. That doesnt impact editors making their own decisions about how to handle their scripts/gadget styling.

.error remains available for template authors to mark output as an error report, so this affects ONLY gadget/script authors from now on, NOT template authors, right?

.error remains available for template authors to mark output as an error report, so this affects ONLY gadget/script authors from now on, NOT template authors, right?

Correct. Provided the content is outputted by the parser.

CentralAuth and PageForms call $this->getOutput()->wrapWikiMsg that method is a wrapper for OutputPage::addParserOutputText
With the changes proposed in T279388, we'll change mw-parser-output to mw-body-content and these should work as is. @Mainframe98 does that sound right to you?

CentralAuth and PageForms call $this->getOutput()->wrapWikiMsg that method is a wrapper for OutputPage::addParserOutputText
With the changes proposed in T279388, we'll change mw-parser-output to mw-body-content and these should work as is. @Mainframe98 does that sound right to you?

Only for cases where they use .error - there's some .success usages too. https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/682973 just converts them to .errorbox/.successbox to be consistent.

Ah yes. Thanks for the sanity check !

If users, could we make use of the errorbox class and improve the error message to make it more actionable e.g. "There was an error parsing this page. The error is Expression error: Unrecognized word "garbage"." ?

error can and will appear inline and is for readers/editors to correct usually (Cite is the most usual case but ParserFunctions also when error-ful data is input). Are errorbox styles suitably styled for inline use? (Given the name of box, I guess not.) They certainly don't look like it to me from the snapshot above.

I'd really prefer if parser-generated error classes were namespaced, eg mw-error instead of just error (if we're keeping that one).

While I agree, En.WP alone has 500 pages with custom CSS targeting .error (now I wonder what Monobook and Modern are doing). There's some cases where .mw-ui-vform is prefixing it otherwise most people are making it big, bold, and red, or all 3. (Take as many grains of salt for the fact we don't know how many of those custom pages are for editors who are active.)

The change in class name is trivial for editors with a basic knowledge of what they're doing, of course.

@cscott prefixing all code is on the long list :)
The first step is scoping this rule to .mw-parser-output and addressing any usages outside that one.

For now, any wikitext containing class="error" should continue to work as before. We can reconsider styling and class name later.