Page MenuHomePhabricator
Create Task
Maniphest T187913

Small change to section header not displayed in historical visual diff
Closed, ResolvedPublic1 Estimated Story Points
Actions

Description

See https://www.mediawiki.org/w/index.php?title=Parsoid/Deployments&diff=next&oldid=2719566&diffmode=visual
The "15" is scratched out but, the 34 is not displayed. Look at the corresponding wikitext diff which shows that 15 has been replaced by 34.

Details

SubjectRepoBranchLines +/-
Revert "For empty / whitespace-only headings, output <p> instead of <h#>"mediawiki/extensions/VisualEditormaster+0 -169
Customize query in gerrit

Related Objects

Event Timeline

ssastry created this task.Feb 21 2018, 5:18 PM
Restricted Application added a project: VisualEditor. · View Herald TranscriptFeb 21 2018, 5:18 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Schnark subscribed.Feb 24 2018, 10:35 AM

Note that the <ins>34</ins> is actually present, but for some reason hidden via display: none.

• Deskana renamed this task from Visual diff bug in the history tab to Small change to section header not displayed in historical visual diff.Feb 26 2018, 11:37 AM
• Deskana triaged this task as Medium priority.
• Deskana moved this task from To Triage to Current work on the VisualEditor board.
• Deskana edited projects, added VisualEditor (Current work); removed VisualEditor.
Esanders updated the task description. (Show Details)Mar 5 2018, 8:52 PM
Esanders added subscribers: Catrope, Esanders.Mar 5 2018, 11:03 PM

This looks like a very complex issue related to the fact that MWHeadingNode has the handlesOwnChildren property set (because of a hack that converts empty headings to paragraphs - which should possibly be moved to Parsoid...), and some weird behaviour that triggers it the deep re-application of html attributes in ve.dm.Converter.static.renderHtmlAttributeList. Basically it's all @Catrope's fault :)

Esanders added a comment.Mar 5 2018, 11:12 PM
In T187913#4025345, @Esanders wrote:

(because of a hack that converts empty headings to paragraphs - which should possibly be moved to Parsoid...)

So turns out that in scrub_wikitext mode, Parsoid does now remove empty headings, so we should be able to kill that hack. That will fix this specific issue, but I still suspect there is a problem with renderHtmlAttributeList.

Esanders added a comment.Mar 5 2018, 11:13 PM

Verified working with a monkey patch:

image.png (191×1 px, 24 KB)

gerritbot added a project: Patch-For-Review.Mar 5 2018, 11:28 PM
Esanders added a comment.Mar 5 2018, 11:29 PM

https://gerrit.wikimedia.org/r/#/c/416601/

ReleaseTaggerBot added a project: MW-1.31-release-notes (WMF-deploy-2018-03-06 (1.31.0-wmf.24)).Mar 6 2018, 3:01 PM
Esanders added a comment.Mar 6 2018, 4:49 PM

Filed T189030 for the converter issue.

Esanders moved this task from Incoming to Code review on the VisualEditor (Current work) board.Mar 6 2018, 4:49 PM
Jdforrester-WMF assigned this task to Esanders.Mar 8 2018, 2:45 PM
Jdforrester-WMF moved this task from Code review to Product owner review on the VisualEditor (Current work) board.
matmarex closed this task as Resolved.Mar 15 2018, 2:07 PM
matmarex removed a project: Patch-For-Review.
matmarex subscribed.

The patch was merged some days ago (Gerritbot really doesn't like this task…). The related task mentioned above is also fixed.

All of this code is actually already in production and works correctly:

image.png (693×1 px, 146 KB)

• Deskana set the point value for this task to 1.Mar 15 2018, 2:07 PM
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