Page MenuHomePhabricator

VisualEditor: We need a representation of red-linked images
Closed, ResolvedPublic8 Estimated Story Points

Description

Quote: "I was editing this revision https://en.wikipedia.org/w/index.php?title=Chubby_bunny&oldid=562098358 of [[Chubby bunny]] to try to remove a thumbnail of a deleted image, the following wikitext:
[[Image:Sara_Jay_Chubby_Bunny_Challenge.png|thumbnail|Sara Jay playing Chubby Bunny]]

The red image link and/or its caption shown in the normal article view do not appear in the Visual Editor view and as far as I tell removing it in that view is impossible."


Version: unspecified
Severity: minor

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 1:46 AM
bzimport set Reference to bz50788.
  • Bug 51968 has been marked as a duplicate of this bug. ***

So currently redlink images are returned as <meta typeof="mw:Placeholder">. I see a couple of ways to solve this:

  1. Parsoid gives us back normal image tags, and we make sure our handling of images can deal with 404 src's. This way when we support changing image src's these will be editable in a useful way.
  1. We treat mw:Placeholder as real data, and come up with a rendering for it (maybe using parsewikitextfragment, like templates). We'd need to know what else converts to a mw:Placeholder as this may be undesirable in other situations.

Pinging gwicke as this requires some Parsoid input.

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

(In reply to C. Scott Ananian from comment #5)

There's a draft spec at
https://www.mediawiki.org/wiki/Parsoid/MediaWiki_DOM_spec#Error_handling --
would that work for VE?

It would be reasonable-ish. Having a separate mw:Error type would be quite inconvenient though, we'd have to add it to (almost) all of our node definitions. Would it be OK to drop the mw:Error type and just have the data-mw thing there?

[15:09] <gwicke> RoanKattouw, re https://bugzilla.wikimedia.org/show_bug.cgi?id=50788: the idea is to have a generic way to represent errored content
[15:09] <gwicke> while still matching CSS etc so that it renders as expected
[15:09] <RoanKattouw> Right
[15:09] <RoanKattouw> I guess I could hack tolerance for that into VE's type system
[15:10] <RoanKattouw> I think it's kind of an abuse of a type but I see why you'd do it
[15:10] <RoanKattouw> You want to have a single unified thing to easily identify error nodes
[15:11] <gwicke> yeah; there is a chance that querySelectorAll also supports typeof
[15:12] <gwicke> [typeof~=mw:Error]
[15:12] <RoanKattouw> Exactl
[15:12] <RoanKattouw> y
[15:12] <gwicke> which is more efficient than [data-mw] and then decoding each & look for errors
[15:14] <gwicke> RoanKattouw, I think you are already giving mw:Placeholder a high precedence
[15:14] <gwicke> could mw:Error have a similarly high precedence?
[15:15] <RoanKattouw> gwicke: No, mw:Placeholder is unrecognized
[15:15] <RoanKattouw> Any unrecognized type matching /^mw:/ triggers immediate alienation
[15:15] <RoanKattouw> For mw:Error we'd need the reverse
[15:15] <gwicke> yeah, but aren't you handling typeof="mw:Placeholder mw:Image" already?
[15:15] <RoanKattouw> Which is ignore mw:Error as a type completely, otherwise either everything with mw:Error would be alienated, or everything would have to specify mw:Error
[15:16] <gwicke> (by letting the placeholder win)
[15:16] <RoanKattouw> No, the placeholder doesn't win
[15:16] <gwicke> ah, I see
[15:16] <RoanKattouw> The rules for "special" type prefixes (for ve-mw, that's only /^mw:/ ) are as follows:
[15:16] <gwicke> that's a problem
[15:16] <RoanKattouw> A class matches if:
[15:16] <RoanKattouw> * It specifies some type that's present on the element
[15:17] <RoanKattouw> * It specifies ALL mw: types that are present on the element
[15:17] <RoanKattouw> (i.e. both of those have to be true)
[15:17] <RoanKattouw> Additionally, we don't currently have a facility for saying "I recognize either mw:Foo or mw:Foo+mw:Error"
[15:17] <RoanKattouw> You can only do AND, not OR
[15:17] <gwicke> RoanKattouw: I see, that makes mw:Error harder to handle for you
[15:18] <RoanKattouw> I mean, I can do it
[15:18] <RoanKattouw> By introducing the concept of ignored types or whatever I want to call it
[15:18] <RoanKattouw> I could even make mw:Placeholder one
[15:18] <RoanKattouw> Then placeholderized images could be handled
[15:18] <RoanKattouw> Handlers for node types still have the option to trigger alienation by returning null
[15:18] <gwicke> we already have mw:Image and mw:ExpandedAttrs
[15:19] <gwicke> and all permutations of images of course
[15:19] <RoanKattouw> So if mw:Image+mw:Placeholder meant "alienate except in certain cases", then we could send it to the image handler and do if ( conditions ) { return null; // Alienate
[15:19] <RoanKattouw> Right, mw:ExpandedAttrs is another good candidate for an ignored typer
[15:19] <RoanKattouw> It can apply to anything
[15:19] <RoanKattouw> And sometimes we need to alienate and sometimes we don't
[15:19] <RoanKattouw> Hmm, maybe the jargon should be "universal type"
[15:19] <gwicke> yup, so the general ability to match that kind of thing is needed in any case
[15:20] <RoanKattouw> Yeah
[15:20] <RoanKattouw> I will file a bug about this

Visual editor displays a placeholder for non-existent images, so red-linked images can be deleted from VE.
Checked in production/betalabs/test2.

This task is open but has two "verified-in-phaseX" projects associated. Could someone clarify?

Mooeypoo subscribed.

This is no longer the case. Red link images (or images that aren't loading, in the case of bad internet, etc) are disappearing from CE completely.

We are emitting the mw:Error markup in Parsoid, I believe, and that hasn't changed. What @Mooeypoo reports is a regression in VE?

We are emitting the mw:Error markup in Parsoid, I believe, and that hasn't changed. What @Mooeypoo reports is a regression in VE?

Probably.

The current handling of red-linked images in wmf1.25/1.26

  • the non-existing images can be inserted only in 'Edit source'
  • in Read they are represented as
    Screen_Shot_2015-04-15_at_12.10.08_PM.png (233×1 px, 34 KB)
  • switching to VE
    Screen_Shot_2015-04-15_at_12.10.25_PM.png (428×1 px, 34 KB)
  • hovering over the image gives: 'Sorry, this element can only be edited in source mode for now.'
  • the image can be deleted in VE

Change 210625 had a related patch set uploaded (by SuchetaG):
[WIP] Representation of red-linked images in VE

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

Change 210625 merged by jenkins-bot:
Representation of red-linked images in VE

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