Page MenuHomePhabricator
Create Task
Maniphest T146187

Selser is broken in the needsOldDom case when opts.from === "pagebundle"
Open, MediumPublic
Actions

Description

(See the discussion in https://gerrit.wikimedia.org/r/#/c/311626/)

https://github.com/wikimedia/parsoid/blob/master/lib/utils/DOMUtils.js#L2678-L2693

mw ids aren't added to the dom which results in a lot of dirty diffs (ids aren't ignored when domdiff'ing).

Ignoring ids with a specialized attribute handler doesn't seem like a good solution.

We can add the ids by setting env.pageBundle = true;, but this is always going to be a best-faith effort when regenerating the dom (subbu's words). This is equally true in the opts.from === "html" case.

What to do? what to do? Should we drop this "feature" or fix the pagebundle path to work like html.

Related Objects

Event Timeline

Arlolra created this task.Sep 20 2016, 6:49 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 20 2016, 6:49 PM
Arlolra triaged this task as Medium priority.Sep 20 2016, 6:50 PM
ssastry moved this task from Needs Triage to html2wt on the Parsoid board.Dec 18 2017, 9:43 PM
ssastry edited projects, added Parsoid-Edit-Support; removed Parsoid.Sep 20 2018, 4:28 PM
ssastry moved this task from Backlog to Selser on the Parsoid-Edit-Support board.Sep 20 2018, 4:48 PM
Aklapper edited projects, added Parsoid; removed Parsoid-Edit-Support.Feb 29 2020, 5:15 PM
ssastry moved this task from html2wt to Needs Investigation on the Parsoid board.Mar 11 2020, 3:57 AM
ssastry assigned this task to Arlolra.Mar 23 2020, 3:40 PM
Arlolra added a comment.Mar 24 2020, 4:10 PM

Ignoring ids with a specialized attribute handler doesn't seem like a good solution.

Not sure why I wrote that but, on the contrary, it does seem like it would be ok.

We can add the ids by setting env.pageBundle = true;, but this is always going to be a best-faith effort when regenerating the dom (subbu's words). This is equally true in the opts.from === "html" case.

This was probably all before we did the analysis in,
https://github.com/wikimedia/parsoid/blob/master/src/Core/WikitextContentModelHandler.php#L24-L58

Arlolra added a comment.Mar 24 2020, 4:15 PM

In practice, RESTBase sends us the old DOM in the "pagebundle" case and we generate the DOM more often in the "html" case.

Also, needsOldDom is now just this condition $selserData->oldHTML === null,
https://github.com/wikimedia/parsoid/blob/master/src/Core/WikitextContentModelHandler.php#L59

Arlolra moved this task from Needs Investigation to Bugs & Crashers on the Parsoid board.Mar 24 2020, 4:16 PM
Arlolra removed Arlolra as the assignee of this task.Aug 22 2022, 5:07 PM
Arlolra claimed this task.
Arlolra mentioned this in T317215: Selser broken on html endpoints.Sep 7 2022, 4:19 PM
daniel mentioned this in T318398: Parsoid: use current revision rendering to enable selser, when no stash key/etag is provided..Sep 23 2022, 9:18 AM
Arlolra mentioned this in T333402: Switching from source editing to visual editing mode is broken with the REST API .Apr 6 2023, 10:16 PM
Arlolra reassigned this task from Arlolra to ABreault-WMF.Jun 6 2024, 3:02 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 · Credits