Page MenuHomePhabricator

Section edit preview doesn't let you preview references defined outside the section being previewed
Open, LowPublic

Description

Spun-off from: T7984: Edit preview doesn't let you preview cite.php footnotes.

The behavior if the reference is defined outside the section edited is quite disappointing; A warning like <ref> tag with name $1 cannot be previewed because it is defined outside the current section or not defined at all.

This basically occurs in two cases:

  1. defined inside <references></references>
  2. defined anywhere else outside the section that is edited

Suggestions that have come up so far to deal with this problem:

  • People can use Anomie's ajaxpreview.js (T7984#105262)
  • Adding a "Preview full article" option to section previews (T7984#105265)
  • Parsing the whole page AND the section and use the references from the page parse, to "fill in the blanks" of the section edit preview. (Disadvantage: performance impact, whereas performance is an oft heard reason to use section editing)
  • Use a previously cached version of the references of the current version to amend the context for the section preview. (T7984#1962535). Disadvantage, the 'current' version is not necessarily the version you are editing).

Event Timeline

TheDJ created this task.Jan 26 2016, 10:19 PM
TheDJ assigned this task to MGChecker.
TheDJ raised the priority of this task from to Low.
TheDJ updated the task description. (Show Details)
TheDJ added a project: Cite.
TheDJ added a subscriber: TheDJ.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 26 2016, 10:19 PM
TheDJ removed MGChecker as the assignee of this task.Jan 26 2016, 10:21 PM
TheDJ set Security to None.
TheDJ added a subscriber: MGChecker.
Restricted Application added a subscriber: Luke081515. · View Herald TranscriptJan 26 2016, 10:21 PM
TheDJ renamed this task from Section edit preview doesn't let you preview references defined outside the sectioning being previewed to Section edit preview doesn't let you preview references defined outside the section being previewed.Jan 26 2016, 10:22 PM
TheDJ updated the task description. (Show Details)
Cenarium added a subscriber: Cenarium.EditedJan 27 2016, 7:19 AM

@Cenarium, I think such an approach would be a nice general solution, but I don't think page_props is particularly suited for this purpose. Perhaps a memcached backed cache + restbase api in front of it ?

Regarding page_props, it looks like the graph extension had the same size issue in T100942, and TemplateData in T53740. It was mostly fixed by compressing the json. Could this be done here ? I've looked at some of the most pathological enwiki articles, and there are at least a couple dozen articles whose gz-compressed json-encoded mRefs (member variable of the Cite class) are in excess of 64 KB. So indeed, it might not work here.
I'm not familiar with the restbase API. Memcache could be a good idea: each ref could be stored in its own cache identified by the article id and the ref key, or in a single cache of all refs for the article id.

And even then, in the context of section editing, how would you deal with the problem of synchronizing the pre and post edited version of it ?

Refs from the edited section as retrieved by the preview parser would be checked first, and only when one has missing text the backed-up ref would be fetched.
The only potential issue I can think of is if a ref that was defined in the section gets removed but is still called in the section: there would be no error in the preview (only after save). But it is anyway inadvisable to remove a named ref definition in section editing since it might be used elsewhere in the article.

Cenarium claimed this task.Jan 28 2016, 8:06 PM

Actually, I think that page_props is a good solution.

The size limit can be circumvented by breaking the compressed json in several parts, in the rare cases where this is necessary, and it should practically never have more than two parts. So instead of saving in 'references' page_prop, it's saved in 'references-1' and 'references-2'. This won't have any significant performance impact on page views since we can delay processing to LinksUpdate.

The compressed json can then be retrieved from page_props, reconstructed if necessary. On top of that, we can use a WAN object cache. This can be retrieved in section preview when a reference text is missing, and this could likely be used by MobileFrontend for T123328.

I've started working on a patch.

@Cenarium great! Let me know when you need help or review.

Change 267515 had a related patch set uploaded (by Cenarium):
Show refs defined elsewhere in section preview

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

Izno added a subscriber: Izno.EditedFeb 5 2016, 2:55 PM

Could there be some indication in the rendering of the particular citation defined elsewhere that indeed the citation is defined elsewhere? Basically, the current message (or shortened?) plus the content of the citation. Alternatively, a span with class "ref_defined_elsewhere" or similar would allow me to manipulate CSS to display a notification.

Also, it seems like the functionality requested in this change would make previewing a section where <references/> lives possible. Is that the case?

I could add such a notice. Wouldn't it be better in parenthesis after the reference text ?

Izno added a comment.Feb 5 2016, 4:38 PM

Placement-wise, I have no preference. Before makes it more "obvious" it's defined external to the section, after makes it less "disruptive", which is probably the other reason it would be good to have a span of some sort around the text. <span class="cite-defined-external">Defined in external section.</span> seems about right.

Cenarium added a comment.EditedFeb 5 2016, 8:27 PM

I've added specific classes for each cite warning. In this case, the class is "mw-ext-cite-cite_warning_sectionpreview_outside".

Ltrlg added a subscriber: Ltrlg.Feb 6 2016, 7:55 PM

This will be fixed along T125981.

He7d3r added a subscriber: He7d3r.Feb 9 2016, 12:36 PM
Izno moved this task from Unsorted backlog to Doing on the Cite board.Feb 20 2016, 3:03 AM
daniel added a subscriber: daniel.EditedSep 20 2016, 7:29 PM

Mashing this into page_props strikes me as a bad idea, especially if it needs to be broken up into several parts.

How about this: store the required info in the ParserOutput (with setExtensionData - if this isn't the case already), and then, when previewing a section, fetch current ParserOutput from the ParserCache, grab whatever info is needed for rendering (and missing from the section), and use it to render the citation.

PageProps is *not* a good place for big JSON blobs.

Mashing this into page_props strikes me as a bad idea, especially if it needs to be broken up into several parts.
How about this: store the required info in the ParserOutput (with setExtensionData - if this isn't the case already), and then, when previewing a section, fetch current ParserOutput from the ParserCache, grab whatever info is needed for rendering (and missing from the section), and use it to render the citation.
PageProps is *not* a good place for big JSON blobs.

I agree, actually I made several commits that did that a while ago, including https://gerrit.wikimedia.org/r/#/c/277445.
I've found out though that the best way to go about it (have a fully and efficiently rendered html) is to create a custom parser cache for Cite, since this requires way too much customization (would need plenty of hooks otherwise). This is done in https://gerrit.wikimedia.org/r/#/c/278703 - I got this to work well. It also completely solves the problem of outdated parses.
I've just rebased those, I'd welcome any feedback :)

@Cenarium Looks like it's worth further investigation. The interaction with FlaggedRevisions is somewhat ugly, though. The relevant revisions is always the edit's base revision. That would typically the the current revision, not the last flagged revision, right?

@Cenarium Looks like it's worth further investigation. The interaction with FlaggedRevisions is somewhat ugly, though. The relevant revisions is always the edit's base revision. That would typically the the current revision, not the last flagged revision, right?

The interaction with FlaggedRevs is designed for MobileFrontend, specifically when an unregistered user views a page with flagging enabled, hence the 'stable' revision; it isn't related to section preview. Section preview doesn't even need the separate parser cache, it's only needed for MobileFrontend.

@Cenarium @TheDJ are you going to be at the Barcelona hackathon? If yes, we could try to get this task rolling again?

cscott added a subscriber: cscott.Jul 18 2018, 8:35 PM

Just for reference, see also T146072: Parsoid DOM spec: Improve Cite representation to support section editing & simpler clients. It *may* be that we can collaborate on improving the infrastructure for citations to solve both of our problems at once? (/me dreams)

Izno moved this task from Doing to External on the Cite board.Apr 9 2019, 5:38 PM
Izno moved this task from External to Enhancement backlog on the Cite board.Apr 9 2019, 5:43 PM