337700 - js changes to MathML are not drawn

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Ingo Planz]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

When manipulating a MathML formula in an XHTML page, the modified parts are not redrawn. Forcing a redraw by covering and uncovering the window leads to blank areas where changes should be. The remaining part of the formula redraws as expected, though. However, changing the font size helps.

On the example URL (sorry, german) you'll find an integral and four buttons to change the boundaries. Clicking triggers some javascript that replaces the relevant parts of the formula through the DOM. These changes do appear in the DOM Inspector, yet  are not shown, as described above.


Reproducible: Always

Steps to Reproduce:
1. go to URL
2. click on any of the buttons
3. cover and uncover the browser window

Actual Results:  
white gaps in the formula

Expected Results:  
fractions resp. numbers

It did work on Firefox 1.0.x.

According to the warning dialog, I have only the "Symbol" missing.

Comment 1

[Jesse Ruderman]

WFM on trunk, Mac OS X.

Btw, awesome use of dynamic MathML and SVG.

Comment 2

[Andrew Schultz]

worksforme with trunk and SeaMonkey 1.0 (Gecko 1.8.0.1).  But SeaMonkey 1.0.1 (Gecko 1.8.0.2) exhibits the bug.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Works for me with Windows 1.8.0 branch builds from 2006-03-21-08, 2006-04-25-05, and 2006-05-12-05.

Comment 4

[Thilo Planz]

I can reproduce the problem with Firefox 1.5.0.3 on Mac OS.
Resizing the window forces the ML to be redrawn correctly.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

Works on Camino 1.0 (Version 2006021400), broken in Camino 1.0.1 (Version 2006042704)

Comment 5

[Andrew Schultz]

This is a regression from bug 309120

Comment 6

[rbs]

Guys, are you sure it is a regression from bug bug 309120? 

From bug 309120 comment 34, that patch was checked in the branch on 2006-02-26, and the WFM period reported here in comment 3 is past that date:

> Works for me with Windows 1.8.0 branch builds from 2006-03-21-08,
> 2006-04-25-05, and 2006-05-12-05.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Maybe I did something wrong when testing those builds, or maybe this isn't a problem on Windows. I'll test again to be sure.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Indeed, I was misinterpreting the instructions. I do see this bug in all of the builds I mentioned above, and in 2006-02-27-06, but not in 2006-02-26-06, which points to the changes from bug 309120 as the cause, as mentioned earlier.

Comment 9

[Daniel Veditz [:dveditz]]

rbs: is there any chance of a fix for this regression in a 1.8.0.x release?

Comment 10

[rbs]

I hope so (or 1.8.1.x). I have been unfortunately unable to investigate so far due to other balls in the air, and I am travelling tomorrow. Hopefully, I will have a chance to dig a bit earlier next month.

Comment 11

[Daniel Veditz [:dveditz]]

Not going to make 1.8.0.5, please nominate for a later 1.8.0.x release if you get it into 1.8.1

Comment 12

[Darin Fisher]

not going to block ff2 for this, but we'll happily consider a safe patch.

Comment 13

[rbs]

Attachment: patch

Curiously, this patch (on the 1.8 branch) makes the bug go away.

Comment 14

[rbs]

reminder: this bug is not present on the trunk.

Comment 15

[rbs]

The current patch in attachment 232369 [details] [diff] [review] does the job. I am investigating what is going on. (If I don't get to understand fully what is going on it by today, I will request r/sr/a on this fallback patch.)

Comment 16

[Daniel Veditz [:dveditz]]

When you get a reviewed patch you're happy with nominate for 1.8.0.x and we'll take a look, but we're not going to block on this.

Comment 17

[rbs]

Attachment: patch

OK, I now understand the problem, and this patch fixes it. I am proposing it for both the trunk and the 1.8.0 and 1.8 branches.

The bug is happening because the bit NS_FRAME_HAS_DIRTY_CHILDREN is set too early [causing ReflowDirtyChild() not to be called]. The sequence is like this:

1- Content is changed, this causes new frames to be created, and we eventually get to nsMathMLContainerFrame::ChildListChanged(), in the patch.

2- There, we walk up to an appropriate parent (e.g., to handle the &integral, an ancestor higher up tells that &integral how tall it ought to be to properly cover the formula). And it is here that the bit is set too early. As we climb up (c.f. the patch), the bit must be set on the current level, not the parent, a level up. Otherwise when we get in

3 - ReLayoutChildren(frame), which eventually calls ReflowDirtyChild(), this turns out to be a no-op because it (purposely) optimizes things out when a frame (repeatedly) calls it with its NS_FRAME_HAS_DIRTY_CHILDREN bit set (so that changes in different children coalesce into posting a single reflow event):

nsMathMLContainerFrame::ReflowDirtyChild()
{
  // The inline container frame does not handle the reflow
  // request.  It passes it up to its parent container.

  // If you don't already have dirty children,
  if (!(mState & NS_FRAME_HAS_DIRTY_CHILDREN)) {
    if (mParent) {
      // Record that you are dirty and have dirty children
      mState |= NS_FRAME_IS_DIRTY;
      mState |= NS_FRAME_HAS_DIRTY_CHILDREN;

      // Pass the reflow request up to the parent
      mParent->ReflowDirtyChild(aPresShell, (nsIFrame*) this);
    }
    else {
      NS_ASSERTION(0, "No parent to pass the reflow request up to.");
    }
  }

  return NS_OK;
}

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233937 [details] [diff] [review]
patch

r+sr=bzbarsky, but you might want to talk to dbaron about how this should work on reflow branch....

Comment 19

[rbs]

Checked in.

Okay if the reflow-branch will involve changing this. I don't mind revisiting it.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233937 [details] [diff] [review]
patch

a=beltzner on behalf of drivers for the mozilla_1_8_branch

Comment 21

[rbs]

fixed1.8.1: checked in the 1.8 branch.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 233937 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 23

[rbs]

I am behind an extremely slow connection at present and I am not getting cvs from my laptop:

D:\Mozilla\m1.8.0\mozilla\layout\mathml\base\src>cvs commit -m"js changes to MathML are not drawn, b=337700, r+sr=bzbarsky, a=dveditz" nsMathMLContainerFrame.cpp

ssh: connect to host cvs.mozilla.org port 22: Connection timed out
cvs [commit aborted]: end of file from server (consult above messages if any)

===========
Please, do well to checkin for me. Thanks.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

I checked this patch in on the 1.8.0 branch.
mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp 	1.113.14.3.2.4