Page MenuHomePhabricator
Create Task
Maniphest T44803

Apply tag minimization on incoming HTML (ex: merge identical adjacent i/b tags)
Closed, ResolvedPublic
Actions

Description

The normalize_document algo that rewrites dom to deal with nested inline tags does not work in the presence of mw:StartTag and mw:EndTag meta-tags. The algo needs fixup to handle that. Plus, the reordered nodes also need their tsr and other attrs. patched up.

longest_linear_path function is one that checks for children.length > 1 -- and this check is not valid anymore. Look for other such checks and update.

Version: unspecified
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=49873
https://bugzilla.wikimedia.org/show_bug.cgi?id=53208

Details

Reference
bz42803

Related Objects
Search...

Event Timeline

• bzimport raised the priority of this task from to High.Nov 22 2014, 12:54 AM
• bzimport added a project: Parsoid-DOM.
• bzimport set Reference to bz42803.
ssastry created this task.Dec 6 2012, 11:28 PM
cscott added a comment.Apr 9 2013, 9:49 PM

CSA notes: see http://www.mediawiki.org/wiki/Parsoid/PostProcessor:DOM_Tag_Minimization

Also -- we might need to handle <a> tags in this algorithm, to ensure that <a> doesn't get broken up by reordering the other tags. In particular, look at the wikitext snippet:

''Something [http://example.com mixed''''', even bold]'''

which should not break up the <a> tag.

gerritbot added a comment.Apr 24 2013, 1:11 AM

Related URL: https://gerrit.wikimedia.org/r/60617 (Gerrit Change I25711292536b66d33c8228ca01c526ccfa93875b)

cscott added a comment.Apr 24 2013, 1:12 AM

The tag minimization is running out of stack space. The algorithm needs to be rewritten with an external stack.

See https://gerrit.wikimedia.org/r/60617

• GWicke added a comment.May 7 2013, 11:45 PM

Disabling this (predictably) breaks our serializer handling of adjacent i/b tags, which was reported in bug 48194. We should write an iterative version of longest_linear_path.

ssastry added a comment.May 8 2013, 2:47 AM

Why does disabling the DOM pass (on wt2html) break our serializer handling of i/b tags on (html2wt)?

• GWicke added a comment.May 8 2013, 3:37 AM

Something like <i>foo</i><i>bar</i> *could* be serialized as ''foo''<nowiki/>''bar'', but that is not what we are currently doing (and nor is it very desirable IMO).

Minimization avoids this issue by merging adjacent italic/bold ranges. With it enabled, the result will be ''foobar''.

ssastry added a comment.May 8 2013, 4:05 AM

But, <i>foo</i><<i>bar</i> could be fresh content and not be from original source. Are you suggesting we run this on the dom we get from VE before serialization?

• GWicke added a comment.May 8 2013, 4:08 AM

Yes, that was the original idea. Minimization on the way out is still nice for round-tripping (and HTML sanity), but on the way in it is actually important for serialization.

• GWicke added a comment.May 8 2013, 4:11 AM

Some old notes: http://www.mediawiki.org/w/index.php?title=Parsoid/Todo&oldid=605270#Inline_element_nesting_minimization

ssastry added a comment.May 8 2013, 4:13 AM

For on the way in, meta tags are not an issue since. So, it can already be applied if we fix the stack space issue.

var dpp = require('./mediawiki.DOMPostProcessor.js'), d = require('./domino.js')

undefined

var doc = d.createDocument('<i>foo</i><i>bar</i>')

undefined

dpp.normalizeDocument(doc)

undefined

doc.outerHTML

'<html><head></head><body><i>foobar</i></body></html>'

ssastry added a comment.May 8 2013, 4:20 AM

Anyway, but yes, that code is quite stale and requires upgrade and testing and can be enabled on the DOM on the way in. Not sure if scott is still interested in this ticket. Otherwise, I can tackle this one of these days.

ssastry added a comment.May 8 2013, 4:21 AM

Commit 1c80e2d7f06403a49c80abfbd69b8b1fad039786 has some test DOMs.

• GWicke added a comment.Jun 12 2013, 2:49 PM
  • Bug 49478 has been marked as a duplicate of this bug. ***
• GWicke added a comment.Jun 18 2013, 9:02 PM
  • Bug 49766 has been marked as a duplicate of this bug. ***
Esanders added a comment.Jun 21 2013, 11:26 AM
  • Bug 49873 has been marked as a duplicate of this bug. ***
gerritbot added a comment.Jun 21 2013, 11:28 AM

Related URL: https://gerrit.wikimedia.org/r/69852 (Gerrit Change Ibb18b4f653c664e8ab7876498dc8395d878f7aaa)

cscott added a comment.Jul 12 2013, 5:09 PM

Just recapping an earlier IRC discussion: my opinion is that the general form of this bug is too aggressive (globally rewrites the input too drastically) and unnecessary. I prefer to see local fixes such as the one in comment 16 that address the straightforward local cases (like merging adjacent <span> or other tags with identical attributes) rather than trying to find globally-optimal tag nestings (which may create surprising long-distance mutations which can also be costly to search for).

My opinion may well be wrong. Hopefully the bugs linked as dups can be evaluated in this light; one of them may well be the killer case which simply can't be solved with a local transform and instead needs the resurrection of the global tag minimization code. I look forward to being thus shown incorrect. ;)

• GWicke added a comment.Jul 12 2013, 7:28 PM

(In reply to comment #17)

Just recapping an earlier IRC discussion: my opinion is that the general form
of this bug is too aggressive (globally rewrites the input too drastically)
and
unnecessary.

The incoming normalization will only apply to ranges where at least a part is new or modified content. So no global rewrites.

A proper solution should not be thrown off by other unrelated formatting elements, so only looking for siblings won't quite cut it. This is easier in VE as they are using flat annotations instead of elements (see comment 16). Our equivalent needs to flatten nested elements into ranges, which is basically what this algo already implements.

Esanders added a comment.Jul 13 2013, 10:44 AM

With this change: https://gerrit.wikimedia.org/r/#/c/73523/ it is now possible that VE will send back adjacent annotations, e.g. by loading a document with two bolds separated by text, and then deleting the text between them, we will send back <b>Foo</b><b>Bar</b>, which currently serialises to '''Foo''''''Bar''' (which goes back to <b>Foo'<i>'</i>Foo</b>) so there is potential for corruption.

ssastry added a comment.Jul 15 2013, 3:56 AM

Given all the discussion we've had, I think we should implement a quick and simple solution for now. Here is my proposal.

As far as I can tell, the only place where tag reordering and minimization matters is where wikitext I and B tags end up as siblings. So, just targeting that situation should give what we want.

  1. ..</i><i> ..
  2. ..</b><b> ..
  3. ..</i></b><i> ..
  4. ..</b></i><b> ..

These 4 scenarios boil down to two cases which can be handled fairly easily in the serializer.

While HTML like this <s><i><b>a</b></i></s><b>c</b> (and other variants) could also be minimized (and would indeed be handled by the algorithm under discussion that I implemented), serializing this to <s>'''''a'''''</s>'''c''' should be sufficient since there is no ambiguity in the parse in the front end and hence no corruption.

• GWicke added a comment.Aug 16 2013, 12:23 AM
  • Bug 52876 has been marked as a duplicate of this bug. ***
• GWicke added a comment.Aug 16 2013, 12:32 AM

(In reply to comment #20)

Given all the discussion we've had, I think we should implement a quick and
simple solution for now. Here is my proposal.

As far as I can tell, the only place where tag reordering and minimization
matters is where wikitext I and B tags end up as siblings. So, just
targeting
that situation should give what we want.

  1. ..</i><i> ..
  2. ..</b><b> ..
  3. ..</i></b><i> ..
  4. ..</b></i><b> ..

These 4 scenarios boil down to two cases which can be handled fairly easily
in
the serializer.

We also need to consider variants where other tags are in the mix. Example:

<i><ins><b>bold-italic</b></ins></i><b>bold</b>

This happens to be a case that is handled by our minimization algorithm. The main deficiency is that the current recursive implementation can run out of stack space: https://gerrit.wikimedia.org/r/#/c/60617/

The other issue is that it currently applies to both old and new content. For incoming content it should only apply to b/i pairs in new/modified content.

I still think the simplest solution is to fix the algorithm to be iterative, and to enable it only on new / modified incoming content on the way in.

ssastry added a comment.Aug 16 2013, 2:50 PM

I think for that example: ''<ins>'''bold-italic'''</ins>'''''bold''' is an acceptable serialization which parses back to the same HTML.

But, the minimized HTML <b><i><ins>bold-italic</ins></i>bold</b> can be serialized to '''''<ins>bold-italic></ins>''bold'''. While shorter, this does not parse back to the original HTML, but to <i><b><ins>bold-italic</ins></b></i><b>bold</b>.

I am no longer convinced that we need a generic minimization solution rather than a special-case solution just for b-i tags. This will just let us strip out a big part of our codebase.

So, I am going to tackle this bug in the next couple days and first attempt the simplified solution and fall back to fixing the existing algorithm otherwise.

ssastry added a comment.Aug 16 2013, 2:55 PM

That said, a small part of me will be sad at not using it, because that algorithm was pretty much one of my first commits to the codebase ;-) and I think it is a neat general-purpose algorithm for rewriting interleaved tags to a minimal form.

• GWicke added a comment.Aug 16 2013, 4:05 PM

(In reply to comment #23)

I think for that example: ''<ins>'''bold-italic'''</ins>'''''bold''' is an
acceptable serialization which parses back to the same HTML.

But, the minimized HTML <b><i><ins>bold-italic</ins></i>bold</b> can be
serialized to '''''<ins>bold-italic></ins>''bold'''. While shorter, this
does
not parse back to the original HTML, but to
<i><b><ins>bold-italic</ins></b></i><b>bold</b>.

Which then serializes to

'''''<ins>bold-italic</ins>''bold'''

This is semantically identical, and round-tripping is no issue for new or modified content. Do you see any issue in simply doing this apart from the need to fix up the recursion issue?

ssastry added a comment.Aug 16 2013, 4:27 PM

That (I think that function can easily be converted to tail-recursive form, if it is not already, and then iteration) + feels like using a very heavyweight technique for a relatively simple problem + deleting code is almost always a good thing. It would be more of a no-brainer if there were other scenarios that required this generic solution.

gerritbot added a comment.Aug 22 2013, 4:45 AM

Change 80335 had a related patch set uploaded by Subramanya Sastry:
(Bug 42803) Apply I/B minimization to incoming HTML

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

ssastry added a comment.Aug 22 2013, 10:06 PM
  • Bug 48194 has been marked as a duplicate of this bug. ***
gerritbot added a comment.Aug 23 2013, 11:55 PM

Change 80335 merged by jenkins-bot:
(Bug 42803) Apply I/B minimization to incoming HTML

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

Jdforrester-WMF added a comment.Sep 25 2013, 1:29 AM
  • Bug 53208 has been marked as a duplicate of this bug. ***
Liuxinyu970226 mentioned this in T49326: <a> / <b> / <i> issues..May 15 2018, 1:59 AM
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