I had a quick look into this. We're going to run into the problem that the current error messages come fully-formed from Score as a CdxMessage - there aren't hooks for us to catch the ScoreException or do anything else with the HTML rendering. Additionally, because the Score exceptions are already rendered in the server-side of the entityview, any modification of the exception rendering needs to happen in PHP and can't be done after-the-fact in the wbui2025 Javascript code. As far as I can see we have the following options:

1. Introduce / use a different SnakFormatter::FORMAT for score values in wbui2025 so that we can have special-case rendering in the Score extension
2. Include the Wbui2025FeatureFlag service from Wikibase as an optional dependency to score and use that to detect that the feature is active, rendering exceptions differently if so
3. Parsing the CdxMessage-generated HTML for Score Exceptions in Wikibase (in the backend, somehow) and extracting the data we need to create the error messages

I think 3 is probably out because it will be brittle to changes in Score. I don't have a strong preference between 1 and 2, but 1 might be the more "sustainable" approach, especially if we run into similar issues in different extensions - we probably don't want to make every extension we touch Wbui2025-aware. Thoughts?

Hi @ArthurTaylor,

when i designed this error it was under the assumption that it would be fairly easy to put the grey box inside the error message... if this is not the case, we could have a meeting, discuss possibilities and come up with a different design option!

Of these options, 1 sounds the best to me. I agree that it makes sense to keep Wbui2025 logic out of random extensions.

That said, Score seems to already include html rendering that is tailored to the Wikibase UI, and that's what we're fighting against. If that's true, it could make sense to clean up Score so that its html output is simpler and more easily plugged into the different UIs. (caveat: I have not deeply investigated Score's output, so these statements could be nonsense)

Additionally, because the Score exceptions are already rendered in the server-side of the entityview, any modification of the exception rendering needs to happen in PHP and can't be done after-the-fact in the wbui2025 Javascript code.

Do we really want to server-render the popover button already? Because I assume that would mean that no-JS users can’t see the error at all.

I would suggest leaving the server-side rendering unchanged, and adding some JS code that pulls out all the error boxes (e.g. anything matching .mw-ext-score-error, or perhaps .cdx-message--error) and wraps them in popover buttons. Depending on the class we choose (and we can change that class in ScoreException::getHtml()), that wouldn’t need to be extension-specific.

@Lucas_Werkmeister_WMDE Depends how bad we want to make that work - https://caniuse.com/mdn-api_htmlelement_popover https://developer.mozilla.org/en-US/docs/Web/API/Popover_API . But there's for sure a question how much engineering we want to do for the NoScript Android mobile users (who anyway won't be able to make edits).

But yeah - let's call parsing the exceptions in Javascript and leaving the server-side version unchanged option 4.

I see your point in option 3 is out because it might break if the Score changes its HTML structure which is a valid concern.

The option 2 I have some concerns about:

• Couples Score to Wbui2025 which is not optimal.
• It will make Score aware of some feature flags it should not know about (other parties that uses Score will not be knowing why/how to use the feature).
• I think it will be harder to maintain on the long term.

Option 1 (recommended). ✅

Alternative or a middle ground: Option 5 (Structured errors data) 💡 :
We can add introduce a new method to ScoreException that returns a structured error data the way we need it so the wbui can use it efficiently, something like:

public function getStructuredError(): array {
    return [
        'message' => $this->getUserFriendlyMessage(),
        'details' => $this->getFullErrorDetails(),
        'type' => $this->getErrorType()
    ];
}

@mahmoud.abdelsattar.wmde Option 4 (5 maybe?) looks good, but how can we access the ScoreException from wbui / Wikibase? Currently (as I understand it) the code in Score just swallows the exceptions and renders the CdxMessage HTML.

@ArthurTaylor From my understanding to how the Score extension works, I think we can introduce a new format like:

// in extensions/Wikibase/lib/includes/Formatters/SnakFormatter.php
const FORMAT_HTML_STRUCTURED = 'text/html+structured';

And with this format, we will be able to return a structured html with error data (if any):

// Add this to extensions/Score/includes/ScoreFormatter.php as a separate function
public function formatWithErrorHandling( $value ) { // or whatever the function name
    $valueString = $value->getValue();
    
    try {
        $html = $this->formatAsHtml( $valueString);
        return [ 'html' => $html, 'error' => null ];
    } catch ( ScoreException $exception ) {
        return [
            'html' => (string)$exception,
            'error' => $exception->getStructuredError() // this was the function I mentioned in my previous comment.
        ];
    }
}

The previous PHP code snippet is just explainign the approach, not the actual implementation.
I also renamed my suggested solution to option 5.

@mahmoud.abdelsattar.wmde thanks for the explanation! That seems fully aligned with (or depends on) option 1, but I agree it makes sense to return the information in a structured way if we can. I'm not sure we can change the interface of formatSnak to return an array instead of just HTML. I was thinking we might need to sneak the structured information into data-error-details attributes of the HTML for the text/html+structured format option.

There’s a slight complication with some of these options. I assumed that the error box came from this catch block:

try {
	$valueHtml = Score::renderScore(
		$valueString,
		$args
	);
} catch ( ScoreException $exception ) {
	return (string)$exception;
}

However, for at least some errors, including invalid LilyPond input (e.g. deleting a }), it actually comes from this catch block, one level down in the stack trace:

} catch ( ScoreException $e ) {
	if ( $parser && $parser->getOutput() !== null ) {
		if ( $e->isTracked() ) {
			$parser->addTrackingCategory( 'score-error-category' );
		}
		self::recordError( $e );
	}
	$html = $e->getHtml();
}

Even ScoreFormatter doesn’t see that this is an error (it just returns the $valueHtml which happens to be error-shaped), and Score::renderScore() itself doesn’t know anything about Wikibase. So if we wanted to return more detailed error information, I guess we’d need to refactor this first.

Change #1224980 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] POC: Option 4 for Score LilyPond errors

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

I uploaded a proof of concept for option 4 – looks like this at the moment:

Based on a quick discussion with Alice, we can actually reuse the constraints indicator for this, stepping through both the LilyPond error and any potential constraint violations (see also T414193).

Also, the popover will be affected by T414495: [MEX][QC] Constraint violations popover is invisible on musical notation snaks.

In case the music notation has an error, a generic error message should be showing "Unable to display the value".

Note: Wikibase already has a translated message “The value is invalid and cannot be displayed.” However, since in this case it’s not certain that the value is invalid (it’s also possible that Lilypond score rendering is temporarily unavailable, I think), it seems okay to add a new message for this case.

Change #1228436 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/vendor@master] Bump wmde/php-vuejs-templating to 2.2.0-beta.9

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

Change #1228436 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wmde/php-vuejs-templating to 2.2.0-beta.9

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

Change #1230966 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] POC: Make popover contents a list of refs of lists

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

Change #1234337 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] WIP: Display LilyPond errors in popover

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

Change #1234337 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Display LilyPond errors in popover

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

Change #1224980 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@master] WIP: Display LilyPond errors in popover

Reason:

superseded by Ibd71713995

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

Change #1230966 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@master] POC: Make popover contents a list of refs of lists

Reason:

superseded by Ibd71713995 (I think sooner or later a reactive version of the constraint indicators would still be useful, but we can figure that out later)

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

Change #1242357 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Score@master] Update Cypress test for Wikibase change

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

Change #1242357 merged by jenkins-bot:

[mediawiki/extensions/Score@master] Update Cypress test for Wikibase change

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

Some issues were found during the inspection (mostly related to UX):

• The popover doesn't have a heading text (well, it has, it but doesn't show up) .. tested locally and test wikidata.
• The popover error block should not have the error icon, it should be placed in the header to match the task Figma designs (low priority).
• The width of the popover shows incorrect alignment (discussed on the task T417645) and might fall in the other task's scope.

Screenshot:

Will move it back to "Ready for development"

Yes, we’re just showing the whole error block from the Score extension without “dissecting” it in any way. I think we discussed this in some story time meeting and agreed that it was okay? But I’m not sure… maybe something to discuss when Alice comes back.

@Lucas_Werkmeister_WMDE Yes, you are right .. I remember it (I thought it was only in my head since it was not mentioned in the description), I've edited my comment to discard this one.

My comment also applies to the first bullet point – the “intended” heading text, “Unable to compile LilyPond file”, is also part of the error block from the Score extension. I don’t think we should just duplicate it.

I thought the heading text should be "Issues", and the sub-heading "Unable to compile LilyPond input file:" could be part of the error message.
That's from the screenshots provided in the task, not sure if is it outdated or valid.

I think the Figma design and screenshots are generally outdated following our decision to display the Score error as a full block – I’d prefer to hear from Alice first if this is okay or not before adding a title.