Bug 160509

Summary: [meta] Removing/rewriting the mfenced implementation
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: alex, bfulgham, cfleizach, darin, davidc, distler, jdiggs, jfernandez, rego
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: https://www.w3.org/TR/MathML3/chapter3.html#presm.mfenced
See Also: https://bugs.webkit.org/show_bug.cgi?id=181443
https://github.com/mathml-refresh/mathml/issues/2
Bug Depends on: 57696, 125938, 139436, 160396, 160507, 166011    
Bug Blocks: 195797    

Description Frédéric Wang (:fredw) 2016-08-03 08:00:21 PDT
One of the most serious design issue in the MathML specification is the existence of the mfenced element: 

* It is strictly equivalent to an expanded form using the mrow and mo elements. There are several complicate equivalence rules to handle and WebKit (or any other implementations) does not support them completely. As a consequence this adds duplicate logic and code to our MathML implementation to support something that does not bring anything new for math rendering or accessibility.

* The text in mfenced is contained in the attributes rather than in text nodes. This makes mfenced quite different from the rest of the platform and sharing code even more difficult. This is also the cause of many other bugs or the need for special handling (see below).

Issues with this element have been reported to the W3C for a long time but it seems to have been a controversial topic in the Math WG mailing list from its beginning. The last thread is https://lists.w3.org/Archives/Public/www-math/2016Jul/0004.html but it seems unlikely that the Math WG will address this anytime soon.

The mfenced element is not used in the vast majority of pages or authoring tools (Wikipedia, LibreOffice, latexml, itex2MML, etc). I personally believe the best would just be to remove support for that element in order to clean up our code (Mozilla could do the same in Gecko and other web engines do not support it anyway). If it turned out to be used in old pages then backward compatibility can be ensured by this simple polyfill: https://people.igalia.com/fwang/mfenced-polyfill/mfenced.js

If we decide to keep it then it will definitely have to be rewritten completely. Here is a non-exhaustive list of issues in WebKit's implementation:

I) We do not handle the edge case of empty <mfenced> correctly. This is test 1 of https://people.igalia.com/fwang/mfenced-polyfill/

II) The MathML specification is not clear about whether open and close fences can have multiple characters but we removed support for this edge case in r202271. This is test 10 of https://people.igalia.com/fwang/mfenced-polyfill/. open and close fences are still stored on the RenderMathMLFenced class as string. Probably this can just become UChar32 and does not need to be stored on the class.

III) Similarly, the parsing of the separators attribute can probably be made more efficient and we may not need to store m_separators on the RenderMathMLFenced class. See also bug 125938.

IV) We do not handle surrogate pairs in attributes correctly. This is test 15 of https://people.igalia.com/fwang/mfenced-polyfill/. See also bug 125938.

V) RenderMathMLFenced is not updated properly when attributes are modified. See bug 57696 and https://bug-57696-attachments.webkit.org/attachment.cgi?id=285222.

VI) RenderMathMLFenced is not updated properly when the child list is modified. See bug 160507 and https://bug-160507-attachments.webkit.org/attachment.cgi?id=285227.

VII) Because the text of operators is in attributes they are not searchable/selectable by default. See bug 139436.

VIII) We use of the old RenderPtr object to hold anonymous operators. See bug 160396.

IX) The anonymous operators need special handling to work the same as "normal" operators causing additional maintenance cost. This has been quite painful to handle during code refactoring. However, the code for anonymous operators is now better isolated (RenderMathMLFencedOperator.cpp and m_isAnonymousOperator=true in AccessibilityMathMLElement.cpp).

Some of the issues above could be fixed by applying the same refactoring as for other MathML classes:

1) Do not create anonymous nodes. This was done for other element in https://trac.webkit.org/wiki/MathML/Early_2016_Refactoring#Phase1

2) Move attribute parsing from RenderMathMLFenced class to DOM classes. This was done for other elements in https://trac.webkit.org/wiki/MathML/Early_2016_Refactoring#Phase3

One of the issue with 1) is that the accessibility code assumes the presence of these anonymous operators in the render tree and so removing them without modifying the accessible tree will be tricky.

A third option is of course to keep the status quo of mfenced will all its bugs and design issues and hope that it will now no longer interfere too much with the rest of the code.
Comment 1 Frédéric Wang (:fredw) 2017-11-14 23:53:34 PST
Another issue:

X) We don't apply mathvariant/displaystyle to mfenced operators (see tests 18 and 19 in https://people.igalia.com/fwang/mfenced-polyfill/). Inconsistencies with normal non-anonymous operators is the cause of bug 166011.
Comment 2 Frédéric Wang (:fredw) 2018-05-22 05:54:22 PDT
It seems that some RenderTreeBuilderMathML class was added in bug 181443 for the sake of RenderMathMLFenced. Dropping mfenced support would probably avoid this code.