Page MenuHomePhabricator

<pre> text displays contents in the surface without the newlines, which is confusing.
Closed, ResolvedPublic1 Story Points

Description

Preformatted text is no longer directly editable, displays without newlines and opens an inspector for editing.

Example pages:

Read mode:


Visual editor:

Event Timeline

matmarex created this task.Feb 28 2017, 3:03 PM
Restricted Application added a project: VisualEditor. · View Herald TranscriptFeb 28 2017, 3:03 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

@matmarex re T159231: <pre> text displays contents in the surface without the newlines, which is confusing. - I thought the inspector is the new feature for editing pre-formatted text.

No, it's ve.ui.MWExtensionInspector. I guess it falls back to that for some reason?

I wonder if Parsoid output for <pre> in wikitext changed? I don't know how used to look like, but it looks like this now (copied from https://en.wikipedia.org/api/rest_v1/page/html/GIF?redirect=false):

<pre typeof="mw:Extension/pre" about="#mwt151" data-mw='{"name":"pre","attrs":{},"body":{"extsrc":"\n                          Table           9-bit\n                     string --> code      code    Action\n                          #0 | 000h               Initialize root table of 9-bit codes\n                    palette  |  :\n                     colors  |  :\n                        #255 | 0FFh\n                         clr | 100h\n                         end | 101h\n                             |            100h     Clear\nPixel          Local         |\ncolor Palette  string        |\nBLACK  #40     28            |            028h     1st pixel always to output\nWHITE  #255    FF            |                     String found in table\n                  28 FF      | 102h                Always add 1st string to table\n               FF            |                     Initialize local string\nWHITE  #255    FF FF         |                     String not found in table\n                             |            0FFh     - output code for previous string\n                  FF FF      | 103h                - add latest string to table\n               FF            |                     - initialize local string\nWHITE  #255    FF FF         |                     String found in table\nBLACK  #40     FF FF 28      |                     String not found in table\n                             |            103h     - output code for previous string\n                  FF FF 28   | 104h                - add latest string to table\n               28            |                     - initialize local string\nWHITE  #255    28 FF         |                     String found in table\nWHITE  #255    28 FF FF      |                     String not found in table\n                             |            102h     - output code for previous string\n                  28 FF FF   | 105h                - add latest string to table\n               FF            |                     - initialize local string\nWHITE  #255    FF FF         |                     String found in table\nWHITE  #255    FF FF FF      |                     String not found in table\n                             |            103h     - output code for previous string\n                  FF FF FF   | 106h                - add latest string to table\n               FF            |                     - initialize local string\nWHITE  #255    FF FF         |                     String found in table\nWHITE  #255    FF FF FF      |                     String found in table\nWHITE  #255    FF FF FF FF   |                     String not found in table\n                             |            106h     - output code for previous string\n                  FF FF FF FF| 107h                - add latest string to table\n               FF            |                     - initialize local string\nWHITE  #255    FF FF         |                     String found in table\nWHITE  #255    FF FF FF      |                     String found in table\nWHITE  #255    FF FF FF FF   |                     String found in table\n                                                   No more pixels\n                                          107h     - output code for last string\n                                          101h     End\n"}}' id="mwASw">                          Table           9-bit
                     string --> code      code    Action
                          #0 | 000h               Initialize root table of 9-bit codes
                    palette  |  :
                     colors  |  :
                        #255 | 0FFh
                         clr | 100h
                         end | 101h
                             |            100h     Clear
Pixel          Local         |
color Palette  string        |
BLACK  #40     28            |            028h     1st pixel always to output
WHITE  #255    FF            |                     String found in table
                  28 FF      | 102h                Always add 1st string to table
               FF            |                     Initialize local string
WHITE  #255    FF FF         |                     String not found in table
                             |            0FFh     - output code for previous string
                  FF FF      | 103h                - add latest string to table
               FF            |                     - initialize local string
WHITE  #255    FF FF         |                     String found in table
BLACK  #40     FF FF 28      |                     String not found in table
                             |            103h     - output code for previous string
                  FF FF 28   | 104h                - add latest string to table
               28            |                     - initialize local string
WHITE  #255    28 FF         |                     String found in table
WHITE  #255    28 FF FF      |                     String not found in table
                             |            102h     - output code for previous string
                  28 FF FF   | 105h                - add latest string to table
               FF            |                     - initialize local string
WHITE  #255    FF FF         |                     String found in table
WHITE  #255    FF FF FF      |                     String not found in table
                             |            103h     - output code for previous string
                  FF FF FF   | 106h                - add latest string to table
               FF            |                     - initialize local string
WHITE  #255    FF FF         |                     String found in table
WHITE  #255    FF FF FF      |                     String found in table
WHITE  #255    FF FF FF FF   |                     String not found in table
                             |            106h     - output code for previous string
                  FF FF FF FF| 107h                - add latest string to table
               FF            |                     - initialize local string
WHITE  #255    FF FF         |                     String found in table
WHITE  #255    FF FF FF      |                     String found in table
WHITE  #255    FF FF FF FF   |                     String found in table
                                                   No more pixels
                                          107h     - output code for last string
                                          101h     End
</pre>

Yeah, this looks suspicious: rGPAR79ccfb9372cb: Treat html pre and nowiki as extension tags

Our ve.dm.MWPreformattedNode (actually ve.dm.PreformattedNode) only specifies to match <pre> tags:

ve.dm.PreformattedNode.static.matchTagNames = [ 'pre' ];

And apparently ve.dm.MWExtensionNode takes priority over that with:

ve.dm.MWExtensionNode.static.getMatchRdfaTypes = function () {
	return [ 'mw:Extension/' + this.extensionName ];
};

So, I guess we should make ve.dm.MWPreformattedNode match 'mw:Extension/pre' explicitly?

(But we still need to match a plain <pre> tag too. Indented code still uses that. https://en.wikipedia.org/w/index.php?title=User:Matma_Rex/sandbox&oldid=768064102)

matmarex claimed this task.Mar 1 2017, 4:08 PM

Change 340554 had a related patch set uploaded (by Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor] ve.dm.MWPreformattedNode: Update matching for Parsoid changes

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

Change 340555 had a related patch set uploaded (by Bartosz Dziewoński):
[VisualEditor/VisualEditor] ve.ce.ContentBranchNode: Don't set 'white-space: normal' on <pre> tags

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

The first patch should fix the editability/inspector behavior, and the second should fix the missing newlines.

So, this is actually not so easy. If https://gerrit.wikimedia.org/r/340554 is merged, preformatted text that previously existed on a page can't be edited – changes seem to apply, but they are lost upon saving (this is also seen in the diff or when switching to wikitext).

The reason why is because the contents are now duplicated in data-mw.body.extsrc, and ve.dm.MWPreformattedNode doesn't handle that.

Assuming that we want to go back to the "inline" editing, and not an inspector for <pre>, this will require some more significant changes. I'm actually not sure how to fix them, but ve.dm.MWPreformattedNode will need to get a lot more complicated.

If we want a dirty workaround, it looks like ve.dm.MWPreformattedNode.static.preserveHtmlAttributes = false; makes it work – it causes the 'typeof', 'data-mw' etc. to be stripped, and the preformatted text is turned into wikitext from a normal HTML tag by Parsoid. But this will cause dirty diffs when editing (removal of <pre>…</pre> and addition of one-space indent).

(https://gerrit.wikimedia.org/r/340555 is safe to merge, and should make this problem look a lot less broken from the user's perspective.)

After talking with Ed: we should probably keep this mostly as it currently is, just with some UI polishing, as our previous handling was a bit broken anyway.

For preformatted text generated by wikitext one-space indent, we keep using ve.dm.MWPreformattedNode with no changes. Note that this can have annotations or other content in it.

For preformatted text generated by wikitext <pre>, there should be a new node and a new inspector, only handling plain text. There should be an option to convert it to the other kind.

Change 340555 merged by Bartosz Dziewoński:
[VisualEditor/VisualEditor] ve.ce.ContentBranchNode: Don't set 'white-space: normal' on <pre> tags

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

Change 340690 had a related patch set uploaded (by Jforrester):
[mediawiki/extensions/VisualEditor] Update VE core submodule to master (3056a4a46)

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

Change 340690 merged by Jforrester:
[mediawiki/extensions/VisualEditor] Update VE core submodule to master (3056a4a46)

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

Change 340554 had a related patch set uploaded (by Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor] Update <pre> support for Parsoid changes

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

Checked in betalabs both samples for preformatted text from https://en.wikipedia.org/wiki/GIF#Example_GIF_file
https://en.wikipedia.org/wiki/Domain_Name_System#Wildcard_DNS_records.

Betalabs displays VE for preformatted text with preserved new lines.


vs .wmf14

Jdforrester-WMF renamed this task from Preformatted text no longer directly editable, displays without newlines and opens an inspector for editing to <pre> text displays contents in the surface without the newlines, which is confusing..Mar 8 2017, 1:00 AM
Jdforrester-WMF closed this task as Resolved.
Jdforrester-WMF triaged this task as Normal priority.
Jdforrester-WMF removed a project: Patch-For-Review.
Jdforrester-WMF set the point value for this task to 1.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptMar 8 2017, 1:00 AM
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Splitting the other part into a new task.