[Webkit-unassigned] [Bug 82353] msqrt's implied mrow should do operator stretching

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=82353





--- Comment #5 from Dave Barton <dbarton at mathscribe.com>  2012-03-27 11:46:06 PST ---
(In reply to comment #4)
> (From update of attachment 134098 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134098&action=review
> 
> The result looks OK. The code looks tolerable.  (Nothing against your coding skills, simply ho-hum on this design we hashed out at our meeting.)  Would be curious of Julien's opinion.

I have been thinking & reading code & trying out things since our meeting, and I think we can basically use absolute positioning instead of anonymous blocks for a lot of stuff - so the next bug/patch may be less ho-hum!

> > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:81
> > +    newStyle->setPaddingLeft(Length(gFrontWidthEms * style()->fontSize(), Fixed));
> > +    newStyle->setPaddingTop(Length(gPaddingTopEms * style()->fontSize(), Fixed));
> 
> So this overrides any padding-left/padding-top set by CSS (as we discussed), just confirming?

Yes, this patch overrides any user padding-left/padding-top. I don't remember us discussing it, and it isn't ideal, but it's no worse than what's done now. Fixing this is definitely lower priority than getting basic MathML working in Safari/Chrome/etc., IMHO, since users will rarely add extra padding to a <msqrt>, and could do it if necessary with an <mpadded> or other wrapper element.

> > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:83
> > +    setNeedsLayout(true, false);
> 
> I wish these were enums. :)  Maybe I'll fix that today!
> 
> > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:166
> > +    ASSERT(style()->refCount() == 1);
> 
> You should explain why this is with a brief comment.

I'll add a comment.

> > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:168
> > +    setNeedsLayout(true, false);
> 
> Is this needed to cause our children to layout?

I may be confused, but I added it for the opposite reason. If our children are perhaps marked as needing layout, but we aren't, then some really smart code might not pick up our style()->setPaddingBottom(...). I thought to be pedantic, we should ensure selfNeedsLayout() here.

> > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:177
> > +        setNeedsLayout(true, false);
> > +        
> > +        RenderBlock::layout();
> 
> This style of calling layout from within layout concerns me.  These could be huge-subtrees we're re-laying-out.   It may be OK for now, but do we have a plan for the "right" way?  Do we know if other parts of the code call setNeedsLayout() and layout() from within layout()?

Huge subtrees shouldn't be re-layed-out. Our children will be marked as not needing layout after our first layout() call, so they won't be laid out again by our second layout() call. (Right?)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.