330964 - (row-column-spacing) Add support for mtable@rowspacing/columnspacing/framespacing attributes

Status: RESOLVED FIXED

(Core :: MathML, defect, P3)

Description

[David Harvey]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/417.9 (KHTML, like Gecko) Safari/417.8
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

The "rowspacing" attribute for MathML tables is not supported. According to the MathML spec, this attribute controls the amount of space between rows in the table. Correct implementation would allow constructs like AMS-LaTeX's "\substack" command.

For example, the following three chunks should render differently, but Gecko draws them the same way:

== example 1 ==

    <mtable rowspacing="2ex">
        <mtr><mtd><mi>x</mi></mtd></mtr>
        <mtr><mtd><mi>x</mi></mtd></mtr>
    </mtable>

== example 2 ==

    <mtable rowspacing="0">
        <mtr><mtd><mi>x</mi></mtd></mtr>
        <mtr><mtd><mi>x</mi></mtd></mtr>
    </mtable>

== example 3 ==

    <mtable>
        <mtr><mtd><mi>x</mi></mtd></mtr>
        <mtr><mtd><mi>x</mi></mtd></mtr>
    </mtable>


Reproducible: Always

Steps to Reproduce:
1. View a page with the above MathML

Actual Results:  
Example 1 should have widely spaced rows; example 2 should tightly spaced rows; example 3 should be somewhere in between.

Expected Results:  
All three tables have the same amount of inter-row space (I'm guessing 1ex).

I've only tried this on Mac OS, haven't tried other platforms.

Comment 1

[Steuard Jensen]

Attachment: Testcase based on reporter's code

Comment 2

[rbs]

The spec seems a bit vague as to the meaning of the rowspacing attribute (and its cousin -- columnspacing):

"The rowspacing and columnspacing  attributes specify how much space should be added between each row and column. However, spacing before the first row and after the last row (i.e. at the top and bottom of the table) is given by the second number in the value of the framespacing attribute, and spacing before the first column and after the last column (i.e. on the left and on the right of the table) is given by the first number in the value of the framespacing attribute."

1/ Case of a mathbackground set on a row, I have always understandood the MathML's rowspacing as HTML's cellpadding. Hence I would expect the extra space to go in the cell and thus get the specified mathbackground color. (Note that this is different from HTML's cellspacing -- which is outside the cell, and does not get the background-color of the cell).

2/ Case of a table with rowlines, consider this mtable:

<mtable rowspacing="1ex" rowline="solid">
   row1
   ----    <---- solid rowline
   row2
</mtable

Is the rowspacing split between the rowline, i.e., 0.5*rowspacing(before) + rowline + 0.5*rowspacing(after)? Or does the spacing means the "double", i.e., rowspacing(before) + rowline + rowspacing(after)? Or even more enigmatic, it is assumed that rowspacing should always be specified in pairs?...

I have always understood it as meaning 0.5*rowspacing(before) + rowline + 0.5*rowspacing(after). But I got confused even more looking at the testsuite.
The testcases there are not clear cut either (e.g., they have as much values in the rowspacing as the number of rows -- whereas the spec says that the rowspacing is irrelevant to before the first row and after the last row):

Testsuite links:
http://www.w3.org/Math/testsuite/testsuite/Presentation/TablesAndMatrices/mtable/mtableAspacing1.xml
http://www.w3.org/Math/testsuite/testsuite/Presentation/TablesAndMatrices/mtable/mtableAspacing2.xml
http://www.w3.org/Math/testsuite/testsuite/Presentation/TablesAndMatrices/mtable/mtableAspacing3.xml
http://www.w3.org/Math/testsuite/testsuite/Presentation/TablesAndMatrices/mtable/mtableAspacing4.xml

Comment 3

[distler]

Hmmm. It doesn't seem that MathML3 sheds much light on RBS's questions. Perhaps these issues should be raised on the www-math mailing list.

Whatever the answer, SOME implementation of @rowspacing is better than none.

Comment 4

[Frédéric Wang (:fredw)]

Apart from understanding how rowspacing should work, I think one of the problem  is the way some MathML attributes are currently implemented. For example if <mtable/> has a "columnalign" attribute, private "-moz-math-columnalign" attributes are created on <mtd/>'s and the style is then given by the mathml.css sheet:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/mathml.css#256

This works when we only have a small set of values, but not for attributes with a length value:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/mathml.css#303

BTW, I don't like much this method because it adds unwanted attributes in the DOM (see bug 527201) and I suspect it is causing the layout issue of bug 491384. I guess the appropriate solution would be fixing bug 69409...

Comment 5

[Bernd]

shouldn't rowspacing map to borderspacing.mY and columnspacing to borderspacing.mX like cellspacing http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTableElement.cpp#1065 ?

Comment 6

[Frédéric Wang (:fredw)]

mtable seems a bit different, because rowspacing can have several values and we also have framespacing. Here is how I understand the spec for a mtable of N rows, framespacing="X" and rowspacing="Y0, Y2, ..., Y_(n-1)":

^
| X
V
-----
/////
row0
/////
-----
^
| Y_(0 mod n)
V
-----
/////
row1
/////
-----
^
| Y_(1 mod n)
V
-----
/////
...
/////
-----
^
| Y_(N-2 mod n)
V
-----
/////
row_(N-1)
/////
-----
^
| X
V

However, the spec does not provide any attribute "rowpadding" that would allow to describe the spacing inside the cell. I guess that's why RBS wanted instead to split the rowspacing half inside and half outside the cell...

Comment 7

[Frédéric Wang (:fredw)]

Mass change: setting priority to 3 for bugs preventing Gecko's Native MathML to be enabled by default in MathJax.

Comment 8

[Frédéric Wang (:fredw)]

Here is a more info for people wanted to work on this:

== DESCRIPTION ===

The goal is to implement the rowspacing and columnspacing on the mtable element (perhaps framespacing too):

http://www.w3.org/TR/MathML/chapter3.html#presm.mtable.attrs

I asked precision on the Math WG mailing list some time ago: http://lists.w3.org/Archives/Public/www-math/2010Aug/0004.html

The rowspacing attribute is a list of lengths "l_0 l_1 l_2 ...". If the table has N rows, then the spacing applies to the N-1 gaps:

[ROW_0]
  ↕ l_0
[ROW_1]
  ↕ l_1
[ROW_2]
  ↕ l_2
...
[ROW_(N-1)]
  ↕ l_(N-1)
[ROW_N]

The note about "multiple values" say that if there are more gaps than supplied values, the last value is repeated as needed. If there are too many values supplied, the excess are ignored.

The columspacing attribute is similar but applies to gaps between columns.

In theory these attribute values can come from an mstyle ancestors (bug 768819) but we want to WONTFIX that one after bug 838509, so no need to worry about that.

As said in comment 5, this seems to correspond to HTML attributes cellspacing:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLTableElement.cpp#703
https://developer.mozilla.org/en-US/docs/Web/CSS/border-spacing

except that the spacing can be different for each gap between column/row so it's a bit more complicated. Also, note that the border-spacing is used for the spacing around the table, so it might be relevant to consider the mtable@framespacing attribute as well: this one takes two length values and specifies the additional spacing added between the table and frame, if frame is not "none". The first value specifies the spacing on the right and left; the second value specifies the spacing above and below.

=== PROPOSAL == 

The work is very similar to what Quentin did in bug 731667. I see essentially three steps (so three separate patches):

1) Tests (see attachment 8347144 [details] [diff] [review]). Some ideas:

   - rowspacing/columnspacing
   - single/multiple values
   - not enough values / too much values
   - invalid attributes
   - dynamic changes via Javascript
   - various values for spacing including 0 and default

2) Parsing of rowspacing/columnspacing

   See http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmtableFrame.cpp and in particular MapAllAttributesIntoCSS where we create new FrameProperties. On the one hand, this can be easier because you only have to consider attributes from the <mtable> (not <mtd> or <mtr>) but on the other hand you have to store a list of nsCSSValues returned by nsMathMLElement::ParseNumericValue (http://mxr.mozilla.org/mozilla-central/source/content/mathml/content/src/nsMathMLElement.cpp), rather than just enumerate values like center/left/right, so the code should be adapted.

3) Rendering of rowspacing/columnspacing

You'll have to generalize the layout/tables implementation from which nsMathMLmtableFrame is derived so that it is able to handle different different spacing for each gap between column/row. Perhaps one way would be to make nsTableFrame::GetCellSpacingX() and  nsTableFrame::GetCellSpacingY() take an (optional?) argument indicating the gap index (perhaps using -1 and N for the spacing around the table, or something like that). Then every places where it is used in layout/tables/ should be modified to specify the gap index explicitly. nsTableFrame::GetCellSpacingX() and nsTableFrame::GetCellSpacingY() will just ignore that gap index and use the frame-spacing value, so the HTML implementation won't be affected. However nsMathMLmtableFrame will override this GetCellSpacingX/GetCellSpacingY to take into account the gap index and to return the value from FrameProperties, when specified.

An alternative for 2) and 3): one could create new private CSS properties  -moz-row-spacing / -moz-column-spacing that would handle a list of length and map the MathML attributes to these properties on the content/ side. Then modify layout/tables/ to consider these values in place of frame-spacing, when they are specified.

Comment 9

[James Kitchener (:jkitch)]

Attachment: tablespacing.png

The attached picture shows a 2x2 <mtable>, with each <mtd> containing a single <mn>. row/column/framespacing are all set to 0 (or 0 0).  Document set to standards compliance mode.

The blue dashed border indicates the <mn> element's outline, the red border that of the <mtd> elements and the black border that of <mtable>.  I interpret the touching of the borders as indicating that the 0 spacing is rendered appropriately.

What I haven't worked out is what is causing the spacing within the <mtd> element within standards compliance mode.  (It disappears when rendered in quirks mode).  I've tried zeroing the margin and padding for everything in the page, but without success.    

Can you identify the cause of the whitespace?  I presume it is something that needs to be taken into account, otherwise 0.5ex will be added to whatever rowspacing the user specifies.

Comment 10

[Frédéric Wang (:fredw)]

Do you have a testcase / patch that you used to take that screenshot?

Comment 11

[James Kitchener (:jkitch)]

Attachment: Part1 Add arguments to GetCellSpacing[XY] WIP

Adds an row/column index argument to GetCellSpacing.  The two argument version is a nicety that calculates the spacing of multiple columns.  In the default tableFrame case it simply multiplies the answer out.  These functions are to be overloaded by nsMathMLmtableFrame.

I'm not yet certain of selecting the correct row/column in all cases, but if the code isn't exercised by MathML the choice is unimportant and won't change the outcome.  I suppose I can only use the argument method on instances invoked by MathML tables and have the argument-less one assert in such situations.

Comment 12

[James Kitchener (:jkitch)]

Attachment: Part 2: MathML mtable changes

As far as I can tell row/column/framespacing isn't exposed to anything except mtable, so performing the parsing within the class seems reasonable.

It mostly works, but there are a few test failures still to resolve and more tests are needed.

Comment 13

[James Kitchener (:jkitch)]

Attachment: Part 3: tests WIP

Not yet exhaustive tests are attached.

attachment 8377104 [details] corresponds to tablespacing-5.html although I doubt that particular test will be included as part of the final set.

Tests to ensure that rowspacing="5ex" really creates a 5ex space are yet to be written.

Comment 14

[James Kitchener (:jkitch)]

Attachment: Part 0: Fix to CalcUnpaginagedHeight

The purpose of CalcUnpaginagedHeight is to compute a height, which is presumably a vertical distance.  Everything within the function deals with a vertical difference except the declaration of computedHeight, which seeks to multiply a number of rows (rowspan - 1) with the distance between columns (GetCellSpacingX()).  This I believe to be a mistake.

GetCellSpacingY() which returns the distance between two rows seems a more appropriate fit.

Comment 15

[James Kitchener (:jkitch)]

Attachment: Part1: Add arguments to GetCellSpacing[XY]

The spacing between rows and clumns in MathML mtables is variable, so I would like to add an argument to GetCellSpacing[XY] to specify which row/column space to use.

In ordinary tables the argument doesn't matter, so there shouldn't be any effect other than the asserts ensuring sensible values are passed in.  These asserts are also helpful in checking whether the correct function is called in situations where the number of rows and columns differ.

The two argument version of GetCellSpacing[XY] is a convenience to allow cumulative spacing to be determined by a single multiplication when index doesn't matter, rather than repeatedly adding the same operand.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry but I have to ask: are these attributes really needed? Could authors just use CSS padding instead?

Comment 17

[James Kitchener (:jkitch)]

Attachment: Difference between spacing types

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Sorry but I have to ask: are these attributes really needed? Could authors
> just use CSS padding instead?

My understanding is that two accomplish different things.  In the attached diagram:

left example sets mtd {padding:5px;}
middle example sets mtable {border-spacing:5px} mtd{padding:0px;}
right example sets rowspacing="5px 20px" columnspacing="5px 20px" framespacing="5px 5px" (using the patches from this bug)

padding adds the space inside the table cells, which are then placed touching each other, resulting in a solid block of red.
border-spacing leaves 5px of white between the cells, resulting in a windowpane effect.
rowspacing/columnspacing is similar to the border-spacing example, except different spaces can be specified.

Now I am not an expert in CSS, so the third example might still be replicable by other means but it may not be a simple process.

Comment 18

[Frédéric Wang (:fredw)]

One thing to consider is that in practice we are bit constrained by the current status of math:

1) LaTeX is the de facto standard. And in LaTeX we put all the table formatting as an argument of the environment command  that generates the table. So it's a bit tedious for LaTeX-to-MathML converters to have to move all this formating to cell nodes, especially when they are fast stream filter like itex2MML where you are essentially handling a MathML string not a DOM tree. That's why LaTeX-to-MathML converters use columnspacing/rowspacing and the other alignment attributes already implemented.

2) MathML is still used in environment where CSS is not available. Or in polyfills like MathJax that don't work well with CSS. In particular, Wikipedia will rely on MathJax to do server-side LaTeX-to-MathML conversion and thus the MathML output served will contain columnspacing/rowspacing.

Of course, MathML is designed with 1) and 2) in mind. That's a bit a pain that we have to modify so much of the table code to do that but I don't really see how we can do otherwise without duplicating the code into the MathML table module...

Comment 19

[James Kitchener (:jkitch)]

Attachment: MathML mtable changes

Now passes tests.

The old padding CSS has been stripped out, but I'm not sure if explicitly zeroing it is correct.

A consequence of this patch is that css border-spacing on longer has any effect.

Comment 20

[James Kitchener (:jkitch)]

Attachment: Part 4: tests

Tested:
static and dynamic setting of attributes
Default values
Error handling (an unreadable value falls back to the default value) 
Pixel counting mochitest to ensure a spacing of 7px really is 7px.

Comment 21

[James Kitchener (:jkitch)]

Attachment: testcase for attachment 8377104

Testcase for the screenshot posted earlier in attachment 8377104 [details].

I've disabled padding and margin for everything, but cannot account for the gap between the <mn> border and the <mtd> border.

The attached patches ensure that rowspacing="0px" and rowspacing="10px" give a spacing beween <mtd> elements of 0 and 10px respectively, but there is an additional 0.5ex in vertical spacing between the <mn> elements.

Comment 22

[Frédéric Wang (:fredw)]

Comment on attachment 8380506 [details] [diff] [review]
MathML mtable changes

Review of attachment 8380506 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks that looks good to me. Some minor comments below.

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +365,5 @@
> +// Unitless values are permitted and provide a multiple of the default value
> +// Negative values are forbidden.
> +//
> +
> +// rowspacing

framespacing

@@ +386,5 @@
> +
> +#define DEFAULT_ROWSPACING_EX 1.0
> +#define DEFAULT_COLUMNSPACING_EM 0.8
> +#define DEFAULT_FRAMESPACING_ARG0_EM 0.4
> +#define DEFAULT_FRAMESPACING_ARG1_EX 0.5

what about using static const variables instead of macros?

@@ +435,5 @@
> +    }
> +
> +    // Grab the value found and process it.
> +    if (count > 0) {
> +

blank line not needed.

@@ +474,5 @@
> +
> +  NS_ASSERTION(aAttribute == nsGkAtoms::rowspacing_ ||
> +               aAttribute == nsGkAtoms::columnspacing_ ||
> +               aAttribute == nsGkAtoms::framespacing_,
> +               "Non spacing attribute passed");

I would move this assertion at the top of the function body.

@@ +494,5 @@
> +    aFrame->SetRowSpacingArray(valueList);
> +  } else if (aAttribute == nsGkAtoms::columnspacing_) {
> +    aFrame->SetColSpacingArray(valueList);
> +  } else { // framespacing_
> +    if (valueList.Length() >= 2) {

I think this should be ==. The spec/RelaxNG schema does not seem to accept more than 2 values.

@@ +660,5 @@
> +  nsPresContext* presContext = tableFrame->PresContext();
> +  if (aAttribute == nsGkAtoms::rowspacing_ ||
> +      aAttribute == nsGkAtoms::columnspacing_ ||
> +      aAttribute == nsGkAtoms::framespacing_ ) {
> +    nsMathMLmtableFrame* mathMLmtabFrame = do_QueryFrame(tableFrame);

could be the full word: mathMLmtableFrame

@@ +675,5 @@
> +    // clear any cached property list for this table
> +    presContext->PropertyTable()->
> +      Delete(tableFrame, AttributeToProperty(aAttribute));
> +    // Reparse the new attribute on the table.
> +    ParseFrameAttribute(tableFrame, aAttribute, true);

So I think these could be reorganized:

if aAttribute is nsGkAtoms::*spacing_ => ParseSpacingAttribute

else if aAttribute is nsGkAtoms::*align or nsGkAtoms::*lines => ParseFrameAttribute

else => ignore attributes that do not affect layout (return NS_OK)

@@ +848,5 @@
>  {
>    nsresult rv = nsTableFrame::SetInitialChildList(aListID, aChildList);
>    if (NS_FAILED(rv)) return rv;
>    MapAllAttributesIntoCSS(this);
> +  ParseSpacingAttributes(this);

I think the call to ParseSpacingAttributes can be move to MapAllAttributesIntoCSS, so that you don't need to do it here.

@@ +857,5 @@
>  nsMathMLmtableFrame::RestyleTable()
>  {
>    // re-sync MathML specific style data that may have changed
>    MapAllAttributesIntoCSS(this);
> +  ParseSpacingAttributes(this);

ditto

@@ +874,5 @@
> +    return 0;
> +  }
> +  if (aColIndex < 0 || aColIndex >= GetColCount()) {
> +    NS_ASSERTION(aColIndex == -1 || aColIndex == GetColCount(),
> +                 "Desired column beyond bounds of table + frame");

do you mean "table frame"? (without the +)

::: layout/mathml/nsMathMLmtableFrame.h
@@ +114,5 @@
> +  /** helper to get the cell spacing X style value */
> +  nscoord GetCellSpacingX(int32_t aColIndex) MOZ_OVERRIDE;
> +
> +  /** helper to get the combined cell spacing X style values of multiple
> +   *  columns

perhaps you could say the sum from aStartColIndex to aEndColIndex

Comment 23

[Frédéric Wang (:fredw)]

(In reply to James Kitchener (:jkitch) from comment #19)
> The old padding CSS has been stripped out, but I'm not sure if explicitly
> zeroing it is correct.
> 
> A consequence of this patch is that css border-spacing on longer has any
> effect.

IIRC for the other attributes, we tried to preserve the CSS when attributes are not specified explicitly. Maybe we can do the same here.

Comment 24

[James Kitchener (:jkitch)]

Attachment: Part 2: MathML mtable changes

The new table spacing code is now opt in.  If at least one of rowspacing, columnspacing or framespacing is specified the new spacing is adopted, otherwise it falls back to the old CSS implementation.

The code now reports a parsing error if the attribute value contains no lengths (although rowspacing="" will still opt in to the new table spacing system).

Comment 25

[James Kitchener (:jkitch)]

Attachment: Part 3: tests

A consequence of the changes in the previous patch is that dir-6 once again fails as it uses the old CSS implementation.  The test has been cloned to dir-6a which tests against the new system.

Dynamic tests involving transitioning from the old system to new (and vice versa) have been added.

Comment 26

[James Kitchener (:jkitch)]

Comment on attachment 8382070 [details] [diff] [review]
Part 2: MathML mtable changes

Flagging for review to get confirmation on behaviour change.

Comment 27

[Frédéric Wang (:fredw)]

(In reply to James Kitchener (:jkitch) from comment #26)
> Comment on attachment 8382070 [details] [diff] [review]
> Part 2: MathML mtable changes
> 
> Flagging for review to get confirmation on behaviour change.

OK, change sounds good. I'm expecting that specifiying no attributes behaves the same as the default MathML values. Do you know why it is not working in RTL mode and why your previous patch fixed that?

Comment 28

[James Kitchener (:jkitch)]

Comment on attachment 8380339 [details] [diff] [review]
Part1: Add arguments to GetCellSpacing[XY]

Cancelling review until further bugs are resolved.

Comment 29

[James Kitchener (:jkitch)]

Comment on attachment 8382070 [details] [diff] [review]
Part 2: MathML mtable changes

I haven't looked into the rtl problem yet.

Cancelling review as the patch is still buggy.  setting one of rowspacing/columnspacing/framespacing breaks rowlines and columnlines.

rowlines/columnlines draw the lines at the edge of the <mtd> element, which is no longer the midpoint between two cells.

Comment 30

[James Kitchener (:jkitch)]

Attachment: Part 2: MathML mtable changes

This patch applies on top of the patches from bug 975935.

The rowlines/columnlines attributes now render properly when used in conjunction with rowspacing/columnspacing/framespacing.  The additional area occupied by the row and column lines are considered as overflow for the table cells.

Comment 31

[James Kitchener (:jkitch)]

Attachment: Part 3: tests

Now includes a test for combined rowlines/columnlines and rowspacing et al.  It blacks out most of an mtable, leaving only the middle exposed.  Success occurs if something is drawn in the middle region.

I'm avoiding an explicit comparison between a CSS rendered table and a rowspacing etc. one as the two cases may differ due to rounding.

Comment 32

[James Kitchener (:jkitch)]

Comment on attachment 8380339 [details] [diff] [review]
Part1: Add arguments to GetCellSpacing[XY]

Reflagging for review.

Comment 33

[James Kitchener (:jkitch)]

Attachment: Part 2: MathML mtable changes

whitespace fixes.

Comment 34

[James Kitchener (:jkitch)]

Attachment: Part1: Add arguments to GetCellSpacing[XY]

whitespace fixes for the lines touched by my code.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to James Kitchener (:jkitch) from comment #17)
> My understanding is that two accomplish different things.  In the attached
> diagram:
> 
> left example sets mtd {padding:5px;}
> middle example sets mtable {border-spacing:5px} mtd{padding:0px;}
> right example sets rowspacing="5px 20px" columnspacing="5px 20px"
> framespacing="5px 5px" (using the patches from this bug)
> 
> padding adds the space inside the table cells, which are then placed
> touching each other, resulting in a solid block of red.
> border-spacing leaves 5px of white between the cells, resulting in a
> windowpane effect.
> rowspacing/columnspacing is similar to the border-spacing example, except
> different spaces can be specified.
> 
> Now I am not an expert in CSS, so the third example might still be
> replicable by other means but it may not be a simple process.

I think it could be done using invisible, collapsing borders. Could MathJax use that?

(In reply to Frédéric Wang (:fredw) from comment #18)
> One thing to consider is that in practice we are bit constrained by the
> current status of math:
> 
> 1) LaTeX is the de facto standard. And in LaTeX we put all the table
> formatting as an argument of the environment command  that generates the
> table. So it's a bit tedious for LaTeX-to-MathML converters to have to move
> all this formating to cell nodes, especially when they are fast stream
> filter like itex2MML where you are essentially handling a MathML string not
> a DOM tree. That's why LaTeX-to-MathML converters use
> columnspacing/rowspacing and the other alignment attributes already
> implemented.

If I'm right that we could use borders, then you could emit a scoped style sheet and give the cells classes corresponding to row and column numbers to get the streaming behavior.

> 2) MathML is still used in environment where CSS is not available. Or in
> polyfills like MathJax that don't work well with CSS. In particular,
> Wikipedia will rely on MathJax to do server-side LaTeX-to-MathML conversion
> and thus the MathML output served will contain columnspacing/rowspacing.

I don't like making browser design decisions driven by the needs of platforms where CSS isn't available. This happened in SVG for a long time and it forced duplication of functionality that ended up being pointless since all non-CSS-capable SVG viewers died out.

I'm not totally opposed to taking these patches but we have to consider the costs and benefits --- and alternatives --- carefully.

dbaron might have an opinion.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> I don't like making browser design decisions driven by the needs of
> platforms where CSS isn't available. This happened in SVG for a long time
> and it forced duplication of functionality that ended up being pointless
> since all non-CSS-capable SVG viewers died out.

Actually this isn't completely true, but it's true enough that we stopped caring about non-CSS-capable SVG viewers.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> I don't like making browser design decisions driven by the needs of
> platforms where CSS isn't available. This happened in SVG for a long time
> and it forced duplication of functionality that ended up being pointless
> since all non-CSS-capable SVG viewers died out.

Note that one option is that MathJax produces output containing both the attributes and the CSS, and we make sure that UAs support one or the other but not both.

Comment 38

[James Kitchener (:jkitch)]

(In reply to Frédéric Wang (:fredw) from comment #27)
> OK, change sounds good. I'm expecting that specifiying no attributes behaves
> the same as the default MathML values. Do you know why it is not working in
> RTL mode and why your previous patch fixed that?

The problem lies in CSS.
>mtd:first-child {
>  padding-left: 0em;
>}

With dir=rtl this selects the rightmost mtd, rather than the leftmost, so the padding is removed from the wrong side.

Comment 39

[Frédéric Wang (:fredw)]

(In reply to James Kitchener (:jkitch) from comment #38)
> The problem lies in CSS.
> >mtd:first-child {
> >  padding-left: 0em;
> >}
> 
> With dir=rtl this selects the rightmost mtd, rather than the leftmost, so
> the padding is removed from the wrong side.

OK, thanks that makes sense. Can you please try to use -moz-padding-start/-moz-padding-end/-moz-margin-start/-moz-margin-end instead?

Comment 40

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #39)
> (In reply to James Kitchener (:jkitch) from comment #38)
> > The problem lies in CSS.
> > >mtd:first-child {
> > >  padding-left: 0em;
> > >}
> > 
> > With dir=rtl this selects the rightmost mtd, rather than the leftmost, so
> > the padding is removed from the wrong side.
> 
> OK, thanks that makes sense. Can you please try to use
> -moz-padding-start/-moz-padding-end/-moz-margin-start/-moz-margin-end
> instead?

Sorry, I just realized that the problem is with the :first-child selector...

(this seems to be one more complication for MathML generators that would like to use CSS instead of rowspacing etc)

Comment 41

[James Kitchener (:jkitch)]

(In reply to Frédéric Wang (:fredw) from comment #40)
> (In reply to Frédéric Wang (:fredw) from comment #39)
> > (In reply to James Kitchener (:jkitch) from comment #38)
> > > The problem lies in CSS.
> > > >mtd:first-child {
> > > >  padding-left: 0em;
> > > >}
> > > 
> > > With dir=rtl this selects the rightmost mtd, rather than the leftmost, so
> > > the padding is removed from the wrong side.
> > 
> > OK, thanks that makes sense. Can you please try to use
> > -moz-padding-start/-moz-padding-end/-moz-margin-start/-moz-margin-end
> > instead?
> 
> Sorry, I just realized that the problem is with the :first-child selector...
> 
> (this seems to be one more complication for MathML generators that would
> like to use CSS instead of rowspacing etc)

Actually it works.  It is a neater solution than relying on :-moz-dir(rtl), which doesn't actually work for MathML due to the way the dir attribute is implemented.

Comment 42

[James Kitchener (:jkitch)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> I think it could be done using invisible, collapsing borders. Could MathJax
> use that?
> 

One potential issue is the rowlines and columnlines attributes.  These draw lines between the cells and are currently implemented by us and Webkit by selectively applying table borders.

To handle both rowspacing and rowlines with the same border mechanism would require specifying a border that is mostly invisible but has a solid or dashed line in a different colour along one edge.

Comment 43

[Frédéric Wang (:fredw)]

Note: I agree with Robert that in general we should try to reuse CSS when possible and deprecate MathML features that do not work well with the CSS layout (that was one of the motivation to limit the mstyle support btw). However, in this particular case:

a) I have not really studied whether it will be easy/possible to implement that with CSS properties only.

b) I'm not sure it will be convenient to handle for MathML generators. I don't understand the "scoped style sheet" since the generators can e.g. just return a <math>...</math> string not a whole HTML document. Also, the style will be lost by copy and paste of the equation, if it is not done by inline style="" attributes. Even if we do that with such attributes, that would make things difficult for stream converters.

c) in the short term I think we want to align on what MathJax and current tools do. If a) is not a problem, then in the long term we could either ask to change the spec or try to emulate the specific MathML table layout internally with some Web Components that would automatically do the CSS mapping.

Currently, the MathJax native MathML mode has some workaround for rowspacing/columnspacing that adds some CSS style but I'm not sure it always works well (in particular with the rowline/columnline attributes mentioned by James). This CSS style is added neither by MathJax in the SVG/HTML modes nor by the LaTeX-to-MathML server-side conversion that will be used in Wikipedia. If MathJax is changed to add these style in these modes too, I suspect it will conflict with MathJax's own table layout (which does not rely on the built-in CSS table layout). I'm not even sure the MathJax team will agree to modify the preprocessing to always/conditionally add these CSS rules. Finally, we want these attributes to work in other situations and tools where MathJax is not involved.

So at the moment, James' patch seems the best (or least bad) option to me. We already have similar things for rowalign/columnalign, except that here it modifies more things outside the MathML code.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Frédéric Wang (:fredw) from comment #43)
> a) I have not really studied whether it will be easy/possible to implement
> that with CSS properties only.

I think we should study that!

If it's hard, I don't mind taking these patches. But if it's easy, I don't think we should take these patches.

> b) I'm not sure it will be convenient to handle for MathML generators. I
> don't understand the "scoped style sheet" since the generators can e.g. just
> return a <math>...</math> string not a whole HTML document. Also, the style
> will be lost by copy and paste of the equation, if it is not done by inline
> style="" attributes. Even if we do that with such attributes, that would
> make things difficult for stream converters.

You can write
  <mtable>
    <style scoped>
      // style rules that only match the descendants of *this* mtable
    </style>
  </mtable>

> c) in the short term I think we want to align on what MathJax and current
> tools do. If a) is not a problem, then in the long term we could either ask
> to change the spec or try to emulate the specific MathML table layout
> internally with some Web Components that would automatically do the CSS
> mapping.

We really don't want to add any Web-exposed features as a short-term solution to anything. They will live forever.

> Currently, the MathJax native MathML mode has some workaround for
> rowspacing/columnspacing that adds some CSS style but I'm not sure it always
> works well (in particular with the rowline/columnline attributes mentioned
> by James). This CSS style is added neither by MathJax in the SVG/HTML modes
> nor by the LaTeX-to-MathML server-side conversion that will be used in
> Wikipedia. If MathJax is changed to add these style in these modes too, I
> suspect it will conflict with MathJax's own table layout (which does not
> rely on the built-in CSS table layout). I'm not even sure the MathJax team
> will agree to modify the preprocessing to always/conditionally add these CSS
> rules. Finally, we want these attributes to work in other situations and
> tools where MathJax is not involved.

I don't understand this. It seems much preferable to change MathJax and other "situations and tools" for MathML than to add unnecessary features to the Web platform.

Comment 45

[distler]

>I don't understand this. It seems much preferable to change MathJax and other "situations and tools" for MathML than to add unnecessary features to the Web platform.

If I may say so, I find this a little confusing.

The MathML Specification specifies certain attributes for the <mtable> elements which are not currently implemented in Gecko. The same effect (you say) can be achieved by judicious use of the HTML5 feature, <style scoped>. So, rather than Gecko implementing these attributes, tools which generate MathML should be changed to emit HTML5 <style scoped> elements, instead.

As the author of such a tool, this makes no sense to me. I strive to produce valid MathML3 output, and <style scoped> is certainly not part of MathML3. If you want to lobby the MathML WG to make <style scoped> part of MathML4, be my guest. But it doesn't seem to make sense to expect MathML generators to start emitting random snippets of HTML5, just because you don't like the MathML Spec, as currently written.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't look at the Web platform as an amalgam of many pieces that each need to be useful independently. That kind of thinking leads to duplication of functionality across those pieces. Therefore, where MathML specs introduce features that duplicate functionality available in other parts of the Web platform, I don't think those features should be implemented in any Web browser, and ideally they'd be removed from the spec.

Having said that, I agree it's unclear that rowspacing/columnspacing/framespacing should be considered to "duplicate" <style scoped> plus some judiciously chosen CSS rules.

Comment 47

[Frédéric Wang (:fredw)]

To add some input here, I just checked a few of the tools mentioned on https://developer.mozilla.org/en-US/docs/Web/MathML/Authoring:

- MathJax relies heavily on rowspacing/columnspacing. In particular Mathoid (server-side MathJax) will be used for native MathML on MediaWiki-based wikis.

- The two main LaTeX-to-(HTML+MathML) converters LaTeXML and tex4ht also use these attributes.

- For simple LaTeX-to-MathML converters: itex2MML, TeXZilla (intended to be used in Mozilla products and that we already integrated into MDN, Seamonkey, FirefoxOS and hopefully soon Thunderbird after bug 992127) and BlahTeX rely on these attributes too (actually this bug is mentioned in BlahTeX source code).

Of course, there are tons of such MathML generators and many others probably use these rowspacing/columnspacing attributes. Also, we have supported the similar rowalign/columnalign attributes for a very long time (probably since the first years of the MathML implementation?) and these align attributes are also used in the tools above as well as others (for example LibreOffice Math). So it seems consistent to implement rowspacing/columnspacing too.

So I would personally vote for taking this patch, because the fact that it is already used in practice by many tools and addresses some need seems more important that the potential theorically unpleasing feature duplication.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8384504 [details] [diff] [review]
Part1: Add arguments to GetCellSpacing[XY]

Review of attachment 8384504 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tables/nsTableCellFrame.cpp
@@ +815,5 @@
>    int32_t rowIndex;
>    firstCellInFlow->GetRowIndex(rowIndex);
>    int32_t rowSpan = aTableFrame.GetEffectiveRowSpan(*firstCellInFlow);
> +  nscoord cellSpacing = firstTableInFlow->GetCellSpacingY(rowIndex,
> +                                                          rowIndex + rowSpan - 1);

This doesn't seem right. if rowSpan is 1 then cellSpacing will be 0, which is a change in behavior.

::: layout/tables/nsTableFrame.cpp
@@ +3413,5 @@
> +  NS_ASSERTION(aStartColIndex <= aEndColIndex,
> +               "End index must not be less than start index");
> +  // Just pick one and multiply it out.  Tables where index matters will
> +  // override this function
> +  return GetCellSpacingX(0) * (aEndColIndex - aStartColIndex);

To be slightly more efficient here, have nsTableFrame implement a private, non-virtual GetCellSpacingX() with no parameters, and call it from here and from GetCellSpacingX(int32_t).

Same for GetCellSpacingY.

::: layout/tables/nsTableRowGroupFrame.cpp
@@ +1523,5 @@
>      }
>      if (parentRS && (tableFrame == parentRS->frame) && 
>          (parentRS->ComputedHeight() > 0) && (parentRS->ComputedHeight() < NS_UNCONSTRAINEDSIZE)) {
> +      nscoord cellSpacing =
> +        tableFrame->GetCellSpacingY(-1, std::max(-1, tableFrame->GetRowCount()));

This seems incorrect. startRowIndex should be used.

Also, presumably the std::max here should just be removed.

Comment 50

[James Kitchener (:jkitch)]

Attachment: Part 2: MathML mtable changes

rebased

Comment 51

[James Kitchener (:jkitch)]

Attachment: Part 3: tests

rebased

Comment 52

[James Kitchener (:jkitch)]

Attachment: Part1: Add arguments to GetCellSpacing[XY]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> Comment on attachment 8384504 [details] [diff] [review]
> Part1: Add arguments to GetCellSpacing[XY]
> 
> Review of attachment 8384504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/tables/nsTableCellFrame.cpp
> @@ +815,5 @@
> >    int32_t rowIndex;
> >    firstCellInFlow->GetRowIndex(rowIndex);
> >    int32_t rowSpan = aTableFrame.GetEffectiveRowSpan(*firstCellInFlow);
> > +  nscoord cellSpacing = firstTableInFlow->GetCellSpacingY(rowIndex,
> > +                                                          rowIndex + rowSpan - 1);
> 
> This doesn't seem right. if rowSpan is 1 then cellSpacing will be 0, which
> is a change in behavior.
> 

The use of (rowspan - 1) was the original behaviour.

> nscoord computedHeight = ((rowSpan - 1) * cellSpacing) - aVerticalBorderPadding;

According to the comments, CalcUnpaginagedHeight excludes border and padding from the calculated height, so a 0 cellspacing contribution for a rowspan of 1 seems reasonable.

> ::: layout/tables/nsTableFrame.cpp
> @@ +3413,5 @@
> > +  NS_ASSERTION(aStartColIndex <= aEndColIndex,
> > +               "End index must not be less than start index");
> > +  // Just pick one and multiply it out.  Tables where index matters will
> > +  // override this function
> > +  return GetCellSpacingX(0) * (aEndColIndex - aStartColIndex);
> 
> To be slightly more efficient here, have nsTableFrame implement a private,
> non-virtual GetCellSpacingX() with no parameters, and call it from here and
> from GetCellSpacingX(int32_t).
> 
> Same for GetCellSpacingY.
> 

Done.

> ::: layout/tables/nsTableRowGroupFrame.cpp
> @@ +1523,5 @@
> >      }
> >      if (parentRS && (tableFrame == parentRS->frame) && 
> >          (parentRS->ComputedHeight() > 0) && (parentRS->ComputedHeight() < NS_UNCONSTRAINEDSIZE)) {
> > +      nscoord cellSpacing =
> > +        tableFrame->GetCellSpacingY(-1, std::max(-1, tableFrame->GetRowCount()));
> 
> This seems incorrect. startRowIndex should be used.
> 

The code before modification calculates the total cellspacing for (tableFrame->GetRowCount() + 1) rows which I interpret as the entire table and the top and bottom borders.  I do not see how startRowIndex can be incorporated without 
a) changing the number of rows considered
or
b) or adding an offset which will mean considering the cell spacing of rows that do not exist.  

> Also, presumably the std::max here should just be removed.

Done.  There is now an assumption that the table has at least one row, but if that wasn't the case, how does the table have a non-zero height?

Comment 53

[James Kitchener (:jkitch)]

Comment on attachment 8380338 [details] [diff] [review]
Part 0: Fix to CalcUnpaginagedHeight

obsoleted wrong patch

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8439912 [details] [diff] [review]
Part1: Add arguments to GetCellSpacing[XY]

Review of attachment 8439912 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tables/nsTableFrame.h
@@ +380,5 @@
> +   */
> +  virtual nscoord GetCellSpacingX(int32_t aColIndex);
> +
> +  /** Sums the combined cell spacing between the columns aStartColIndex to
> +   *  aEndColIndex.

This comment needs to clarify exactly which cells' spacing is included.

@@ +392,5 @@
> +   */
> +  virtual nscoord GetCellSpacingY(int32_t aRowIndex);
> +
> +  /** Sums the combined cell spacing between the rows aStartRowIndex to
> +   *  aEndRowIndex.

ditto

Comment 55

[James Kitchener (:jkitch)]

Attachment: Part1: Add arguments to GetCellSpacing[XY]

Comments added

Comment 56

[James Kitchener (:jkitch)]

Attachment: Part 2: MathML mtable changes

Performance improvement for the two argument versions of GetCellSpacing[XY].

Beforehand it would iterate over every row/column specified by the arguments, calling the single argument version each time.

However as the length of the array of user specified row/column spacings is known, it is only necessary to iterate up to that length.  The remaining range can simply be multiplied by the last element in the array and added to the total.

For larger mtables this could save hundreds of virtual method calls.

Comment 57

[James Kitchener (:jkitch)]

Attachment: Part 3: tests

The Mochitest now supports HiDPI displays

Comment 58

[Frédéric Wang (:fredw)]

Comment on attachment 8440372 [details] [diff] [review]
Part 2: MathML mtable changes

Review of attachment 8440372 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +262,5 @@
> +    overflow.bottom = mathMLmtableFrame->GetCellSpacingY(rowIndex)/2;
> +  }
> +  return overflow;
> +}
> +

Can you keep consistent spacing around / + - operators?

@@ +545,5 @@
>    // Map mtable columnalign & columnlines.
>    ParseFrameAttribute(aTableFrame, nsGkAtoms::columnalign_, true);
>    ParseFrameAttribute(aTableFrame, nsGkAtoms::columnlines_, true);
>  
> +  ParseSpacingAttributes(aTableFrame);

For consistency, can you add a comment

// Map mtable rowspacing, columnspacing & framespacing

please?

Comment 59

[Frédéric Wang (:fredw)]

Comment on attachment 8440373 [details] [diff] [review]
Part 3: tests

Review of attachment 8440373 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I see you only introduced a small error tolerance.

Comment 60

[James Kitchener (:jkitch)]

Attachment: Part 2: MathML mtable changes

Comment 61

[James Kitchener (:jkitch)]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=b252cefa378e

One OOM related orange that as far as I can tell isn't caused by these changes, and wasn't present when the run was retriggered.

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f21c62b41eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/248103817c8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d70b7473bff7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b662e2341b4a

Comment 63

[Carsten Book [:Tomcat]]

sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=41782155&tree=Mozilla-Inbound

Comment 64

[Bill Gianopoulos [:WG9s]]

Is it just me or does that log make no sense as the numbering of differing pixels is way less than the max number.  How can this be a failure????

Comment 65

[Karl Tomlinson (:karlt)]

IIUC "max difference: 220" means that the difference in at least one color channel on at least one pixel is 220.

Comment 66

[James Kitchener (:jkitch)]

tablespacing-4.html turns out to be an existing bug.
https://tbpl.mozilla.org/?tree=Try&rev=a0d517965eed

Most of the existing mtable tests are marked random-if(B2G&&browserIsRemote).  Could this be the same issue?

Comment 67

[James Kitchener (:jkitch)]

Attachment: Part 4: Flag failing tests

Looking over the MathML reftest.list, it seems that B2G has a problem with dynamic tests.  This patch adds another four to the list of failing tests.

The tests in question involve the addition and removal of table cells (4) and modification to the spacing between table elements (5, 5a, 6).  tablespacing-4 exhibits the same failure mode with and without patches 0-2 applied.

Do these tests failures need to be fixed before this can land, or are they part of some larger known issue with B2G?

Comment 68

[Karl Tomlinson (:karlt)]

Comment on attachment 8441346 [details] [diff] [review]
Part 4:  Flag failing tests

Yes, this is most likely a separate existing bug.  It is not necessarily (and most likely not, given code is the same) a B2G-only problem but timing may happen to be different there.  It need not hold up landing the changes here.

Are the failures really random?
It would be better to mark them fails-if if they fail consistently, so we can enable the tests when they are fixed.

If they are random, then they may start randomly failing elsewhere too, but we can see how they go and annotate later if necessary.

Comment 69

[James Kitchener (:jkitch)]

Attachment: Part1: Add arguments to GetCellSpacing[XY]

rebased

Comment 70

[James Kitchener (:jkitch)]

Attachment: Part 3: tests

Forget to add a test to the reftest index.

Comment 71

[James Kitchener (:jkitch)]

Attachment: Part 4: Flag failing tests

The tests haven't passed in ~20 attempts (try/inbound), so marking as fails-if

Comment 72

[James Kitchener (:jkitch)]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=e577ef073ba4

I couldn't find anything that could be attributed to these changes.

Please apply patches in numerical order.

Comment 73

[James Kitchener (:jkitch)]

Attachment: Part 2: MathML mtable changes

A few whitespace changes per comment 58 that were missed earlier.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb90458ef6e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bdccca2ba59
https://hg.mozilla.org/integration/mozilla-inbound/rev/2406e2818db4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ed3c02d590b
https://hg.mozilla.org/integration/mozilla-inbound/rev/81f06450987a

Comment 75

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Should this have had an Intent to Implement?

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb90458ef6e6
https://hg.mozilla.org/mozilla-central/rev/8bdccca2ba59
https://hg.mozilla.org/mozilla-central/rev/2406e2818db4
https://hg.mozilla.org/mozilla-central/rev/1ed3c02d590b
https://hg.mozilla.org/mozilla-central/rev/81f06450987a

Comment 77

[James Kitchener (:jkitch)]

(In reply to :Ms2ger from comment #75)
> Should this have had an Intent to Implement?

Deferring to someone more senior.

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, I suppose it should have.

Comment 79

[Jean-Yves Perrier [:teoli]]

Updated docs (I didn't saw any pref, so I think it will be live in that release):
https://developer.mozilla.org/en-US/Firefox/Releases/33
https://developer.mozilla.org/en-US/docs/Web/MathML/Element/mtable

Comment 80

[Frédéric Wang (:fredw)]

For some reason, I can not edit the MDN page right now (js error in ckeditor?), but the MathML status page should also be updated:

https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/Status

Comment 81

[Jean-Yves Perrier [:teoli]]

Frédéric: thanks, updated.