Page MenuHomePhabricator
Create Task
Maniphest T303298

mw:Placeholder elements are editable in VE
Closed, ResolvedPublicBUG REPORT
Actions

Description

When creating a node that has a typeof="mw:Placeholder" element, the expectation from Parsoid is that these nodes are not modified by clients (as per spec https://www.mediawiki.org/wiki/Specs/HTML/2.4.0#Expectations_of_editing_clients). However, this doesn't seem to be the case.

The problem can be displayed on https://patchdemo.wmflabs.org/wikis/6871284e64/wiki/Test?veaction=edit - where the content between the <translate> nodes is wrapped inside a <div> with that attribute, and where that part of the content stays editable.

A (convoluted) way to test this behaviour on a local installation:

  • run MW with Parsoid patch https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/769071/2 (test failures are irrelevant to the issue - tests haven't been updated yet for the new behaviour introduced by the patch)
  • declare some annotation (for instance by activating the Translate extension)
  • create wikitext content with said annotation, in a way that the annotation doesn't wrap a well-nested range, for instance
* hello <translate> world

some paragraph</translate> and some more

Details

SubjectRepoBranchLines +/-
Update VE core submodule to origin/REL1_38mediawiki/extensions/VisualEditormaster+1 -1
tests: mw.cx.TranslationTracker: Fix for changes in VEmediawiki/extensions/ContentTranslationREL1_38+5 -2
Update VE core submodule to REL1_38mediawiki/extensions/VisualEditorREL1_38+3 -1
Allow unknown types for ve.dm.AlienMetaItemVisualEditor/VisualEditorREL1_38+2 -0
ve.dm.ModelRegistry: Disallow unknown types on tag and tag+func matchesVisualEditor/VisualEditorREL1_38+49 -32
Update VE core submodule to master (cda4ec127)mediawiki/extensions/VisualEditormaster+1 -1
Update VE core submodule to master (38b8213b5)mediawiki/extensions/VisualEditormaster+3 -1
Allow unknown types for ve.dm.AlienMetaItemVisualEditor/VisualEditormaster+2 -0
tests: mw.cx.TranslationTracker: Fix for changes in VEmediawiki/extensions/ContentTranslationmaster+5 -2
ve.dm.ModelRegistry: Disallow unknown types on tag and tag+func matchesVisualEditor/VisualEditormaster+49 -32
Customize query in gerrit

Related Objects
Search...

StatusSubtypeAssignedTask
 OpenReleaseNoneT84936 Release VisualEditor-MediaWiki as "1.0"
 OpenNoneT50429 [Epic] Support editing parts of a page in VisualEditor-MediaWiki
 OpenNoneT54365 Explore performance gains from progressive (JIT?) de-alienation in VisualEditor
 OpenNoneT174303 Copy-pasting linked ISBN numbers from view mode HTML into VisualEditor inserts wikitext links to Special:BookSources (it should turn them into magic links?)
 OpenFeatureNoneT54091 The read HTML should have hinting to allow full DOM copying (as opposed to just rich copying) from read mode into VE surfaces
 OpenNoneT55784 [EPIC] Use Parsoid HTML for all page views
 OpenNoneT258838 [EPIC] Ensure all extensions on the Wikimedia cluster that use Parser hooks or implement tag hooks are retargeted for Parsoid
 OpenNoneT351374 Improve Translatable Pages based on research input
 OpenNoneT131516 Reduce or eliminate the need for the user to touch <translate> tags and unit markers
 OpenNoneT143327 Make the Translate extension be based on DOM, not strings
 ResolvedEsandersT73730 VisualEditor: Content inside templates for translation of templates does not render in friendly ways
 OpenFeatureNoneT55974 Create a VisualEditor plugin tool to add/edit translations and translation variables (Translate extension)
 ResolvedihurbainT261172 Much content is missing when editing a translatable page
 ResolvedihurbainT261181 Make Translate extension compatible with Parsoid
 ResolvedFeatureihurbainT305066 Make extended annotation ranges uneditable
 ResolvedBUG REPORTmatmarexT303298 mw:Placeholder elements are editable in VE

Event Timeline

ihurbain created this task.Mar 8 2022, 5:13 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 8 2022, 5:13 PM
matmarex claimed this task.Mar 8 2022, 5:14 PM
gerritbot added a comment.Mar 8 2022, 8:54 PM

Change 769112 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[VisualEditor/VisualEditor@master] ve.dm.ModelRegistry: Disallow unknown types on tag and tag+func matches

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

gerritbot added a project: Patch-For-Review.Mar 8 2022, 8:54 PM

Change 769113 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@master] Pull through changes for Id4296736

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

matmarex added a comment.Mar 8 2022, 8:58 PM

This seemed like an obvious bug to me at first, but fixing it caused some test failures, so we might want to be a little more careful and think about whether this behavior was intentional.

The current behavior is that VE only makes nodes with typeof="mw:Placeholder" (or any other unrecognized rel, typeof or property) uneditable if they also have a rel, typeof or property that is recognized.

matmarex added a project: Editing-team (Kanban Board).Mar 8 2022, 8:58 PM
matmarex moved this task from To Triage to Triaged on the VisualEditor board.
matmarex moved this task from Incoming to Code Review on the Editing-team (Kanban Board) board.
cscott subscribed.Mar 15 2022, 3:50 PM

The MediaWiki DOM spec says:

A typeof="mw:Placeholder" protects DOM structures from any editing. Clients are expected to preserve / protect subtrees marked as such. Clients are also expected to preserve any DOM subtrees marked up with typeof, rel, property in the http://mediawiki.org/rdf/ namespace they don't understand. This decouples clients from Parsoid development, and lets them concentrate on editing constructs whose special semantics they understand without having to implement all possible content elements.

It's worth noting the <span typeof="bogus"> is valid wikitext. VE doesn't check the "in the mw: namespace." part, but Parsoid handles this by escaping all user-generated types (eg <span data-x-typeof="foo">) . So if VE sees a typeof, then it can depend on the fact that this was generated by Parsoid. We should probably amend the DOM spec to discuss this detail.

The DOM spec also discusses renaming mw:Placeholder to mw:Uneditable. Once @matmarex's patch lands, Parsoid should be able to do that freely, since both of these names will be equally "unknown" to VE. There won't be any 'special' behavior to the mw:Placeholder name in particular.

In practice, Parsoid used mw:Placeholder for two different purposes: initially it was to mark wikitext features that "VE didn't support yet". The use was then expanded to also mark HTML subtree that contains "uneditable wikitext". We expect to continue the latter use. But if a new wikitext feature was added to Parsoid's output, we'd expect that we'd only need to add a unique (not known to VE) typeof to the subtree; we wouldn't "also" have to tag it explicitly as placeholder/uneditable as we'd done in the past.

matmarex awarded a token.Mar 15 2022, 8:54 PM
PatchDemoBot added a comment.Mar 28 2022, 5:01 PM

Test wiki created on Patch demo by IHurbainPalatin (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/48e84ba4ae/w/

PatchDemoBot added a comment.Mar 28 2022, 5:02 PM

Test wiki on Patch demo by IHurbainPalatin (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/48e84ba4ae/w/

gerritbot added a comment.Mar 28 2022, 10:59 PM

Change 769112 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] ve.dm.ModelRegistry: Disallow unknown types on tag and tag+func matches

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

ihurbain added a comment.Mar 29 2022, 12:21 PM

Hi @matmarex - quick question: is the above-merged patch a partial/complete solution to this issue? I created a patchdemo with that patch and mine - https://patchdemo.wmflabs.org/wikis/e26b32870e/wiki/Test - and I was expecting the first two elements of the list to be uneditable (because they're enclosed in a mw:Placeholder <div>) - am I missing something, or is this to be expected? Thanks!!

matmarex added a comment.Mar 29 2022, 7:21 PM

@ihurbain It is a complete solution – I think this is a problem with patchdemo, as I'm not seeing the new version of the code in https://patchdemo.wmflabs.org/wikis/e26b32870e/w/load.php?debug=2&lang=en&modules=ext.visualEditor.core. I don't know what happened there. Sorry D:

matmarex added a comment.Mar 29 2022, 7:26 PM

When trying to merge the submodule update (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/769113), we were stopped by some test failures – it looks like we might need to update code in other extensions for this change. Basically, any ve.dm class that specifies matchTagNames or matchFunction, but not matchRdfaTypes, might require allowedRdfaTypes = null to be added to preserve existing behavior.

To be reviewed: https://codesearch.wmcloud.org/search/?q=static%5C.(matchTagNames%7CmatchFunction%7CmatchRdfaTypes)&i=nope&files=&excludeFiles=&repos=

matmarex added a comment.Mar 29 2022, 8:09 PM

I've reviewed the list and I haven't found anything that looks like it requires updates in extensions (only possibly ve.dm.AlienMetaItem in VE core). Suspicious classes either inherit matchRdfaTypes, or they are not supposed to match nodes with a type. In CX in particular, there is ve.dm.CXSentenceSegmentAnnotation with no matchRdfaTypes, but I think it's not supposed to match nodes with a type.

Unfortunately this means I don't know why the tests in CX are failing.

matmarex added a comment.Mar 29 2022, 8:29 PM

OK I found it!

The test for mw.cx.TranslationTracker.static.getSectionNodeValidationTokens includes some sample HTML that should have resulted in ve.dm.CXSectionNode matches, but that JS code was not being loaded in unit tests, so it was instead matching ve.dm.SectionNode… until now, when the lack of allowedRdfaTypes on that class resulted in it no longer matching.

gerritbot added a comment.Mar 29 2022, 8:37 PM

Change 774974 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/ContentTranslation@master] tests: mw.cx.TranslationTracker: Fix for changes in VE

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

gerritbot added a comment.Mar 29 2022, 8:38 PM

Change 774973 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[VisualEditor/VisualEditor@master] Allow unknown types for ve.dm.AlienMetaItem

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

gerritbot added a comment.Mar 30 2022, 5:01 AM

Change 774974 merged by jenkins-bot:

[mediawiki/extensions/ContentTranslation@master] tests: mw.cx.TranslationTracker: Fix for changes in VE

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

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.6; 2022-04-04).Mar 30 2022, 6:00 AM
matmarex added a project: MW-1.38-release.Mar 30 2022, 9:39 AM
ihurbain added a comment.Mar 30 2022, 12:08 PM

@matmarex thanks a lot!!

I actually finally faced the music and tested the patch on my local wiki and it worked as expected. (Now I'll need to polish the UX a bit, but I think that's on Translate side.) I was also going to ask for 1.38 backport, but I saw you took care of putting the tag already :) Thanks!!

ihurbain added a parent task: T305066: Make extended annotation ranges uneditable.Mar 30 2022, 1:57 PM
gerritbot added a comment.Mar 30 2022, 5:45 PM

Change 775361 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (cda4ec127)

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

gerritbot added a comment.Mar 30 2022, 9:20 PM

Change 774973 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Allow unknown types for ve.dm.AlienMetaItem

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

gerritbot added a comment.Mar 31 2022, 11:29 AM

Change 769113 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (38b8213b5)

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

gerritbot added a comment.Mar 31 2022, 12:02 PM

Change 775361 abandoned by Bartosz Dziewoński:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (cda4ec127)

Reason:

Duplicate

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

gerritbot added a comment.Mar 31 2022, 1:42 PM

Change 775440 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[VisualEditor/VisualEditor@REL1_38] ve.dm.ModelRegistry: Disallow unknown types on tag and tag+func matches

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

gerritbot added a comment.Mar 31 2022, 1:42 PM

Change 775441 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[VisualEditor/VisualEditor@REL1_38] Allow unknown types for ve.dm.AlienMetaItem

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

gerritbot added a comment.Mar 31 2022, 1:46 PM

Change 775440 merged by jenkins-bot:

[VisualEditor/VisualEditor@REL1_38] ve.dm.ModelRegistry: Disallow unknown types on tag and tag+func matches

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

gerritbot added a comment.Mar 31 2022, 1:46 PM

Change 775441 merged by jenkins-bot:

[VisualEditor/VisualEditor@REL1_38] Allow unknown types for ve.dm.AlienMetaItem

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

gerritbot added a comment.Mar 31 2022, 1:49 PM

Change 775864 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@REL1_38] Update VE core submodule to REL1_38

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

gerritbot added a comment.Mar 31 2022, 1:58 PM

Change 775442 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/ContentTranslation@REL1_38] tests: mw.cx.TranslationTracker: Fix for changes in VE

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

gerritbot added a comment.Mar 31 2022, 2:02 PM

Change 775864 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@REL1_38] Update VE core submodule to REL1_38

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

gerritbot added a comment.Mar 31 2022, 2:09 PM

Change 775442 merged by jenkins-bot:

[mediawiki/extensions/ContentTranslation@REL1_38] tests: mw.cx.TranslationTracker: Fix for changes in VE

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

matmarex moved this task from Code Review to Ready for Sign Off on the Editing-team (Kanban Board) board.Mar 31 2022, 6:35 PM
matmarex edited projects, added Skipped QA; removed Patch-For-Review.

I think that's everything merged and backported.

• ppelberg closed this task as Resolved.Apr 2 2022, 12:57 AM
matmarex mentioned this in T307305: Tables with template-generated attributes are not editable in VE.May 9 2022, 9:38 PM
gerritbot added a comment.Jan 30 2023, 5:02 PM

Change 884538 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to origin/REL1_38

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

gerritbot added a project: Patch-For-Review.Jan 30 2023, 5:02 PM
gerritbot added a comment.Jan 30 2023, 5:21 PM

Change 884538 abandoned by Reedy:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to origin/REL1_38

Reason:

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

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL