Closed Bug 731667 Opened 12 years ago Closed 11 years ago

Rewrite mtable implementation to avoid use of _moz-* attributes

Categories

(Core :: MathML, defect, P3)

Product:
Core
Component:
MathML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: fredw, Assigned: qheaden)

References

(Blocks 1 open bug)

Details

(Keywords: helpwanted, student-project, Whiteboard: [mentor=fredw][lang=c++])

Attachments

(3 files, 39 obsolete files)

Part 2 - remove legacy code
4.53 KB, patch
Details | Diff | Splinter Review
Part 3 - tests
96.58 KB, patch
Details | Diff | Splinter Review
Part 1 - implement parsing and rendering
38.96 KB, patch
Details | Diff | Splinter Review 
So we want to rewrite implementation of rowalign, columnalign, rowlines and columnlines:

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

Currently, the big picture of our implementation is:

1) layout/mathml/nsMathMLmtableFrame.cpp contains classes for table, row and cell frames that inherit the corresponding frames in layout/tables/

2) The code in nsMathMLmtableFrame.cpp parses the attributes above. Then it takes care of attaching private attributes _moz-math-columnalign, _moz-math-rowalign, _moz-math-rowline, _moz-math-columnline to tabular elements.

3) The rules in mathml.css allow to apply a specific style to the tabular elements, according to the private attributes attached.

4) The parent code in layout/tables/ renders the table with the specific style.

Here is a proposal to get rid of these _moz-* attributes. Anyone working on this bug may be interested to continue with the related work of bug 330964.

Step I: Remove the rules in layout/mathml/mathml.css

Easy (but good starting point).

Step II: Parsing of tabular attributes

The property of a particular cell (e.g. rowalign) may depend on the attribute set on this cell, on the row containing this cell, on the mtable containing this cell or on a parent mstyle. Moreover, one attribute may describe the property of several cells. Thus, I don't think it will be easy to directly map the attributes to CSS style in content/mathml/. I propose to rewrite the way the property of each cell is parsed in layout/mathml/nsMathMLmtableFrame.cpp  in order to set only frame members describing the style and not private attributes. We probably want to use the CSS constants in nsStyleConsts.h (NS_STYLE_VERTICAL_ALIGN_TOP etc) instead of string values (as it is currently the case) to describe these properties.

Step III: Applying the stylistic rules

I don't think it's possible to modify the style context attached to a frame from the layout/mathml/ code. The only place where I see that is via the pseudo-class :-moz-math-increment-script-level to increase -moz-script-level, but it does not seem a good idea to do so for each tabular attributes. I suggest to reimplement the layout/mtable/ code in layout/mathml/nsMathMLmtableFrame.cpp, by overriding methods/classes. IIUC, we have:

  - the rowalign attribute relies on vertical-align rules which are read in nsTableCellFrame::GetVerticalAlign. We could override this function in nsMathMLmtdFrame to use the info from the frame members set in step I.

  - the rowlines/columnlines attributes rely on border rules which are painted in nsCSSRendering::PaintBorder called from the nsDisplayBorder class, used in nsTableCellFrame::BuildDisplayList. Maybe it will be possible to modify nsCSSRendering::PaintBorder to do something specific for mtd cells, again using frame members set in step I.

  - the columnalign attribute relies on text-align rules. I didn't check exactly where they are used in the layout/table/ code, but I hope we can do something similar.

Step IV: Removing the workaround for these private attributes in the parser, serializer etc 

The _moz-math-font-style attribute will still be used (I suspect we need to fix bug 114365 before being able to remove that one), so it may not be possible to do this step immediately. Anyway, Henri Sivonen knows better than me what to do exactly here.
Hopefully, this will also fix bug 491384, which has been reported several times by MathJax users.
One question is whether getComputedStyle should reflect the effects of the attributes. If it should, then your approach of keeping the data out of the style system won't work.

Also, how should !important CSS rules interact with the MathML properties?
I don't think the MathML REC really describes how MathML should behave with respect to getComputedStyle and CSS rules in general. We have many MathML attributes without a CSS equivalent, that are not implemented via CSS and for which these two points do not make sense. However, I think you are right that we should try to take them into account for MathML attributes that are implemented via CSS properties. In any cases, we should definitely do something to get rid of these private attributes, so comment 0 is at least a first proposal.

Other approaches that I imagined:

- Use CSS pseudo rules (as we do for -moz-math-increment-script-level) instead of the private attributes. But that does not seem very clean or efficient and it looks that it will require to add one rule for each possible values of the attributes.

- map everything in content/mathml/. I know how to map an attribute to change the style on a given element, but I'm not sure it is so easy when we want to change the style of the descendants of this element. It seems that something is done for <table> and <td> cells but even there I see a comment suggesting it is not done very cleanly: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTableElement.cpp#953
Blocks: 491384
Keywords: student-project
Some reftests for the mtable element can be found here:

http://devel.mathjax.org/testing/testsuite/MathMLToDisplay/Presentation/TablesAndMatrices/

it should be possible to import them to write tests for this bug and for other MathML tabular attributes/elements.
Assignee: nobody → qheaden
Attached file testcase (obsolete) — 
Here is a testcase. Apparently, attributes are not taken from math/mstyle.
That would be useful to have such a test for attributes that are added/removed with Javascript.
Attached patch Alpha Patch 1 (obsolete) — — Splinter Review 
This is an alpha patch. I'm just submitting it so that you can see what progress I am making, and I can be sure whether or not I am moving in the right direction. This patch completely removes rowalign rules from the mathml.css file, and it sets the rules from C++. The only problem with it is that it does not currently accept rules from <mstyle> or <mtable> elements, which is my next goal. Please let me know if I am moving in the right direction with my code.
Attachment #627874 - Flags: feedback?(fred.wang)
Comment on attachment 627874 [details] [diff] [review]
Alpha Patch 1

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

You probably should be using mapped attributes here to map presentation attributes into CSS style rules as much as possible.

See how nsHTMLTableElement.cpp implements its attributes, using nsHTMLTableElement::IsAttributeMapped and nsHTMLTableElement::GetAttributeMappingFunction.

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +61,5 @@
>  //
>  // <mtable> -- table or matrix - implementation
>  //
>  
> +static PRInt8 parseRowAlignItem(const nsString& aString)

Functions start with a capital letter.

@@ +69,5 @@
> +  }
> +  if (aString.EqualsLiteral("bottom\0")) {
> +    return NS_STYLE_VERTICAL_ALIGN_BOTTOM;
> +  }
> +  if (aString.EqualsLiteral("center\0")) {

These \0s are not needed.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
>
> You probably should be using mapped attributes here to map presentation
> attributes into CSS style rules as much as possible.
> 

The problem here is that it is complicated. One can specify

<mtable columnalign="left right center">

and you have to split the column attribute to apply the style value to each column. One can also use

<mtr columnalign="left right center">

on a row and

<mtd columnalign="left">

on a cell. To complicate things any further, one can use

<mstyle columnalign="left right center">

to apply the style to all descendants <mtable>.

For HTML it is a bit more simple but as I said in comment 3, even here it seems not to be done very cleanly:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTableElement.cpp#923

Also, it seems to use some HTML DOM specific functions to browse the table and we don't have this in content/mathml/ (we only have one class for all MathML elements)

Personally, I'm happy if Quentin cleans up the implementation on the layout side and removes the use of these _moz-* attributes. I don't think not mapping on the content side will be a too serious issue.
Comment on attachment 627874 [details] [diff] [review]
Alpha Patch 1

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

Thank you Quentin, that looks good in general.

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +244,5 @@
> +  nsIContent* tableContent = aTableFrame->GetContent();
> +  nsIContent* rowContent = aRowFrame->GetContent();
> +
> +  // TODO - Grab a pointer to the parent mstyle
> +

You don't need a pointer to the parent mstyle, GetAttribute will do it for you. See below.

@@ +252,5 @@
> +  if (!nsMathMLFrame::GetAttribute(rowContent, nsnull,
> +                                   nsGkAtoms::rowalign_, parentAttr))
> +    nsMathMLFrame::GetAttribute(tableContent, nsnull,
> +                                nsGkAtoms::rowalign_, parentAttr);
> +

The MathML spec says:

"Similarly, when rowalign, columnalign, or groupalign are specified on an mstyle element, they apply only to the mtable element, and not the mtr, mlabeledtr, mtd, and maligngroup elements."

So you have to pass aTableFrame->mPresentationData.mstyle as the second parameter of nsMathMLFrame::GetAttribute(tableContent, ... in order to get the attribute from a possible mstyle ancestor.

@@ +269,5 @@
> +    FrameProperties props = cellFrame->Properties();
> +    if (!cellAttr.IsEmpty())
> +      props.Set(RowAlignProperty(), new nsValueList(cellAttr));
> +    else if (!parentAttr.IsEmpty())
> +      props.Set(RowAlignProperty(), new nsValueList(parentAttr));

Here you are setting a frame property on each cell when the alignment is specified on the row/table. I'm not sure what would be the best, but maybe to use less memory you should only set the frame property on the tables (a list of values in that case), the rows and the cells that have an attribute.

The accepted syntax for the mtable attribute is 

("top" | "bottom" | "center" | "baseline" | "axis") +

i.e. it is a list of attributes that you have to split and apply the appropriate one to each row. More precisely, the REC says:

"In this context, a single value specifies the value to be used for all rows (resp., columns or gaps). A list of values are taken to apply to corresponding rows (resp., columns or gaps) in order, that is starting from the top row for rows or first column (left or right, depending on directionality) for columns. If there are more rows (resp., columns or gaps) than supplied values, the last value is repeated as needed. If there are too many values supplied, the excess are ignored. "

Currently, when you create a nsValueList the attribute is split and stored as a list of strings. To use less memory, I think you should use a single PRUint8 value (obtained from ParseRowAlignItem) for mtr/mtd and a list of PRUint8 values for mtable.

@@ +935,5 @@
> +    return parseRowAlignItem(valueList->mData);
> +  else
> +    return NS_STYLE_VERTICAL_ALIGN_BASELINE;
> +}
> +

Here, if you set frame properties on the table/row too, as I suggest above, then you have to fetch the frame property value from a parent mtable/cell.

Also, if the frame properties are stored as PRUint8, you won't need to call ParseRowAlignItem here.
Attachment #627874 - Flags: feedback?(fred.wang) → feedback+
> Here you are setting a frame property on each cell when the alignment is
> specified on the row/table. I'm not sure what would be the best, but maybe
> to use less memory you should only set the frame property on the tables (a
> list of values in that case), the rows and the cells that have an attribute.
> 

Another reason why you should attach frame properties on mtable/mtr elements is to handle dynamic changes. When you remove/modify the rowalign attribute on a mtable/mtr element, you have to recalculate the values on each descendant cell with your method. If you use frame property on mtable/mtr too, you just have to update the frame property on the modified mtable/mtr/mtd. In GetVerticalAlign you'll have to verify at most one or two additional ancestors, which is still fast.
Attached patch Alpha Patch 2 (obsolete) — — Splinter Review 
Here is my second alpha-stage patch. This patch changes the way the rowalign frame properties are set. Instead of setting the property on every single cell, I set the property on each cell, row, and table as needed.

Also, I had trouble gaining access to a valid mPresentationData pointer. For some reason, when I query the mtable's outer frame and get a pointer to its nsIMathMLFrame interface, the mPresentationData pointer is always null, even when an mstyle surrounds the table. So as a workaround, I simply got a pointer to the parent of the mtable's outer frame, and checked whether or not it is actually an mstyle frame. If it is an mstyle frame, I passes it to nsMathMLFrame::GetAttribute(). If anyone knows why this could be happening, please let me know.

Finally, this patch does not yet support multi-valued attributes such as rowalign="top bottom center". I am working on that now, and will include it in my next patch. Please let me know if I am heading in the right direction with this patch.

Thanks.
Attachment #627874 - Attachment is obsolete: true
Attachment #631617 - Flags: feedback?(fred.wang)
Comment on attachment 631617 [details] [diff] [review]
Alpha Patch 2

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

Thank you Quentin. I already gave you indications about multi-valued attributes in my previous mail. Below are some comments on this patch.

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +72,5 @@
> +  }
> +  if (aString.EqualsLiteral("center")) {
> +    return NS_STYLE_VERTICAL_ALIGN_MIDDLE;
> +  }
> +  // XXXfw axis is not supported

I think that should be XXXqheaden now you took the bug

@@ +171,5 @@
> +{
> +  delete static_cast<PRUint8*>(aPropertyValue);
> +}
> +
> +NS_DECLARE_FRAME_PROPERTY(RowAlignProperty, DestroyPRUint8Value)

Perhaps find a better name for the type of frame properties e.g. DestroyTableProperties instead of DestroyPRUint8Value?

@@ +262,5 @@
> +                                nsGkAtoms::rowalign_, tableAttr);
> +  else
> +    nsMathMLFrame::GetAttribute(tableContent, nsnull, nsGkAtoms::rowalign_,
> +                                tableAttr);
> +

> Also, I had trouble gaining access to a valid mPresentationData pointer. For some reason, when I query the mtable's outer frame and get a pointer to its nsIMathMLFrame interface, the mPresentationData pointer is always null

I'm not really sure of the frame hierarchy, this has to be checked carefully. Maybe you can ignore mstyle for now.

Have you tried to use do_QueryFrame to case nsMathMLmtableOuterFrame to a nsIMathMLFrame and then use GetPresentationData? Or maybe doing this cast with a parent/child frame of nsMathMLmtableOuterFrame?

@@ +982,5 @@
> +    if (alignmentPtr) {
> +      alignment = *alignmentPtr;
> +      currentFrame = nsnull; // A null frame pointer exits the loop
> +    }
> +    else {

should be "} else {"

@@ +992,5 @@
> +  }
> +
> +  return alignment;
> +}
> +

You'll probably have to share this implementation between the various mtable attributes, maybe distinguishing vertical/horizontal properties. Something to think about later.
Attachment #631617 - Flags: feedback?(fred.wang) → feedback+
Comment on attachment 631617 [details] [diff] [review]
Alpha Patch 2

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

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +72,5 @@
> +  }
> +  if (aString.EqualsLiteral("center")) {
> +    return NS_STYLE_VERTICAL_ALIGN_MIDDLE;
> +  }
> +  // XXXfw axis is not supported

I think it should just be TODO (bug ######)

@@ +259,5 @@
> +
> +  if (mstyleContent->Tag() == nsGkAtoms::mstyle_)
> +    nsMathMLFrame::GetAttribute(tableContent, mstyleFrame, 
> +                                nsGkAtoms::rowalign_, tableAttr);
> +  else

{}

@@ +289,5 @@
> +
> +  // Cell rowalign parsing
> +  nsIFrame* cellFrame = aRowFrame->GetFirstPrincipalChild();
> +
> +  while(cellFrame) {

while (

@@ +975,5 @@
> +  nsIFrame* currentFrame = (nsIFrame*)this;
> +
> +  while (currentFrame) {
> +    FrameProperties props = currentFrame->Properties();
> +    PRUint8* alignmentPtr = (PRUint8*)props.Get(RowAlignProperty());

static_cast

@@ +985,5 @@
> +    }
> +    else {
> +      if (IsTable(currentFrame->GetStyleDisplay()->mDisplay))
> +        currentFrame = nsnull; // Exit the loop when we reach an <mtable>
> +      else

{}
While you are on this, perhaps you can update this code

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmtableFrame.cpp#554

and add that the MathML REC says:

"When the align attribute is set with mstyle, it applies only to the munder, mover, and munderover elements, and not to the mtable and mstack elements."

(and GetAttribute can be replaced by GetAttr I think)
Attached file testcase (obsolete) — 
Add missing <mtext> elements.
Attachment #622314 - Attachment is obsolete: true
(In reply to Frédéric Wang (:fredw)
from comment #5)
> Here is a testcase. Apparently, attributes are not taken from math/mstyle.
> That would be useful to have such a test for attributes that are
> added/removed with Javascript.

MathJax creates MathML trees dynamically, but forces a reflow after the creation of a MathML tree in order to workaround bug 491384 and similar:
https://github.com/mathjax/MathJax/issues/260

I created a MathJax branch without this workaround. Attachment 633989 [details] renders the testcase using this MathJax branch. I see that:
- rowalign=top works on mtd/mtr but not mtable
- columnalign=right works on mtd but not mtr/mtable
Attached patch Alpha Patch 3 (obsolete) — — Splinter Review 
Here is the third alpha-stage patch. This patch rewrites parsing for the columnalign attributes, and changes the rendering code to correctly use the columnalign properties set on the mtable cell frames.

In the nsLineLayout constructor, I originally used a dynamic_cast, but dynamic_cast failed to compile on Linux because of the -fno-rtti switch, so I switched to using a static_cast. After some debugging, it seems that the aOuterReflowState->frame is always a child of nsBlockFrame, although I could be wrong on this.
Attachment #631617 - Attachment is obsolete: true
Attachment #635427 - Flags: feedback?(fred.wang)
(In reply to Quentin Headen from comment #18)
> In the nsLineLayout constructor, I originally used a dynamic_cast, but
> dynamic_cast failed to compile on Linux because of the -fno-rtti switch, so
> I switched to using a static_cast. After some debugging, it seems that the
> aOuterReflowState->frame is always a child of nsBlockFrame, although I could
> be wrong on this.

If there is some doubt, do_QueryFrame is what you want:
http://mxr.mozilla.org/mozilla-central/ident?i=do_QueryFrame
Attached patch Alpha Patch 3.5 (obsolete) — — Splinter Review 
This patch makes one small change. It uses do_QueryFrame()in the nsLineLayout constructor, as was suggested in the above comment by Karl, to determine whether or not aOuterFrameReflow->frame is actual a descendant  of nsBlockFrame. This removes the need of a dynamic_cast.
Attachment #635427 - Attachment is obsolete: true
Attachment #635427 - Flags: feedback?(fred.wang)
Attachment #635596 - Flags: feedback?(fred.wang)
Comment on attachment 635596 [details] [diff] [review]
Alpha Patch 3.5

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

I didn't look at the changes in details, but IIUC the only difference is that you rewrote columnalign and that looks good. Here are some things to consider now and later (some previous comments still hold):

1) As I told you, I'm wondering whether that would be easier to make nsIFrame::GetStyle##name_ virtual. But I'm not sure reviewers will accept this change.
2) As Ms2ger suggested, perhaps you can add a "TODO (bug ######)" comment for rowalign=axis and open a bug.
3) The attributes should also be supported on mstyle/math. However, that does not seem to be the case currently, so you can handle that later. Can you please open a new bug and add a TODO comment for this too? Then the XXX comment about mstyle in nsMathMLmtableOuterFrame::Reflow can be handle in this bug too (comment 14).
4) Multiple values and rowline/columnline should be supported. That would be helpful to write testcases for these features...
5) You should try as much as possible to share the code in nsMathMLmtdInnerFrame::InitializeStyleTextInLine(), in nsMathMLmtdInnerFrame::InitializeStyleTextInLine (and in future functions that will take care of rowline/columnline attributes).
6) Dynamic changes must be handled correctly. Attachment 633989 [details] test dynamic changes of the attribute and does not seem to work correctly at the moment (perhaps you can write simpler testcase that do not rely on MathJax). I'm curious to know whether your patch improves the situation. However, I suspect you'll need to update the various *::AttributeChanged functions.
7) If your patch becomes too big, you may split it in several small patches. That's easier to handle for reviewers. Comment 0 contains a suggestion on how to split the work and thus the patches.
8) You will have to write reftests for all these attributes (see comment 4). As you know Andrii is working on dynamic MathML so he can explain you how to write dynamic versions too.
Attachment #635596 - Flags: feedback?(fred.wang) → feedback+
Blocks: row-column-spacing
Blocks: 768817, 768819
Attached file testcase (Multi-valued attributes) (obsolete) — 
This is a modified version of the original testcase attachment that singles out multi-valued rowalign and columnalign attributes on the <mtable> element.
Attached patch Patch 4.0 (obsolete) — — Splinter Review 
This new version of the patch adds support for parsing of multi-valued rowalign and columnalign attributes on mtable elements. I still need to add support for multi-valued columnalign attributes on mtr elements, but that will be coming shortly in the next patch.

It also makes reference to two other bugs I created relating to adding support for "axis" alignment, and parsing rowalign and columnalign attributes found on mstyle/math elements.
Attachment #635596 - Attachment is obsolete: true
Attachment #638443 - Flags: feedback?(fred.wang)
Comment on attachment 638443 [details] [diff] [review]
Patch 4.0

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

Thank you Quentin. I just looked at your approach to handle multiple values and didn't check the whole patch.

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +107,5 @@
> +  }
> +
> +  return alignmentValues;
> +}
> +

So what you are doing here is creating a nsTArray<PRUnichar*> array from aString and then creating a nsTArray<PRInt8> from that array. As I suggested by mail, you can actually directly create nsTArray<PRInt8> from aString: you may adapt SplitString to call the ParseRowAlignItem function before appending each element.

Of course, you may want to share the implementation between all the attributes and add an argument to SplitString to specify the parsing function.

@@ +376,5 @@
> +    } else { // If we have a multi-valued attribute.
> +      nsTArray<PRInt8>* alignmentList = ParseColumnAlignItemList(tableAttr);
> +
> +      FrameProperties props = aTableFrame->Properties();
> +      props.Set(ColumnAlignPropertyList(), alignmentList);      

I'm wondering if it's really worth distinguishing the cases of single/multiple values. The former code only created list of strings, so I think you can just always create list of integers instead. That would make thing simpler here and in nsMathMLmtdFrame::GetVerticalAlign.

@@ +1101,5 @@
> +const nsStyleText* nsMathMLmtdInnerFrame::InitializeStyleTextInLine()
> +{
> +  nsStyleText* styleText = const_cast<nsStyleText*>(GetStyleText());
> +
> +  // Set the deafult alignment in case nothing was specified

default
Attachment #638443 - Flags: feedback?(fred.wang) → feedback+
Status: NEW → ASSIGNED
Attached file testcase (obsolete) — 

        Attachment #633988 -
        Attachment is obsolete: true

        Attachment #633989 -
        Attachment is obsolete: true

        Attachment #638437 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 5 (Broken) (obsolete)
        — 
          — Splinter Review
      

    


  
This patch works, but it is broken when it comes to rowline and columnline support. The reason why I am posting this patch is because I have been battling this rowline/columnline problem for a while and I need some help.

I am going to post a picture of the issue I am having. Basically, I have added a virtual function called ProcessBorders to nsTableCellFrame so that nsMathMLmtdFrame (a child of nsTableCellFrame) can implement its own method of rendering borders. Please note that the code I placed in nsMathMLmtdFrame::ProcessBorders is not complete functionally, but I coded it that way to demonstrate the problem I am having.

In a nutshell, every table element on the web page is getting borders drawn on it, but it is happening because of the ProcessBorders function in nsMathMLmtdFrame. I want this function to only affect MathML cells, not normal table cells.

Can someone please point me in the right direction with this?

    
    

    
  

      
      
      
      

        Attached image
          
        Rowline/Columnline Border Problem (obsolete)
        — 
      

    


  
Here is a picture of the problem I described in the previous comment.

    
    

    
  
In nsMathMLmtdFrame::ProcessBorders, it seems that you modify the nsStyleBorder property (you did the same in the previous version of your patch that you sent by mail, which relied on nsDisplayGeneric). I'm wondering if you should rather create a copy of nsStyleBorder and pass it to (an adapted version of) nsDisplayBorder.

    
    

    
  
I don't know the style system very well, but I guess the data structure for the border property is shared in some way between frames and when you modify the nsStyleBorder for the mtd it is also modified for all the descendants and ancestors, or something like that.

    
    

    
  
@Frédéric

Your guess given in your last comment was actually the issue! I guess that's why GetStyleBorder() is a const function, because the style border pointer is shared among the page elements.

I created a new child class of nsDisplayBorder, and I created a new, specialized nsStyleBorder within its overridden Paint() function. The borders are now drawn around only the MathML cells. I will continue to implement the remaining functionality of the rowline/columnline code.

    
    

    
  
Uh.. yes.  If you're using const_cast, chances are your code is going to be buggy unless you _really_ know what you're doing.

There's also other const_cast in the patch that I recommend removing.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 6 (obsolete)
        — 
          — Splinter Review
      

    


  
Well, I am happy to say that Patch 6 is a feature-complete patch. I have re-implemented rowalign, columnalign, rowline, and columnline. Once again, thanks everyone for helping me out, especially with the rowline and columnline issues I was having.

Although this patch is feature complete, I will have to see if the code is in good shape. I will also start work on the tests.

        Attachment #638443 -
        Attachment is obsolete: true

        Attachment #651199 -
        Attachment is obsolete: true

        Attachment #651551 -
        Flags: feedback?(fred.wang)

    
    

    
  
Comment on attachment 651551 [details] [diff] [review]
Patch 6

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

Thank you very much for your help on this, Quentin. I'm glad to hear we finally have a working patch for this bug. However, I think it still remains to review and test this patch carefully, and that can take some time.

Unfortunately, we have almost no reftests for mtable elements and that will be hard to detect if your patch breaks anything. So I would suggest you to write the reftests based on the testcase and on comment 4, plus other features discussed in this bug (like what happens when the user redefine the style?). One of my hope is that your patch will also fix some reflow issues especially when it comes to dynamic changes. For those, I would also like to have reduced tests that do not rely on MathJax.

As for the review, I have already given feedback and guidance several times so I think that would be better if someone else could do it. I guess Karl is the most appropriate person for the MathML part. And we'll probably need another review from someone who knows well frame and styling stuff. To help reviewing the patch, that would be good to split it into several pieces. Here is a suggestion:

1) One patch for the reftests
2) One patch that removes the code from mathml.css, the atoms from content/base/src/nsGkAtomList.h (please do this BTW).
3) The changes related to parsing (in nsMathMLmtableFrame.cpp and not included in patch 4). This part will need more work and feedback/review.
4) The changes related to the rendering like InitializeStyleTextInLine, GetVerticalAlign, ProcessBorders, nsDisplaymtdBorder

One note on code styling: you have a couple of places where you use extra spaces at the end of a line like the first change in "layout/mathml/nsMathMLmtableFrame.h" or an empty line made of spaces. Sometimes, the line length of new code is greater than 80 chars (like in "ExtractStyleValues"). Please avoid these kinds of change.

::: layout/generic/nsBlockFrame.h
@@ +279,5 @@
>    /**
> +    * This method is used by the nsLineLayout class, and can be overriden so
> +    * that children of the nsBlockFrame class can specify their own special
> +    * modifications to their style text, and have it returned to the caller.
> +    */

Perharps specify this is only used for the implementation of MathML's columnalign.

::: layout/mathml/mathml.css
@@ -342,5 @@
> -/* internal settings to support 'rowlines' and 'columnlines'. To
> -   achieve the recommended behavior, the back-end code will avoid setting
> -   unsuitable rules on the cells on the first row and the first column.
> -   In general, however, authors can use the 'border' property of CSS to
> -   achieve varying effects down to the level of the table cell. */

So we're likely to lose this feature (at least the the border width). I suspect the MathML spec does not specify what happens when table style and mtable attributes are used together but the Math WG would probably say the latter take precedence. But I don't think it's a problem if we consider the former in priority. So maybe in nsDisplaymtdBorder::Paint, you should verify that the style is not already set to something before calling the SetBorderStyle member. Perhaps for the other properties too.

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +68,5 @@
>  {
>    nsString             mData;
>    nsTArray<PRUnichar*> mArray;
>  
> +  nsValueList(const nsString& aData) {

I'm not sure what this change is about. But I think you can now remove this nsValueList struct, as well as all the code related to the old parsing like SplitString, DestroyValueList, AttributeToProperty, nsGkAtoms::_moz* etc and update the comments. In particular, you need to remove the corresponding calls from the AttributeChanged functions and replace it by your new parsing functions. I haven't looked yet in detail how you map attributes to frame properties, but I suspect after some clean up and refactoring, you could write something *much better* than the current code.

The dynamic testcase (based on MathJax) only tests DOM element/attribute creation, you'll probably detect more with other dynamic tests for DOM element/attribute removal and DOM attribute modification.

@@ +116,5 @@
> +      start = ++end;
> +    }
> +    return styleArray;
> +}
> +

Would it be possible to parse the string without allocating these stringCopy and stringPiece strings ; plus do it in read-only mode (i.e. with BeginReading and EndReading)? I think you can just set start and end appropriately and call "Substring" to pass the fragment delimited by the start/end iterators.

See also https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide

@@ +189,5 @@
> + nsTArray<PRInt8>* lineList = ExtractStyleValues(aString,
> +                                ParseLinesItem);
> +  return lineList;
> +}
> +

Instead of all these functions, I'm wondering if you could just modify ExtractStyleValues to use an enum as the second parameter and use a switch on this enum to set the styleValue.

@@ +229,5 @@
>  }
>  
> +/* This method looks for a proprety that applies to a cell, but it looks
> + * recursively because some cell properties can come from the cell, a row,
> + * a table, etc. This function searches through the heirarchy for a property

s/proprety/property/
s/heirarchy/hierarchy/

@@ +245,5 @@
> +    FrameProperties props = currentFrame->Properties();
> +    propertyData = props.Get(aFrameProperty);
> +    bool frameIsTable = 
> +         (currentFrame->GetStyleDisplay()->mDisplay == NS_STYLE_DISPLAY_TABLE);
> +

That does not seem a good idea to rely on the display property. One could easily set / unset this property with CSS to any element. Maybe relying on the frame tag is better, although I'm not sure that would distinguish inner and outer frame tables so a bit more may be needed. That said, there are DEBUG_VERIFY_THAT_FRAME_IS_* controlling that the CSS property is correctly set, so maybe that's ok...

@@ +317,5 @@
> +        styleBorder.SetBorderStyle(NS_SIDE_TOP,
> +                      rowLinesList->ElementAt(rowIndex - 1));
> +      } else {
> +        styleBorder.SetBorderStyle(NS_SIDE_TOP,
> +                      rowLinesList->ElementAt(listLength - 1));

Here and elsewhere, I would quote the Math spec about the fact that we repeat the last value.

@@ +375,5 @@
>    DEBUG_VERIFY_THAT_FRAME_IS(aRowFrame, TABLE_ROW);
>    PRInt32 rowIndex = ((nsTableRowFrame*)aRowFrame)->GetRowIndex();
> +  nsAutoString tableAttr, rowAttr, cellAttr;
> +
> +  // TODO Bug 768819: Also parse row attributes on <mstyle>/<mtable> frames.

I guess you mean <math> instead of <mtable>

@@ +381,5 @@
> +  // Table rowalign parsing
> +  nsIContent* tableContent = aTableFrame->GetContent();
> +
> +  nsMathMLFrame::GetAttribute(tableContent, nsnull, nsGkAtoms::rowalign_,
> +                                tableAttr);

Here and below, use GetAttr instead of GetAttribute as for the moment we don't want to care about attributes coming from <mstyle>/<math> and they don't apply to all tabular elements anyway.

@@ +385,5 @@
> +                                tableAttr);
> +
> +  if (!tableAttr.IsEmpty()) {
> +    const PRUnichar whitespace = ' ';
> +    

whitespace seems unused.

@@ +389,5 @@
> +    
> +    // Remove leading and trailing whitespace to ensure that spaces are found
> +    // only within the string.
> +    tableAttr.CompressWhitespace(true, true);
> +

Note that CompressWhitespace does a bit more that trimming spaces from both ends, it also compresses successive whitespaces inside the string into a "0x20" space and that's what is assumed in the old SplitString function.

I guess you could also just skip the whitespaces in ExtractStyleValues (with the help of nsCRT::IsAsciiSpace).

@@ +475,5 @@
> +  nsMathMLFrame::GetAttribute(tableContent, nsnull, nsGkAtoms::columnalign_,
> +                                tableAttr);
> +  if (!tableAttr.IsEmpty()) {
> +    const PRUnichar whitespace = ' ';
> +    

whitespace seems unused.

@@ +1217,5 @@
> +  }
> +
> +  styleText->mTextAlign = alignment;
> +  return const_cast<nsStyleText*>(styleText);
> +}

Yes, as Boris said, this does not look quite right as you are modifying the nsStyleText. I think for the MathML case you'll need to allocate a new nsStyleText with the modified alignment property in nsLineLayout's constructor and release it in nsLineLayout's destructor.

        Attachment #651551 -
        Flags: feedback?(fred.wang) → feedback+

    
  

        Attachment #651200 -
        Attachment is obsolete: true

    
    

    
  
I'm now working on the reftests. Where should I place them? Would layout/mathml/tests be good?

    
    

    
  
Please ignore my last question, I realized that the tests need to go into layout/reftests/mathml.

    
    

    
  
I don't know if that could help to write "reduced" dynamic tests, but here is what I guess is happening with the testcase:

- the page contains some MathML code.
- Gecko renders this MathML. To do that, it adds _moz-* attributes to the DOM
- MathJax removes the MathML DOM tree and parses it into its own internal structure (without filtering the _moz-* attributes)
- MathJax reinjects a new MathML DOM tree from what is stored in its internal structure (still without filtering the _moz-* attributes)
- Gecko tries to render the MathML again.

So maybe the presence of _moz-* attributes in the new MathML tree is what is disturbing Gecko.

    
    

    
  
I am going to release the new, split patches. I am only doing this so that my tests can be reviewed to make sure I am heading in the right direction. Please note that I still have to make improvements to my C++ code, so do not review them yet. Just apply them in order to run the reftests.

    
    

    
  


  

        Attachment #651551 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rendering code patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Parsing code patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Reftests (obsolete)
        — 
          — Splinter Review
      

    


  
When you apply my other patches before you run these tests, apply them in this order: CSS Atoms Patch, Rendering Patch, Parsing Patch, Reftest Patch.

Please let me know if I am heading in the right direction with the reftests, including test coverage, style, and even the naming of the html test files.

        Attachment #657745 -
        Flags: feedback?(fred.wang)

    
    

    
  
I'm no longer able to see the behavior mentioned in comment 17 (tested with Firefox 14 and 15). Quentin, do you still see it?

    
    

    
  
OK, the issue still seems to happen randomly when you first open attachment 633989 [details] in a new tab.

    
    

    
  
I just updated the MathJax's issue260 branch and the bug seems to always happen again.

    
    

    
  
Comment on attachment 657745 [details] [diff] [review]
Reftests

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

These tests all look good. It seems that you haven't included the following pages in your patch:

mtable-rowlines-single-mtable-dynamic.html mtable-rowlines-single-ref.html
mtable-rowlines-multi-mtable-dynamic.html
mtable-columnlines-single-mtable-dynamic.html
mtable-columnlines-multi-mtable-dynamic.html
mtable-columnalign-single-mtr-dynamic.html
mtable-columnalign-single-mtable-dynamic.html
mtable-columnalign-multi-mtr-dynamic.html
mtable-columnalign-multi-mtable-dynamic.html

I haven't tried to execute your tests, can you tell us which dynamic tests fail?

- "single" tests verify the result when we specify less than the number of columns/rows (1 < 3)
- "multi" tests verify the result when we specify exactly the number of columns/rows (3 == 3)
- maybe you can write tests when we specify more than the number of columns/rows?

        Attachment #657745 -
        Flags: feedback?(fred.wang) → feedback+

    
    

    
  
@Frédéric

Thanks for the review. I told you over email that the dynamic tests were failing, but I made a code change that fixed the issue. All of the tests in the patch I posted pass.

    
    

    
  
Comment on attachment 657742 [details] [diff] [review]
CSS Atoms and mathml.css code removal

I would like a review on this patch. It is a very simple one, and it removes code that is no longer necessary.

        Attachment #657742 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 657742 [details] [diff] [review]
CSS Atoms and mathml.css code removal

Being able to remove these sounds good but just giving f+ at this because this patch will have to land with other patches, when they are ready.

        Attachment #657742 -
        Flags: review?(karlt) → feedback+

    
    

    
  
I meant "... at this *stage* because ..."

    
    

    
  
From comment #33
> ::: layout/mathml/nsMathMLmtableFrame.cpp
> 
> @@ +1217,5 @@
> > +  }
> > +
> > +  styleText->mTextAlign = alignment;
> > +  return const_cast<nsStyleText*>(styleText);
> > +}
> 
> Yes, as Boris said, this does not look quite right as you are modifying the
> nsStyleText. I think for the MathML case you'll need to allocate a new
> nsStyleText with the modified alignment property in nsLineLayout's
> constructor and release it in nsLineLayout's destructor.

I am trying to develop the code to follow this suggestion, and prevent deconsting of a const nsStyleText to modify it directly. There needs to be some sort of flag that nsLineLayout uses to know whether or not the nsStyleText object mStyleText points to should actually be freed from memory. 

For example, nsMathMLmtdInnerFrame::InitializeStyleTextInLine() needs to create a modified nsStyleText based on the original, so it has to allocate a copy and modify that, returning its pointer to the caller. In that case, the copy can be freed from memory by the nsLineLayout destructor. On the other hand, some implementations of InitializeStyleTextInLine() might return a style text object that should not be freed from memory by nsLineLayout (for example, if you simply return the returning value of GetStyleText(), like nsBlockFrame::InitializeStyleTextInLine() does).

Here is my proposed solution to the problem. Someone please let me know if this will be a good solution:

1) Add a new private member to nsLineLayout named mCanFreeStyleText;

2) Add an output call-by-reference parameter to InitializeStyleTextInLine() called aCanFree

3) In the nsLineLayout constructor, when InitializeStyleTextInLine() is called, pass mCanFreeStyleText as an argument. It will be the responsibility of the method to properly set the mCanFreeStyleText flag via the call-by-reference parameter aCanFree.

4) In the nsLineLayout destructor, check if mCanFreeStyleText is true. If it is, call the destructor on the object mStyleText is is pointing to. Otherwise, just ignore it.

    
    

    
  
(In reply to Quentin Headen from comment #50)
> From comment #33
> > ::: layout/mathml/nsMathMLmtableFrame.cpp
> > 
> > @@ +1217,5 @@
> > > +  }
> > > +
> > > +  styleText->mTextAlign = alignment;
> > > +  return const_cast<nsStyleText*>(styleText);
> > > +}
> > 
> > Yes, as Boris said, this does not look quite right as you are modifying the
> > nsStyleText. I think for the MathML case you'll need to allocate a new
> > nsStyleText with the modified alignment property in nsLineLayout's
> > constructor and release it in nsLineLayout's destructor.
> 
> I am trying to develop the code to follow this suggestion, and prevent
> deconsting of a const nsStyleText to modify it directly. There needs to be
> some sort of flag that nsLineLayout uses to know whether or not the
> nsStyleText object mStyleText points to should actually be freed from
> memory. 

It sounds that your suggestion will work. Alternatively, you can create a a symmetric DeinitializeStyleTextInLine function that would be called nsLineLayout's destructor. This function will free the mStyleText pointer, only in the nsMathMLmtdInnerFrame case.

    
    

    
  
(In reply to Frédéric Wang (:fredw) from comment #51)
> (In reply to Quentin Headen from comment #50)
> > From comment #33
> > > ::: layout/mathml/nsMathMLmtableFrame.cpp
> > > 
> > > @@ +1217,5 @@
> > > > +  }
> > > > +
> > > > +  styleText->mTextAlign = alignment;
> > > > +  return const_cast<nsStyleText*>(styleText);
> > > > +}
> > > 
> > > Yes, as Boris said, this does not look quite right as you are modifying the
> > > nsStyleText. I think for the MathML case you'll need to allocate a new
> > > nsStyleText with the modified alignment property in nsLineLayout's
> > > constructor and release it in nsLineLayout's destructor.
> > 
> > I am trying to develop the code to follow this suggestion, and prevent
> > deconsting of a const nsStyleText to modify it directly. There needs to be
> > some sort of flag that nsLineLayout uses to know whether or not the
> > nsStyleText object mStyleText points to should actually be freed from
> > memory. 
> 
> It sounds that your suggestion will work. Alternatively, you can create a a
> symmetric DeinitializeStyleTextInLine function that would be called
> nsLineLayout's destructor. This function will free the mStyleText pointer,
> only in the nsMathMLmtdInnerFrame case.

I suppose you are right. That is a cleaner solution, and it allows versatility in case the nsStyleText object needs to be freed in a special way for a specific class.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rendering code patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
Here is a new version of the rendering code patch that removes the use of the const_cast in nsMathMLmtdInnerFrame::InitializeStyleTextInLine()

@Boris
I know you said that the const_cast should not be used, and I should not directly modify the nsStyleText gotten from the GetStyleText() method, which I agree with. Can you please give feedback on this new patch version and let me know if this solution will be accetable?

Basically, I am creating a copy of the nsStyleText object returned from GetStyleText(), and I am storing the copy on the nsMathMLmtdInnerFrame object. I then can manipulate and return the copy for custom styling.

@Frédéric
I already emailed you about my solution, but feel free to look at the actual patch and let me know if you spot any issues.

        Attachment #657743 -
        Attachment is obsolete: true

        Attachment #660204 -
        Flags: feedback?(bzbarsky)

    
    

    
  
Comment on attachment 660204 [details] [diff] [review]
Rendering code patch v2

The general approach seems ok.  I'm not sure about the naming of the "initialize" and "deinitialize" methods, nor why we have a deinitialize method at all.  Also, you need to reset mUniqueStyletext when your style context changes.  Modulo those nits, the style bits look good.

        Attachment #660204 -
        Flags: feedback?(bzbarsky) → feedback+

    
    

    
  
@Boris

Thanks for the feedback. I made a mistake leaving the deinitialize method in my patch, as it isn't needed with my new solution. Also, I will ask Karl and Frédéric about the naming of the initialize method.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Reftests v2 (obsolete)
        — 
          — Splinter Review
      

    


  
Here is my new reftest patch. This patch adds new dynamic tests for different attributes on the table.

        Attachment #657745 -
        Attachment is obsolete: true

        Attachment #661133 -
        Flags: feedback?(fred.wang)

    
    

    
  
Comment on attachment 661133 [details] [diff] [review]
Reftests v2

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

Note that the <mstyle> elements are not necessary here, but no need to worry about that. These tests all look good, so I set review+ as I don't think you will need to update them later. More reftests would be better handled in a separate patch. Here are some ideas:

1) Test the attributes against CSS properties set on the <mtd> cells. For example use "text-align" and "vertical-align" for rowalign and columnalign. Maybe you can try something with "border" to test the rowlines/columnlines.

2) Test what happens for multi attributes when we don't specify enough values or when we specify too much values.

3) Test other dynamic operations: child creation/removal, attribute removal, attribute value update. Is the bug found by MathJax in the testcase  detected by your the reftests (they all pass when I tested it)? See what I say in comment 36, maybe you can reproduce MathJax's behavior to exhibit the bug (i.e. try to create _moz- attributes by hand via Javascript and see if that confuses Gecko)

For 2) and 3), you don't necessarily need to write tests for all the combination of attribute names and positions.

        Attachment #661133 -
        Flags: review+

        Attachment #661133 -
        Flags: feedback?(fred.wang)

        Attachment #661133 -
        Flags: feedback+

    
    

    
  
(In reply to Quentin Headen from comment #55)
> @Boris
> 
> Thanks for the feedback. I made a mistake leaving the deinitialize method in
> my patch, as it isn't needed with my new solution. Also, I will ask Karl and
> Frédéric about the naming of the initialize method.

What about GetStyleTextForLineLayout?

BTW, I don't know if someone already mention it to you, but I suggest you had a look at https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

    
    

    
  
@Frédéric 

(From comment #33)
> @@ +189,5 @@
> > + nsTArray<PRInt8>* lineList = ExtractStyleValues(aString,
> > +                                ParseLinesItem);
> > +  return lineList;
> > +}
> > +
> 
> Instead of all these functions, I'm wondering if you could just modify
> ExtractStyleValues to use an enum as the second parameter and use a switch
> on this enum to set the styleValue.

I was thinking about this, and it would seem better to keep the parsing functions instead of using a switch. The main reason for this is that the parsing code can be reusable. You never know what other code will added to nsMathMLmtableFrame.cpp that might require parsing of style values. If we leave that code in their respective functions, it can be reused without having to be rewritten.

Do you agree with my reasoning?

    
    

    
  
(In reply to Quentin Headen from comment #59)
> I was thinking about this, and it would seem better to keep the parsing
> functions instead of using a switch. The main reason for this is that the
> parsing code can be reusable. You never know what other code will added to
> nsMathMLmtableFrame.cpp that might require parsing of style values. If we
> leave that code in their respective functions, it can be reused without
> having to be rewritten.
> 
> Do you agree with my reasoning?

Well, I don't think that this is really necessary for now (if later it turns out that you really need to parse only a single value, then you could as well group the Parse*Item functions in one ParseStyleItem function with an enum as a second argument, do the switch in that function and make ExtractStyleValues call this new function).

    
    

    
  
(In reply to Frédéric Wang (:fredw) from comment #60)
> Well, I don't think that this is really necessary for now (if later it turns
> out that you really need to parse only a single value, then you could as
> well group the Parse*Item functions in one ParseStyleItem function with an
> enum as a second argument, do the switch in that function and make
> ExtractStyleValues call this new function).

I guess you are right. I will convert it to using a switch statement instead.

(From comment #33)
> @@ +245,5 @@
> > +    FrameProperties props = currentFrame->Properties();
> > +    propertyData = props.Get(aFrameProperty);
> > +    bool frameIsTable = 
> > +         (currentFrame->GetStyleDisplay()->mDisplay == NS_STYLE_DISPLAY_TABLE);
> > +
> 
> That does not seem a good idea to rely on the display property. One could
> easily set / unset this property with CSS to any element. Maybe relying on
> the frame tag is better, although I'm not sure that would distinguish inner
> and outer frame tables so a bit more may be needed. That said, there are
> DEBUG_VERIFY_THAT_FRAME_IS_* controlling that the CSS property is correctly
> set, so maybe that's ok...

So what do you plan on doing here? Like you said, DEBUG_VERIFY_THAT_FRAME_IS_* macros do control the CSS property. How hard is it to distinguish an inner and outer table frame? Would do_QueryFrame() do the trick?

Also, so that you know, I should be coming out with new versions of my patches soon. First, I have to implement code to check any existing set styles in nsDisplaymtdBorder::Paint() as you suggested in comment #33. After that, I just have to wrap my lines to 80 characters, and remove any trailing whitespace, then I can release my patches for you guys to take a look at.

    
    

    
  
> (From comment #33)
> > @@ +245,5 @@
> > > +    FrameProperties props = currentFrame->Properties();
> > > +    propertyData = props.Get(aFrameProperty);
> > > +    bool frameIsTable = 
> > > +         (currentFrame->GetStyleDisplay()->mDisplay == NS_STYLE_DISPLAY_TABLE);
> > > +
> > 
> > That does not seem a good idea to rely on the display property. One could
> > easily set / unset this property with CSS to any element. Maybe relying on
> > the frame tag is better, although I'm not sure that would distinguish inner
> > and outer frame tables so a bit more may be needed. That said, there are
> > DEBUG_VERIFY_THAT_FRAME_IS_* controlling that the CSS property is correctly
> > set, so maybe that's ok...
> 
> So what do you plan on doing here? Like you said,
> DEBUG_VERIFY_THAT_FRAME_IS_* macros do control the CSS property. How hard is
> it to distinguish an inner and outer table frame? Would do_QueryFrame() do
> the trick?

I think currentFrame->GetType() == nsGkAtoms::tableFrame would work better (or perhaps nsGkAtoms::tableOuterFrame).

    
    

    
  
> So were you saying that web developers won't be able to use the border-width property on the mtable cells anymore? Also, I am supposed to check for an existing style on only the cells in the first column or row, or do I need to perform the check on all of the cells in the table?

I just looked at layout/style/nsStyleStruct.cpp and

  nsStyleBorder::nsStyleBorder(nsPresContext* aPresContext)

seems to initialize the border style with default values:

  mBorder.Side(side) = medium;
  mBorderStyle[side] = NS_STYLE_BORDER_STYLE_NONE | BORDER_COLOR_FOREGROUND

so that will be the case in your nsDisplaymtdBorder::Paint when you use

nsStyleBorder styleBorder(mFrame->PresContext());

If you want to use the current cell's border, I guess you should do

nsStyleBorder styleBorder(mFrame->GetStyleBorder());

instead. Then the users will be able to use their own style like

<mtable><mtr><mtd style="border: red 5px solid;"><mi>x</mi></mtd></mtr></mtable>

Now, from what I read in nsDisplaymtdBorder::Paint I suspect something like

<mtable columnlines="none" rowlines="dashed"><mtr><mtd style="border: red 5px solid;"><mi>x</mi></mtd></mtr></mtable>

will change the width & style of the top/left borders of the cells: they will no longer be 5px solid, although they will still be red. Perhaps we would like user's specification to take precedence over the columnlines/rowlines, but I'm not sure how to know whether the style was explicitly specified by the user and I don't think that's a serious issue.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Parsing code patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #657744 -
        Attachment is obsolete: true

        Attachment #663937 -
        Flags: review?(karlt)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rendering code patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #660204 -
        Attachment is obsolete: true

        Attachment #663939 -
        Flags: review?(karlt)

    
  

        Attachment #657742 -
        Flags: feedback+ → review?(karlt)

    
    

    
  
I have released new versions of my parsing and rendering patches. From what I see, these patches are feature complete, and have been cleaned up style-wise. I sent all of my patches to the try server, and everything looks green: https://tbpl.mozilla.org/?tree=Try&rev=f88f5cc5bec6

So please review these patches, and let me know if they are ready to be pushed to mozilla-central, or if I have to make any extra changes.

Thanks

    
  

        Attachment #663939 -
        Attachment description: Rendering code patch v2 → Rendering code patch v3

    
    

    
  
(In reply to Quentin Headen from comment #64)
> Created attachment 663937 [details] [diff] [review]
> Parsing code patch v2

This patch has bit-rotted.  Please update.

    
    

    
  
@ Bill

Hmm. I updated my code tree, and the patch applied just fine. Try to apply the patches in this order: css_atoms -> rendering -> parsing -> tests.

I think the parsing patch depends on changes from the rendering patch to apply successfully.

    
    

    
  
OK, so giving me the correct order worked for me.  Builds are in progress and will sow up at http://www.wgis.com/ at some point later tonight.

    
    

    
  
I'm not trying to rush anything, but I just wanted to know the progress on the reviews for my patches.

Thanks.

    
    

    
  
I started looking at this.  Feels good so far.  Hopefully I can get back to you next week.

    
    

    
  
Comment on attachment 663937 [details] [diff] [review]
Parsing code patch v2

>+static nsTArray<int8_t>* ExtractStyleValues(const nsString aString,

Use a reference to avoid copying the string.
Prefer the abstract base class nsAString for parameters unless there is a need
for the function to know more about the storage of the string.
i.e. "const nsAString&".

>+    if (!skippingSpaces && nsCRT::IsAsciiSpace(*start) || *start == *end) {

Use parentheses around the boolean operator to be applies first,
because most/many people do not know that && is applied before ||.

Do you mean "start == end"?

The start == end test needs to be before the nsCRT::IsAsciiSpace(*start) so as
to avoid reading past the end of the strin.

It should be possible to rearrange code so that a maximum of one
nsCRT::IsAsciiSpace(*start) call is required per iteration.
Might be easiest to make the first block
"(start != end) && !nsCRT::IsAsciiSpace(*start)".
Then I think only skippingSpaces would need to be checked on the "else".

>+      int8_t styleValue = -1;

>+      if (styleValue >= 0)
>+        styleArray->AppendElement(styleValue);

Is the >= 0 check necessary here?
Won't that always be true?
If so, then the -1 initialization of styleValue is also unnecessary.

>+      startIndex = startIndex + (count + 1);

startIndex += count + 1

>-    nsAutoString values;
>-    aTableOrRowFrame->GetContent()->GetAttr(kNameSpaceID_None, aAttribute, values);
>-    if (!values.IsEmpty())
>-      valueList = new nsValueList(values);
>-    if (!valueList || !valueList->mArray.Length()) {
>-      delete valueList; // ok either way, delete is null safe
>-      return nullptr;

>  * A variant of the nsDisplayBorder contains special code to render a border
>  * around a nsMathMLmtdFrame based on the rowline and columnline properties
>  * set on the cell frame.
>  */

Something is odd about this patch because that function didn't end and that
comment didn't start.

> // Each rowalign='top bottom' or columnalign='left right center' (from
>-// <mtable> or <mtr>) is split once (lazily) into a nsValueList which is
>+// <mtable> or <mtr>) is split once (lazily) into an nsTArray<int8_t> which is
> // stored in the property table. Row/Cell frames query the property table
> // to see what values apply to them.

Is this still done lazily?
I assume not, or FindCellProperty wouldn't work.

>+/* This method looks for a property that applies to a cell, but it looks
>+ * recursively because some cell properties can come from the cell, a row,
>+ * a table, etc. This function searches through the heirarchy for a property
>+ * and returns its value. The function stops searching after checking a <mtable>
>+ * frame.

I wonder how this function can be used because no distinction is made
between different element tags, and so it is not clear whether the result
should be expected to contain a single value or a list.  Should this
function be picking the particular alignment type that applies to this cell?
Or perhaps it may be easier to add a flag parameter to ExtractStyleValues to
indicate whether multiple values are allowed?

Is it possible, with bad mark up to have an mtr or mtd frame without an
ancestor table frame?  If so, it may be better to recurse only while the
frame corresponds to mtd, mtr, or mlabeledtr.

>+  while(currentFrame) {

while (

> MapRowAttributesIntoCSS(nsIFrame* aTableFrame,

I guess this function should be renamed now.

>+  if (!tableAttr.IsEmpty()) {
>+    // If we have a single-valued attribute.

I don't understand this comment.

>+    nsTArray<int8_t>* alignmentList = ExtractStyleValues(tableAttr,
>+                                        PARSING_TYPE_ROWALIGN);

The alignment of function call arguments is odd here.
Break the line after the '=' and then the ExtractStyleValues arguments can be
on one line.

Doesn't this result in properties being set on aTableFrame again and again for
each row?

>+  if (rowIndex > 0) {
>+    tableContent->GetAttr(kNameSpaceID_None, nsGkAtoms::rowlines_, tableAttr);
>+
>+    if (!tableAttr.IsEmpty()) {
>+      nsTArray<int8_t>* lineList = ExtractStyleValues(tableAttr,
>+                                    PARSING_TYPE_ROWLINES);
>+
>+      if (lineList) {
>+        FrameProperties props = aRowFrame->Properties();
>+        props.Set(RowLinesProperty(), lineList);
>+      }

Why are the properties set on each row instead of on the table frame?

> // map attributes that depend on the index of the column:
> // columnalign, columnlines, XXX need columnwidth and columnspacing too
> static void
> MapColAttributesIntoCSS(nsIFrame* aTableFrame,

The comment and function name don't seem so appropriate now.

Similar issues in this function as for row attributes.

It seems, instead of separate functions for row- and col-index-dependent
attributes, we should now have separate functions to parse attributes on each
of mtable mtr and mtd?  That looks like it would fit in better with the
AttributeChanged methods.

>   // Explicitly request a re-resolve and reflow in our subtree to pick up any changes
>   presContext->PresShell()->FrameConstructor()->
>     PostRestyleEvent(mContent->AsElement(), eRestyle_Subtree,
>                      nsChangeHint_AllReflowHints);

I wonder whether presContext->PresShell()->FrameNeedsReflow() would be
sufficient now.

Should nsMathMLmtdFrame::AttributeChanged() remove frame properties when the
relevant attributes are removed?

>-  // we only have to remove the leading spaces because 
>+  // we only have to remove the leading spaces because

>       aAttribute == nsGkAtoms::columnspan_) {
>-    // use the naming expected by the base class 
>+    // use the naming expected by the base class
>     if (aAttribute == nsGkAtoms::columnspan_)
>       aAttribute = nsGkAtoms::colspan;
>-    return nsTableCellFrame::AttributeChanged(aNameSpaceID, aAttribute, aModType);
>+    return nsTableCellFrame::AttributeChanged(aNameSpaceID, aAttribute,
>+                                              aModType);
>   }

Although the newer style is preferable here, just leave old code as it is, if
you are not changing adjacent lines.
That makes it easier to look though history to work out what changed when and
why.

        Attachment #663937 -
        Flags: review?(karlt) → review-

    
    

    
  
I don;t like changing other peoples summaries.

However,I think it would more correctly describe what is being done here if the word "prevent" in the summary were replaced with the word "avoid".

    
  
Summary: Rewrite mtable implementation to prevent use of _moz-* attributes → Rewrite mtable implementation to avoid use of _moz-* attributes

    
    

    
  
Increasing priority of this bug, because it is blocking implementation of @rowspacing/columnspacing attributes, which are used a lot by MathJax.
Priority: -- → P3
Whiteboard: [mentor=fredw][lang=c++]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rendering Code Patch v4 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #663939 -
        Attachment is obsolete: true

        Attachment #663939 -
        Flags: review?(karlt)

        Attachment #695548 -
        Flags: feedback?(karlt)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Parsing Code Patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
This parsing code patch, and the rendering code patch uploaded along with it fixes many of the issues that Karl spoke about in his last feedback. The biggest change in this patch is the complete removal of the MapRowAttributesIntoCSS() and MapColAttributesIntoCSS() functions.

        Attachment #663937 -
        Attachment is obsolete: true

        Attachment #695549 -
        Flags: feedback?(karlt)

    
    

    
  
Comment on attachment 695549 [details] [diff] [review]
Parsing Code Patch v3

The change from MapRowAttributesIntoCSS and MapColAttributesIntoCSS into
ParseFrameAttribute() is looking good.

>+  while (start <= end) {
>+    // Check if start is pointing to a space character.
>+    onSpace = nsCRT::IsAsciiSpace(*start);

>+    if (start == end || (!skippingSpaces && onSpace)) {

Still need a start != end test before nsCRT::IsAsciiSpace(*start),
so as not to read past the end of the string.

Might be easiest to make the first block
"(start != end) && !nsCRT::IsAsciiSpace(*start)".
Then I think only skippingSpaces would need to be checked on the "else".

>+      int8_t styleValue = 0;

No need to initialize here.  It is makes the code easier to follow if it is
left uninitialized as it indicates that it will always be set below.

>+AttributeToParsingType(nsIAtom* aAttribute)

>+ParsingTypeToAttribute(ParsingType aParsingType)

Probably simpler to remove ParsingType and just pass around nsIAtom*
aAttribute instead.

> >+/* This method looks for a property that applies to a cell, but it looks
> >+ * recursively because some cell properties can come from the cell, a row,
> >+ * a table, etc. This function searches through the heirarchy for a property
> >+ * and returns its value. The function stops searching after checking a <mtable>
> >+ * frame.

Is it possible, with bad mark up, to have an mtr or mtd frame without an
ancestor table frame, or is a table frame always inserted in frame
constructionn?  If there may not be a table frame, then it would be better to
recurse only while the frame corresponds to mtd, mtr, or mlabeledtr.

>+ParseFrameAttribute(nsIFrame* aFrame, IFrameType aFrameType,
>+                    ParsingType aParsingType)

aFrameType is not used in this function.

>+  if (!attrValue.IsEmpty()) {
>+    nsTArray<int8_t>* valueList =
>+      ExtractStyleValues(attrValue, aParsingType);
> 

>+    FrameProperties props = aFrame->Properties();
>+    props.Set(AttributeToProperty(attrAtom), valueList);
>   }

What should happen if the attribute is empty?
Should any existing property be removed?

Looks like AttributeChanged() will delete the property on mtable and mtr, but
what about mtd and RestyleTable()?

>+      ParseFrameAttribute(rowFrame, IFRAME_TYPE_MTR,
>+                          PARSING_TYPE_ROWLINES);

>+      ParseFrameAttribute(rowFrame, IFRAME_TYPE_MTR,
>+                          PARSING_TYPE_COLUMNLINES);

>+          ParseFrameAttribute(cellFrame, IFRAME_TYPE_MTD,
>+                              PARSING_TYPE_ROWLINES);

>+          ParseFrameAttribute(cellFrame, IFRAME_TYPE_MTD,
>+                              PARSING_TYPE_COLUMNLINES);

AIUI rowlines and columnlines are only appropriate on mtable elements.

>+  nsIAtom* rowAtom = nullptr;
>+  nsIAtom* colAtom = nullptr;
> 

>+  if (aAttribute == nsGkAtoms::rowalign_ ||
>+      aAttribute == nsGkAtoms::rowlines_)
>+    rowAtom = aAttribute;
>+  else if (aAttribute == nsGkAtoms::columnalign_ ||
>+           aAttribute == nsGkAtoms::columnlines_)
>+    colAtom = aAttribute;
>+
>+  if (!rowAtom && !colAtom)
>     return NS_OK;
> 
>   nsPresContext* presContext = tableFrame->PresContext();

>+  // clear any cached property list for this table
>   presContext->PropertyTable()->
>     Delete(tableFrame, AttributeToProperty(aAttribute));
> 

>+  // Get which attribute was actually modified.
>+  nsIAtom* modifiedAttr = (rowAtom != nullptr) ? rowAtom : colAtom;

rowAtom and colAtom are no longer required.

>   // Explicitly request a re-resolve and reflow in our subtree to pick up any changes
>-  presContext->PresShell()->FrameConstructor()->
>-    PostRestyleEvent(mContent->AsElement(), eRestyle_Subtree,
>-                     nsChangeHint_AllReflowHints);
>+  PresContext()->PresShell()->
>+      FrameNeedsReflow(this, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY);

Remove "a re-resolve and" from the comment, now that a re-resolve is not
requested.

I'm still not clear how FindCellProperty can be used because no distinction is
made between different element tags, and so it is not clear whether the result
should be expected to contain a single value or a list.  Should this function
be picking the particular alignment type that applies to this cell?  Or
perhaps it may be easier to add a flag parameter to ExtractStyleValues to
indicate whether multiple values are allowed?  Somewhere there should be some
verification that columnalign on mtd, and rowalign on mtd and mtr, contain
only one value.

        Attachment #695549 -
        Flags: feedback?(karlt) → feedback+

    
    

    
  
(In reply to Karl Tomlinson (:karlt back Jan 28) from comment #78)

> > >+/* This method looks for a property that applies to a cell, but it looks
> > >+ * recursively because some cell properties can come from the cell, a row,
> > >+ * a table, etc. This function searches through the heirarchy for a property
> > >+ * and returns its value. The function stops searching after checking a <mtable>
> > >+ * frame.
> 
> Is it possible, with bad mark up, to have an mtr or mtd frame without an
> ancestor table frame, or is a table frame always inserted in frame
> constructionn?  If there may not be a table frame, then it would be better to
> recurse only while the frame corresponds to mtd, mtr, or mlabeledtr.

To test this, I created a simple HTML page that contained some mtr and mtd elements, but no surrounding <mtable> element. Once the page loaded, the parsing code never even fired, and the table never showed on the screen. The table doesn't show without an <mtable> tag in the current release version of Firefox either. So it seems that a table frame is required to be there, and we can reliably look for one in FindCellProperty().

> I'm still not clear how FindCellProperty can be used because no distinction
> is
> made between different element tags, and so it is not clear whether the
> result
> should be expected to contain a single value or a list.  Should this function
> be picking the particular alignment type that applies to this cell?  Or
> perhaps it may be easier to add a flag parameter to ExtractStyleValues to
> indicate whether multiple values are allowed?  Somewhere there should be some
> verification that columnalign on mtd, and rowalign on mtd and mtr, contain
> only one value.

This is the reason I implemented the column and row align attribute properties as lists no matter how many elements there are. If we use lists for align properties no matter how many values are given, it will cut away all of the code needed to check whether or not we are dealing with a single-valued variable, or a multi-valued list. Basically, FindCellProperty() isn't concerned with how a property is interpreted and handled, but rather, whether or not it is found on the current cell, or anywhere in the current table hierarchy. This is the reason for a void* return value, because it is the caller's job to interpret the resulting pointer. Please correct me if my approach to this this problem is wrong.

    
    

    
  
Frame construction never allows table internal frames to leak into the frame tree without wrapping them in a table, for what it's worth.

    
    

    
  
(In reply to Karl Tomlinson (:karlt back Jan 28) from comment #78)
> What should happen if the attribute is empty?
> Should any existing property be removed?
> 
> Looks like AttributeChanged() will delete the property on mtable and mtr, but
> what about mtd and RestyleTable()?

An empty attribute is invalid, so I guess we could just ignore it. This probably means removing any existing properties on the current frame, so that the rendering code can use the properties on ancestor frames.

@Quentin: here is an example to test what Karl means. From the test case

<mtable rowalign="top">
<mtr rowalign="center">
  <mtd rowalign="bottom">
    <mspace width="10px" depth="5px" height="5px" mathbackground="red"></mspace>
  </mtd>
  <mtd rowalign="bottom">
    <mspace width="10px" depth="10px" height="10px" mathbackground="green"></mspace>
  </mtd>
  <mtd rowalign="bottom">
    <mspace width="10px" depth="15px" height="15px" mathbackground="blue"></mspace>
  </mtd>
</mtr>
</mtable>

use Javascript to set the rowalign attributes on the <mtd> elements to the empty string "". After that change, the bars should be aligned "center" as specified on the <mtr> element. Then use Javascript again to set the rowalign on the <mtr> element to "". After that change, the bars should be aligned "top" as specified on the <mtable> element. Then use Javascript again to set the rowalign on the <mtable> element to "". Now the bars should use the default "axis" alignment. Similarly for other attributes.

> I'm still not clear how FindCellProperty can be used because no distinction
> is
> made between different element tags, and so it is not clear whether the
> result
> should be expected to contain a single value or a list.  Should this function
> be picking the particular alignment type that applies to this cell?  Or
> perhaps it may be easier to add a flag parameter to ExtractStyleValues to
> indicate whether multiple values are allowed?  Somewhere there should be some
> verification that columnalign on mtd, and rowalign on mtd and mtr, contain
> only one value.

I think this issue is addressed by Quentin's rendering code patch. Per the spec, if N is the number of rows (or columns) and L the length of the list, then the property used for the cell at index 1 <= I <= N is the min(I, L)'th element of the list. That is if L > N we ignore the excess and if L < N we repeat the last value. In particular, when a single value is returned, Quentin actually uses a list of one element. So the property used for on the cell will be the unique element of that list, as wanted.

    
    

    
  
(In reply to Karl Tomlinson (:karlt back Jan 28) from comment #78)

> The change from MapRowAttributesIntoCSS and MapColAttributesIntoCSS into
> ParseFrameAttribute() is looking good.
> 
> >+  while (start <= end) {
> >+    // Check if start is pointing to a space character.
> >+    onSpace = nsCRT::IsAsciiSpace(*start);
> 
> >+    if (start == end || (!skippingSpaces && onSpace)) {
> 
> Still need a start != end test before nsCRT::IsAsciiSpace(*start),
> so as not to read past the end of the string.
> 
> Might be easiest to make the first block
> "(start != end) && !nsCRT::IsAsciiSpace(*start)".
> Then I think only skippingSpaces would need to be checked on the "else".

I'm a little bit confused as to how this while loop will read past the end of a string. I'm going to assume that "end" points to the memory location of the last character in the string. Although the it is possible that the "start" pointer will be incremented by one past the end of the string, because the while loop exits when this happens, the start pointer is never dereferenced when it is pointing past the end. So this avoids any out of bound errors.

Also, making the logic of the first if statement "if ((start != end) && !nsCRT::IsAsciiSpace(*start))" wouldn't really work either. For example, take the string "top bottom center". On the very first character, start != end evaluates to true. Then !nsCRT::IsAsciiSpace(*start) also evaluates to true, since we are on a letter not a space. That would cause a substring to be created too early.

    
    

    
  
> I'm going to assume that "end" points to the memory location of the last character in the
> string.

It points to the memory location after the last character, in typical C/C++ string terminator fashion.  So dereferencing end is not ok.  So when start == end you can't dereference start.

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #83)
> > I'm going to assume that "end" points to the memory location of the last character in the
> > string.
> 
> It points to the memory location after the last character, in typical C/C++
> string terminator fashion.  So dereferencing end is not ok.  So when start
> == end you can't dereference start.

Ah okay. Thanks for the clarification.

    
    

    
  
(In reply to Quentin Headen from comment #82)
> (In reply to Karl Tomlinson (:karlt back Jan 28) from comment #78)
> > Might be easiest to make the first block
> > "(start != end) && !nsCRT::IsAsciiSpace(*start)".
> > Then I think only skippingSpaces would need to be checked on the "else".

> [...] making the logic of the first if statement "if ((start != end) &&
> !nsCRT::IsAsciiSpace(*start))" wouldn't really work either. For example,
> take the string "top bottom center". On the very first character, start !=
> end evaluates to true. Then !nsCRT::IsAsciiSpace(*start) also evaluates to
> true, since we are on a letter not a space. That would cause a substring to
> be created too early.

The (start != end && !nsCRT::IsAsciiSpace(*start)) block would not finalize the substring, but start/continue tracking it, like the third/final/else block in attachment 695549 [details] [diff] [review].

    
    

    
  
(In reply to Frédéric Wang (:fredw) from comment #81)
> > I'm still not clear how FindCellProperty can be used because no distinction
> > is
> > made between different element tags, and so it is not clear whether the
> > result
> > should be expected to contain a single value or a list.  Should this function
> > be picking the particular alignment type that applies to this cell?  Or
> > perhaps it may be easier to add a flag parameter to ExtractStyleValues to
> > indicate whether multiple values are allowed?  Somewhere there should be some
> > verification that columnalign on mtd, and rowalign on mtd and mtr, contain
> > only one value.
> 
> I think this issue is addressed by Quentin's rendering code patch. Per the
> spec, if N is the number of rows (or columns) and L the length of the list,
> then the property used for the cell at index 1 <= I <= N is the min(I, L)'th
> element of the list. That is if L > N we ignore the excess and if L < N we
> repeat the last value. In particular, when a single value is returned,
> Quentin actually uses a list of one element. So the property used for on the
> cell will be the unique element of that list, as wanted.

That works for valid syntax, but doesn't detect an invalid attribute string that contains multiple values when there should be only one.  I think an invalid attribute string should be ignored, and, if the old code was doing that, and I think it was, then the new code should also do the same.

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #86)
> (In reply to Frédéric Wang (:fredw) from comment #81)
> > > I'm still not clear how FindCellProperty can be used because no distinction
> > > is
> > > made between different element tags, and so it is not clear whether the
> > > result
> > > should be expected to contain a single value or a list.  Should this function
> > > be picking the particular alignment type that applies to this cell?  Or
> > > perhaps it may be easier to add a flag parameter to ExtractStyleValues to
> > > indicate whether multiple values are allowed?  Somewhere there should be some
> > > verification that columnalign on mtd, and rowalign on mtd and mtr, contain
> > > only one value.
> > 
> > I think this issue is addressed by Quentin's rendering code patch. Per the
> > spec, if N is the number of rows (or columns) and L the length of the list,
> > then the property used for the cell at index 1 <= I <= N is the min(I, L)'th
> > element of the list. That is if L > N we ignore the excess and if L < N we
> > repeat the last value. In particular, when a single value is returned,
> > Quentin actually uses a list of one element. So the property used for on the
> > cell will be the unique element of that list, as wanted.
> 
> That works for valid syntax, but doesn't detect an invalid attribute string
> that contains multiple values when there should be only one.  I think an
> invalid attribute string should be ignored, and, if the old code was doing
> that, and I think it was, then the new code should also do the same.

Okay, I'll just simply add a flag parameter to ExtractStyleValues(). But what should be done after the attribute string is ignored? Should a default attribute value be placed on the frame, or should an invalid attribute prevent the setting of any attributes on the frame? I am assuming the latter.

    
    

    
  
Yes, an invalid attribute leading to nothing set sounds good.

    
    

    
  
In particular, when an invalid attribute is used, the previous property on the frame should be removed. See the example for the invalid empty string in comment 81 (you can replace "" by any invalid attribute).

Also, now that we send MathML parsing error to the console (bug 553917), you can use nsMathMLContainerFrame::ReportParseError to indicate a parsing error for an attribute.

    
    

    
  
> Also, now that we send MathML parsing error to the console (bug 553917), you
> can use nsMathMLContainerFrame::ReportParseError to indicate a parsing error
> for an attribute.

What would be a recommended error message for a parsing issue? Should it mention the id of the offending element?

    
    

    
  
(In reply to Quentin Headen from comment #90)
> What would be a recommended error message for a parsing issue? Should it
> mention the id of the offending element?

ReportParseError will report the element, attribute and attribute value, so I think it's fine. See

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmpaddedFrame.cpp#83
http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLContainerFrame.cpp#1534
http://mxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/mathml/mathml.properties

    
    

    
  
I see that ReportParseError() is an instance method on nsMathMLContainerFrame. But the nsMathML*Frame objects such as mtable, mtr, and mtd are not children of nsMathMLContainer frame. So how am I to make use of ReportParseError? Does this method need to be extended to this table frame objects? Should I call nsContentUtils::ReportToConsole() directly?

    
    

    
  
Right, ReportParseError is not too much code, so I guess it's fine if you write a similar function to call ReportToConsole from the mtable module...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rendering Code Path v5 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #695548 -
        Attachment is obsolete: true

        Attachment #695548 -
        Flags: feedback?(karlt)

        Attachment #713403 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 713403 [details] [diff] [review]
Rendering Code Path v5

Requested feedback instead of review.

        Attachment #713403 -
        Flags: review?(karlt) → feedback?(karlt)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Parsing Code Patch v4 (obsolete)
        — 
          — Splinter Review
      

    


  
In the new parsing and rendering code patches, I implemented the given feedback suggestions. After reading Boris' remarks in comment 80, I decided to keep the FindCellProperty() function the same, since we can guarantee a table frame whether it is explicitly or implicitly created.

The major changes in the parsing patch include a complete rewrite of the ExtractStyleValues() parsing loop, and the proper handling and error reporting of attribute lists being applied to single-value-only attributes.

        Attachment #695549 -
        Attachment is obsolete: true

        Attachment #713405 -
        Flags: feedback?(karlt)

    
    

    
  
Comment on attachment 713405 [details] [diff] [review]
Parsing Code Patch v4

>+static int8_t ParseStyleValue(nsIAtom* aAttribute,
>+                              const nsAString& aAttributeValue)

File style is to move function names to the beginning of a new line.

>+  int8_t styleValue;

>+  } else if (aAttribute == nsGkAtoms::rowlines_ ||
>+             aAttribute == nsGkAtoms::columnlines_) {

>+  }

Add an else block here containing MOZ_NOT_REACHED().  For unrecognized
attributes, there isn't a meaningful number we could return, but returning
some random number such as -1 would suppress a gcc compiler warning.  The
styleValue variable is actually no longer needed here as each block can return
NS_STYLE_foo_bar immediately.

There is a change in behavior here when a value (either a single value or one
of multiple values) is not recognized.  The old behavior it seems was to
ignore only the unrecognized value, and so the value used for the
corresponding mtr or mtd was inherited.  The new behavior is to choose a
default value for the corresponding mtr or mtd.  In each case recognized other
values from the same attribute are still used appropriately.  There isn't a
specified behavior for unrecognized attributes, and in some ways what is
specified is a bit odd: the default values for mtr and mtd are inherited and
yet there is no way to specify inheritance of one of multiple values.  So I
think the new behavior is probably fine.

One thing that could and I think should be made more consistent though is the
behavior of a whitespace-only attribute, which currently looks different from
the behavior of an empty attribute.  How about creating the new
nsTArray<int8_t>() for styleArray when a non-whitespace value is found.  That
way the frame property will only be set when there is a non-whitespace value.

>+      nsDependentSubstring valueString = Substring(aString, startIndex, count);

This can be shortened to
nsDependentSubstring valueString(aString, startIndex, count);
(I'm guessing the Substring template functions are convenience functions for
when the substring is just used as a temporary variable, without being
declared explicitly.)

>+static void*
>+FindCellProperty(const nsIFrame* aCellFrame,

This function can cast the return value to nsTArray<int8_t>* and save all the callers having to perform the cast.

>+      ReportParseError(aFrame, aAttribute->GetUTF16String(),
>+                       attrValue.BeginReading());

Use attrValue.get() here.  BeginReading() is suitable for use with
EndReading(), but, for some classes, it may not return a NUL-terminated
string.  get() will only return a NUL-terminated string.

>@@ -531,58 +533,34 @@ nsMathMLmtableOuterFrame::AttributeChang

>+  // clear any cached property list for this table
>   presContext->PropertyTable()->
>     Delete(tableFrame, AttributeToProperty(aAttribute));

>+  // Should we allow multiple attribute values.
>+  bool allowMultiValues = (aAttribute == nsGkAtoms::rowalign_ ||
>+                           aAttribute == nsGkAtoms::rowlines_ ||
>+                           aAttribute == nsGkAtoms::columnalign_ ||
>+                           aAttribute == nsGkAtoms::columnlines_);
>+  // Reparse the new attribute on the table.
>+  ParseFrameAttribute(tableFrame, aAttribute, allowMultiValues);

Return early if the attribute is not one that ParseFrameAttribute() is
expecting.  Then the allowMultiValues will always be true.

>+  // Explicitly request a reflow in our subtree to pick up any changes
>+  PresContext()->PresShell()->

Use the presContext variable initialized above.
Similarly in nsMathMLmtrFrame::AttributeChanged().

>   nsTArray<int8_t>* alignmentList = static_cast<nsTArray<int8_t>*>(
>-                                 FindCellProperty(this, ColumnAlignProperty()));
>+    FindCellProperty(this, ColumnAlignProperty()));

This change seems to be in the wrong patch.

        Attachment #713405 -
        Flags: feedback?(karlt) → feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Parsing Code Patch v5 (obsolete)
        — 
          — Splinter Review
      

    


  
This patch addresses the last set of suggestions given by Karl.

        Attachment #713405 -
        Attachment is obsolete: true

        Attachment #734458 -
        Flags: feedback?(karlt)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rendering Code Patch v6 (obsolete)
        — 
          — Splinter Review
      

    


  
I took one of the changes made in the parsing patch and placed it into the rendering patch since the change was made to the rendering code.

        Attachment #713403 -
        Attachment is obsolete: true

        Attachment #713403 -
        Flags: feedback?(karlt)

        Attachment #734459 -
        Flags: feedback?(karlt)

    
    

    
  
Comment on attachment 734458 [details] [diff] [review]
Parsing Code Patch v5

>+      if (styleArray == nullptr)

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
prefers (!styleArray).

>     nsTArray<int8_t>* rowLinesList =
>-      static_cast<nsTArray<int8_t>*>(
>-      FindCellProperty(mFrame, RowLinesProperty()));
>+      FindCellProperty(mFrame, RowLinesProperty());
> 
>     nsTArray<int8_t>* columnLinesList =
>-      static_cast<nsTArray<int8_t>*>(
>-        FindCellProperty(mFrame, ColumnLinesProperty()));
>+      FindCellProperty(mFrame, ColumnLinesProperty());

These changes are in the wrong patch, but, once these patches are reviewed, they'll need folding together before pushing anyway.

>   // ...and the other attributes affect rows or columns in one way or another

>+  // Only allow attributes that this function is allowed to handle (which is
>+  // specified in the comment at the beginning of the function.)

The first comment is sufficient here.  No need for the additional comment.

        Attachment #734458 -
        Flags: feedback?(karlt) → feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Parsing Code Patch v6 (obsolete)
        — 
          — Splinter Review
      

    


  
Fixed issues in parsing patch outlined in previous comment.

        Attachment #734458 -
        Attachment is obsolete: true

        Attachment #737315 -
        Flags: feedback?(karlt)

        Attachment #737315 -
        Flags: feedback?(karlt) → feedback+

    
    

    
  
Comment on attachment 734459 [details] [diff] [review]
Rendering Code Patch v6

Please add MOZ_OVERRIDE to nsDisplaymtdBorder::Paint().

>+    nsStyleBorder styleBorder(*(mFrame->StyleBorder()));

nsStyleBorder styleBorder = *mFrame->StyleBorder();

is the way this would normally be written in Gecko.
It is easier to read with fewer parentheses.

>+    nscoord borderWidth = (mFrame->PresContext()->GetBorderWidthTable())
>+                            [NS_STYLE_BORDER_WIDTH_THIN];

I think the parentheses here are unnecessary and so should be removed.

>+    nsTArray<int8_t>* rowLinesList =
>+      static_cast<nsTArray<int8_t>*>(
>+      FindCellProperty(mFrame, RowLinesProperty()));

>+    // We don't place a row line on top of the first row
>+    if (rowIndex > 0 && rowLinesList) {
>+      // If the row number is greater than the number of provided rowline
>+      // values, we simply repeat the last value.
>+      int32_t listLength = rowLinesList->Length();
>+      if (rowIndex < listLength) {
>+        styleBorder.SetBorderStyle(NS_SIDE_TOP,
>+                      rowLinesList->ElementAt(rowIndex - 1));
>+      } else {
>+        styleBorder.SetBorderStyle(NS_SIDE_TOP,
>+                      rowLinesList->ElementAt(listLength - 1));
>+      }
>+      styleBorder.SetBorderWidth(NS_SIDE_TOP, borderWidth);
>+    }

Please add an assertion where the properties are set in ParseFrameAttribute
that the length of the array is always non-zero and point out that the
property reading code depends on that.

>+    nsCSSRendering::PaintBorderWithStyleBorder(mFrame->PresContext(), *aCtx,
>+                                               mFrame, mVisibleRect,
>+                                               nsRect(offset,
>+                                               mFrame->GetSize()),
>+                                               styleBorder,

Please indent mFrame->GetSize() to align with |offset| above.

This doesn't deal with visited links now, but I think that's fine.

>+  // Set the default alignment in case nothing was specified
>+  uint8_t alignment = NS_STYLE_TEXT_ALIGN_CENTER;

Is this necessary to ensure that there is a default alignment?
I'm wondering whether we can use the style-resolved value of text-align if no
mathml attributes have been specified?  It looks like we'd currently do that
and I don't see anything being removed that ensured default-center alignment,
so I assume that would still happen.

Similarly for NS_STYLE_VERTICAL_ALIGN_BASELINE?

I suspect the GetContentEmpty() logic in ProcessBorders() can be
simplified a bit now.
aFrame->IsBorderCollapse() || !borderStyle->HasBorder() can be an early return.

>+  mUniqueStyleText = new (PresContext()) nsStyleText(*(StyleText()));

The outer parentheses in *(StyleText()) can be removed.

(In reply to Boris Zbarsky (:bz) from comment #54)
> Also, you need to reset mUniqueStyletext when your style
> context changes.

I don't think this has been addressed.
I don't know whether there is a way to know when the style context changes.

Currenly alignment is only evaluated when an nsLineLayout is constructed, but I wonder whether is possible to change the alignment without reconstructing the nsLineLayout, in which case the alignment change will not be noticed here.

        Attachment #734459 -
        Flags: feedback?(karlt) → feedback+

    
    

    
  
@qheaden: any update on this?
Flags: needinfo?(qheaden)

    
    

    
  
@fredw

Hey Frédéric. Sorry about the delay. Unfortunately, I'm buried in schoolwork right now, and I have a new job at my university. So I'm not sure when I will get the time to work on this bug further. Until I get some more free time, perhaps someone else can fill in the gaps.
Flags: needinfo?(qheaden)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        reftest v3 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #661133 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rendering Code Patch v7 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #734459 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Parsing Code Patch v7 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #737315 -
        Attachment is obsolete: true

    
  

        Attachment #8337940 -
        Flags: feedback?(karlt)

    
  

        Attachment #8337941 -
        Flags: feedback?(karlt)

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #102)

> (In reply to Boris Zbarsky (:bz) from comment #54)
> > Also, you need to reset mUniqueStyletext when your style
> > context changes.
> 
> I don't think this has been addressed.
> I don't know whether there is a way to know when the style context changes.
> 
> Currenly alignment is only evaluated when an nsLineLayout is constructed,
> but I wonder whether is possible to change the alignment without
> reconstructing the nsLineLayout, in which case the alignment change will not
> be noticed here.

Boris, can you please comment on this?
Flags: needinfo?(bzbarsky)

    
    

    
  
> I don't know whether there is a way to know when the style context changes.

Override nsIFrame::DidSetStyleContext?

> but I wonder whether is possible to change the alignment without reconstructing the
> nsLineLayout

I don't know, and I'm not sure why the question even arises, actually.  Any such change wouldn't be picked up by the nsLineLayout, since it's already caching the nsStyleText and style structs are immutable.

But it _is_ possible to change what the nsStyleText for a given frame is over time (due to restyles, say).  And the patch just saves the nsStyleText in the frame's constructor and then never regets it from the style context, which means it would fail to pick up changes to whatever properties are stored in there.
Flags: needinfo?(bzbarsky)

    
  

        Attachment #8337940 -
        Flags: feedback?(karlt)

    
  

        Attachment #8337941 -
        Flags: feedback?(karlt)

    
    

    
  
Comment on attachment 657742 [details] [diff] [review]
CSS Atoms and mathml.css code removal

That one will probably need update later, depending on whether bug 114365 is taken before or after.

        Attachment #657742 -
        Flags: review?(karlt)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rendering Code Patch v8 (obsolete)
        — 
          — Splinter Review
      

    


  
I added a nsMathMLmtdInnerFrame::DidSetStyleContext.

I don't know if that was the case before, but nsStyleText::WhiteSpaceCanWrap (and other functions) has an ASSERTION to ensure that the StyleText of the frame argument == this so that does not work for the MathML cell. I have added something to prevent that assertion in nsLineLayout, but I'm not really sure it is really clean. Any better idea is welcome.

        Attachment #8337940 -
        Attachment is obsolete: true

        Attachment #8338405 -
        Flags: feedback?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Parsing Code Patch v8 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8337941 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8338405 [details] [diff] [review]
Rendering Code Patch v8

> I added a nsMathMLmtdInnerFrame::DidSetStyleContext.

Looks good.

The asserts are there to make sure people don't pass in a totally unrelated frame. I suggest asking whoever added the asserts what the right way to adjust them is....

        Attachment #8338405 -
        Flags: feedback?(bzbarsky) → feedback+

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #113)
> Comment on attachment 8338405 [details] [diff] [review]
> Rendering Code Patch v8
> 
> > I added a nsMathMLmtdInnerFrame::DidSetStyleContext.
> 
> Looks good.
> 
> The asserts are there to make sure people don't pass in a totally unrelated
> frame. I suggest asking whoever added the asserts what the right way to
> adjust them is....

Cameron, any suggestion?
Flags: needinfo?(cam)

    
    

    
  
I recommend expanding out the contents of WhiteSpaceCanWrap into the caller.  So:

  psd->mNoWrap = !mStyleText->WhiteSpaceCanWrapStyle() ||
                 LineContainerFrame()->IsSVGText();

WhiteSpaceCanWrapStyle returns whether the value of white-space indicates that text is allowed to wrap automatically, while WhiteSpaceCanWrap takes into account whether the frame is for SVG text, where text never wrap automatically.
Flags: needinfo?(cam)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 4: cleanup (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2 - tests (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #657742 -
        Attachment is obsolete: true

        Attachment #8337935 -
        Attachment is obsolete: true

        Attachment #8338405 -
        Attachment is obsolete: true

        Attachment #8338407 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 3 - rendering (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 1  - parsing (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8339288 -
        Flags: review?(karlt)

    
  

        Attachment #8339285 -
        Attachment description: Part 1: CSS Atoms and mathml.css code removal → Part 1: cleanup

    
  

        Attachment #8339287 -
        Flags: review?(karlt)

    
    

    
  
The changes are almost independent but let's take that after bug 114365.
Depends on: mathvariant

    
  
Blocks: 838506

    
  

        Attachment #8339285 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 8339288 [details] [diff] [review]
Part 1  - parsing

The rendering patch depends on this one, so please land this first, assuming it compiles on its own.  If this patch doesn't compile on its own, then please fold in the rendering patch.  Perhaps it is best to do that regardless.

>+    PR_NOT_REACHED("Unrecognized attribute.");

NS_NOTREACHED

>+      if (styleArray == nullptr)

!styleArray

>+  // Only allow attributes that this function is allowed to handle (which is
>+  // specified in the comment at the beginning of the function.)

Something like "Ignore attributes that do not affect layout."

        Attachment #8339288 -
        Attachment description: Part 4  - parsing → Part 1  - parsing

        Attachment #8339288 -
        Flags: review?(karlt) → review+

    
    

    
  
Comment on attachment 8339287 [details] [diff] [review]
Part 3 - rendering

>Bug 731667 - Rewrite mtable implementation to avoid use of _moz-* attributes - implement rendering. r=karlt, r=bz

f=bz

        Attachment #8339287 -
        Flags: review?(karlt) → review+

    
    

    
  
Comment on attachment 8339285 [details] [diff] [review]
Part 4: cleanup

I think it makes more sense to remove these when they are no longer used, after the parsing patch, at least.

        Attachment #8339285 -
        Attachment description: Part 1: cleanup → Part 4: cleanup

        Attachment #8339285 -
        Flags: review?(karlt) → review+

    
  

        Attachment #640067 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #8339285 -
        Attachment is obsolete: true

        Attachment #8339286 -
        Attachment is obsolete: true

        Attachment #8339287 -
        Attachment is obsolete: true

        Attachment #8339288 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2 - remove legacy code (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  


  

        Attachment #8347141 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 3 - tests
        — 
          — Splinter Review
      

    


  

    
    

    
  
(In reply to Karl Tomlinson (:karlt back 3 Jan) from comment #121)
> Comment on attachment 8339288 [details] [diff] [review]
> Part 1  - parsing
> 
> The rendering patch depends on this one, so please land this first, assuming
> it compiles on its own.  If this patch doesn't compile on its own, then
> please fold in the rendering patch.  Perhaps it is best to do that
> regardless.
> 
> >+    PR_NOT_REACHED("Unrecognized attribute.");
> 
> NS_NOTREACHED

MOZ_something? :)

    
    

    
  
(In reply to :Ms2ger from comment #128)
> > 
> > NS_NOTREACHED
> 
> MOZ_something? :)

mmh, can't find the MOZ_*REACHED equivalent. Do you have any idea?

    
    

    
  
MOZ_ASSUME_UNREACHABLE? But note the warning in the comment:
https://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#382

    
    

    
  
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #130)
> MOZ_ASSUME_UNREACHABLE? But note the warning in the comment:
> https://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#382

Thanks. So I don't think it's relevant to use it here. So should I convert the NS_NOTREACHED to a 
MOZ_ASSERT, MOZ_CRASH or leave it as it?

    
    

    
  


  
Replacing NS_NOTREACHED by MOZ_CRASH.

        Attachment #8347140 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed
Depends on: 975935

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: