Added a file that tests *all* elements for too few children. #19737
Conversation
There are 32 tests in the file. Currently, the test assumes that the structure is preserved. This is still up in the air (#15). If that changes, then the elements with id="ref_xxx" need to change, but everything else should remain unchanged. I have removed the old tests which were more limited (fixes #159)
There was a problem hiding this comment.
overall this looks good to me, but I think some changes should be made first.
Commit message should contain the full URL to https://github.com/mathml-refresh/mathml/issues other github is confused regarding which issues you are referring to.
| <script src="/mathml/support/feature-detection.js"></script> | ||
| <script src="/mathml/support/layout-comparison.js"></script> | ||
| <script> | ||
| function getBox(id, i) { |
There was a problem hiding this comment.
This should be called "getElement" I guess.
| return result; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: here and elsewhere, two blank lines are used when only one is really needed.
| function removeIgnoredMrows(children) { | ||
| let result = []; | ||
| Array.from(children).forEach(child => { | ||
| if (child.getAttribute("class")==="ignore_mrow") { |
There was a problem hiding this comment.
This is better written child.classList.contains("ignore_mrow") so that it works even with multiple classes.
| ]; | ||
|
|
||
| // add a has_xxx key/value pair for feature dectection | ||
| elements.forEach((e) => { e.has = "has_"+e.name;}); |
There was a problem hiding this comment.
This can be written
`has_{e.name}`
( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals )
Anyway, I don't think this is needed.
| // ) | ||
| }, | ||
| "features/preconditions for running empty MathML tests" | ||
| ); |
There was a problem hiding this comment.
This should be removed. The test() are really independent from each other, so this will make this one test fail for browsers lacking mathml layout but does not affect at all the result in the other tests.
| {name:"mtd", n:1}, | ||
| {name:"maction", n:1}, | ||
| {name:"semantics", n:1}, | ||
| {name:"math", n:1} |
There was a problem hiding this comment.
I don't think the tests for n=1 are correct. It seems to me that all these elements are allowed to have no children. We should only keep the elements that have explicit expected number of children in MathML Core.
I would also move mmultiscripts and tabular elements in separate tests.
| function() { | ||
| compareLayout(getBox(element.name, i), getBox("ref_" + element.name, i), 0); | ||
| }, | ||
| "too few children for " + element.name + " (" + i + " " + (i==1 ? "child" : "children") + ")." |
There was a problem hiding this comment.
Again, please use string literal here.
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Test if elements layout properly if they have too few children</title> | ||
| <link rel="help" href="https://mathml-refresh.github.io/mathml-core/#script-and-limit-schemata"> |
There was a problem hiding this comment.
We should have links to the section corresponding to each element tested here. And these sections should clearly say how to handle "too few children". AFAIK, the core spec allows mrow-like elements with 0 children and the tabular section is not really well-explained (it defers to HTML).
| if (i < element.n) { | ||
| test( | ||
| function() { | ||
| compareLayout(getBox(element.name, i), getBox("ref_" + element.name, i), 0); |
There was a problem hiding this comment.
Adding a
assert_true(MathMLFeatureDetection.has_mspace());
assert_true(MathMLFeatureDetection.has_mrow());
at the beginning of the function should be enough for now. Note that MathMLFeatureDetection caches the result, so the subsequents calls are fast.
If you want you can also add
assert_true(MathMLFeatureDetection[`has_${element.name}`]());
so that the detection is stronger when the functions are implemented in MathMLFeatureDetection
|
|
||
| function runTests() { | ||
|
|
||
| // elements being tested |
There was a problem hiding this comment.
I think the description of the array can be improved here. Probably renaming "n" to "minChildCount" would help to understand what these are about.
…ably will be needed once code is fixed to match expected behavior according to the spec. Also fixed a bug in the script that isn't currently being tickled but might when more tests get added.
There are 32 tests in the file.
Currently, the test assumes that the structure is preserved. This is still
up in the air (#15). If that changes, then the elements with id="ref_xxx"
need to change, but everything else should remain unchanged.
I have removed the old tests which were more limited (fixes #159)