64763 - Switch from nsRect mRect to nsArea in the frame model

Status: RESOLVED INCOMPLETE

(Core :: Layout, defect)

Comment 1

[rbs]

Bug 51684 suggests a number of fundamental items to evaluate prior
freezing nsIFrame for Mozilla 1.0. One such suggestion is the switch
from 'nsRect mRect' to 'nsArea mArea' in the geometric frame model.

The pros and cons of this are detailed in bug 51684. Here is a summary of
the motivations. Since the basic rectangular metrics (nsRect mRect) stored
in the frames are too limited for layout purposes, the suggestion is to 
switch from nsRect mRect (defined as: struct nsRect { x, y, width, height}) 
to a nsArea mArea (defined as: struct nsArea { x, y, width [, height],
ascent, descent }).

Or to put literally, let the frame store its 'ascent' and 'descent' together 
with its other dimensions.

(Although height = ascent + descent, the 'height' may be kept in nsArea 
for convenience. Indeed, as we shall we see below, storing the extra ascent,
descent information can bring savings to the overall system. However, the
'height' could well be removed later to get additional savings).

The fact that the ascent and descent are not stored in the base frame class
has been the source of undue complications. Indeed, nearly all frame owners
have come to the realisation that they need to keep track of the
ascent and descent of their frames. As a result, people cache their 
aDesiredSize.ascent and/or aDesiredSize.descent from reflow, and this 
means that separate getter/setter methods are needed for them.

Since (with hindsight) we now see that nearly all frames (XUL/CSS/Table/MathML)
need their ascent/descent information long after the life time of aDesiredSize
from reflow, nsArea will provide a unified way to store/retrieve the 
information, thus leading to leaner/cleaner code, and saving time to developers
who would not need to devise work-around to the problem. Moreover, nsArea
coincides with the metrics used in the box model of the XSL-FO spec, and so
nsArea can be seen as a necessary building block with which to consolidate
nsIFrame to later support XSL-FO.

I have taken a first step in the switching direction by setting up
nsArea.h/nsArea.cpp (like nsRect.h/nsRect.cpp). Then, using an 
'#ifdef MOZ_USE_NSAREA', I have added GetArea/SetArea in nsIFrame.h,
and made the following change in nsFrame.h :

class nsFrame : public nsIFrame
{
  [...]

#ifndef MOZ_USE_NSAREA
  nsRect           mRect;
#else
  nsArea           mRect;
#endif

  [...]
}

Then with related changes in nsFrame.cpp for consistency (see attachments),
I was able to rebuild and run the application without problems. This worked
straightaway because nsRect is a proper subset of nsArea (descent=0), and so a
direct mapping to the present code is possible. However, if the switch has to
be carried out properly, it will be necessary to rename mRect to mArea to avoid
confusions. It will also be necessary to remove the various codes that
permeate layout just because the ascent/descent are not stored in the base
frame class. The later represent where the benefits will come from.

Like string changes that have been made, this switch will require gradual
passes over the tree. Supporting both GetRect/SetRect and GetArea/SetArea
in layout, and  removing the rect variants (and related work-around) when
the changes are over.

This bug has been opened to track the switch.

If folks object to the switch, could they voice their concerns so that I don't
waste further precious cycles on something that is doomed.

Comment 2

[rbs]

Attachment: gfx/public/nsArea.h

Comment 3

[rbs]

Attachment: gfx/src/nsArea.cpp

Comment 4

[rbs]

Attachment: gfx diff for a first try at nsArea.h/.cpp in the frame code

Comment 5

[rbs]

Attachment: layout diff for a first try at nsArea in the frame code

Comment 6

[rbs]

Attachment: occurrences of mRect in layout (with line numbering)

Comment 7

[Boris Zbarsky [:bzbarsky]]

ccing roc, since he's in the process of rewriting nsIFrame anyway....

roc?  dbaron?  Thoughts on this?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

As bug 189604 suggests, we may not need to track the descent at all.

Comment 9

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

I think we should keep the nsRect GetRect() that we have right now. We need the
height much more often than we need the ascent or the descent.

I think we should definitely have GetAscent() and GetDescent() methods on nsIFrame.

I'm not convined we should move an mDescent member into nsIFrame. For some
frames (e.g., SVG elements, images, OBJECT) it's not needed (right?). The
default in nsIFrame should be to for GetDescent() to return 0 and for
GetAscent() to return the height. We can of course move mDescent declarations as
far up the class hierarchy as makes sense (maybe up to nsHTMLContainerFrame).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Right; we only need to track 2 of { ascent, descent, height }.  The question is
which two.

Comment 11

[rbs]

>I think we should keep the nsRect GetRect() that we have right now.

Why do you think so? Legacy? (i.e., doing a switch over is too much hassle?)
BTW, from bug 1777 nsLineBox got its separate mAscent too :-( Whereas if layout
objects were switched to nsArea, so that people felt compelled to fill it
properly, the foundation will be better.

> We need the
>height much more often than we need the ascent or the descent.
>
>I'm not convined we should move an mDescent member into nsIFrame. For some
>frames (e.g., SVG elements, images, OBJECT) it's not needed (right?).

Taking just ascent and height is fine too. SVG has its SGVtext that can follow a
path an be aligned in various way.

What I have found very disturbing with this issue is that it is a core issue.
When these ascent (or descent) metrics are missing, it is very difficult to
work-around the problem (e.g., bug 55456). One basically has to go all the way
back to recover it, and the resulting code after that isn't clean. The code
becomes more obscure than necessary. Another problem is when mixing/re-using
different frames. Sometimes one may instantiate a certain frame just to benefit
from the behavior that the frame provides, and if that frame didn't have its
ascent (or descent), it is really a pain to recover that. Bug 130958 illustrates
this well. If the ascent was computed from the outset, it would be trivial to
fix the bug. But now, one has to go back and check (repeating pretty much what
bz did in bug 167236 and suffered from regressions). So yes, having |mAscent| is
good, but perhaps having |nsArea| is better as a foundation.

Comment 12

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

Check out bug 190375 where the nsIFrame cleanup patch is.

> nsLineBox
isn't an nsIFrame.

> SVG has its SGVtext
Yeah but for most SVG frames, e.g. line segements, there is no meaningful
ascent/descent. In general there is only a meaningful ascent/descent for things
that can contain text.

There are a couple of overlapping issues here: what should the nsIFrame
interface be, and how should it be implemented.

*** INTERFACE ***

I agree that all frames should be able to return their ascent and descent at any
time. Currently my nsIFrame patch has three geometry getters:
   nsRect GetRect() const;
   nsPoint GetPosition() const;
   nsSize GetSize() const;
I propose we add another getter for the descent:
   nscoord GetDescent() const;
I don't think having an additional "nsArea GetArea()" is all that helpful, and I
suspect it would mostly lead to confusion since "Area" isn't meaningfully
different from "Rect".

Chucking nsAreas instead of nsRects all over the place might make people think
more about ascents, but I don't think it's the right solution to that problem. I
suspect we'd end up in a mess of nsRect/nsArea conversion with mysterious
ascent/descent lossage.

One thing to remember is alternative writing-modes, e.g. tbrl. In some
writing-modes the invariant is "ascent+descent = width". I assume we don't want
to have multiple per-axis ascent/descent :-). Anyway, so that frames can have
reasonable defaults without being writing-mode aware, I suggest we don't have an
explicit method to return the ascent. Then a frame such as an image or an OBJECT
can return 0 for the descent and its context can figure out if the ascent should
be the width or the height.

*** IMPLEMENTATION ***

The implementation's not nearly as important as the interface, as long as there
is one :-). As I said above, frames which can't contain text (including things
like IFRAMEs where the text isn't logically part of the same document) can just
return 0 for the descent. Some frames can just forward a GetDescent() call to a
child frame and adjust the result. Other frames should cache it in an mDescent
member.

Comment 13

[rbs]

> > nsLineBox
> isn't an nsIFrame.

Yeah, but I was making the case that nsArea would have fitted the bill there too.
Rather than nsLineBox::mRect + nsLineBox::mDescent, just have nsLineBox::mArea.

> > SVG has its SGVtext
> Yeah but for most SVG frames, e.g. line segements, there is no meaningful
> ascent/descent. In general there is only a meaningful ascent/descent for things
> that can contain text.

True. But when it is missing somewhere, the suffering is shared. For example, in
the midst of processing/aligning a frame child list, one doesn't know which
child frame is which -- unless a special-casing is done and that's why I was
saying that handling the issue adds undue obscurity in the code of consumers.

   nsRect GetRect() const;
   nsPoint GetPosition() const;
   nsSize GetSize() const;

-  nscoord GetDescent() const;
+  nscoord GetBaseline() const;

do you buy this? might return the same as height (rather than returning 0 as
GetDescent() would have done.)

Although it could later be |nsPoint GetBase()| or something like that [e.g.,
nscoord GetYBaseline() / GetXBaseline() or some other names], if the intention
is to support top-to-bottom layout as per CSS3, so that base.y could allow to
stack frames horizontally while base.x could allow to stack them vertically.

>I don't think having an additional "nsArea GetArea()" is all that helpful, and I
>suspect it would mostly lead to confusion since "Area" isn't meaningfully
>different from "Rect".

I would have cut the APIs and gone for:

   nsArea Get/SetArea() const;
   nsPoint Get/SetPosition() const;

Then, have some separate helpers, e.g., nsRect(const nsArea&), for the few
consumers who want to pass this outside of layout. LXR showed that only a
handful of consumers use nsIFrame::GetSize(). And there is a problem with
SetSize() becauses it requires setting the baseline too so that things are kept
in sync.

> As I said above, frames which can't contain text (including things
> like IFRAMEs where the text isn't logically part of the same document) can just
> return 0 for the descent.

An interesting observation so far is that nearly all frames need the ascent (or
descent) information at some point, either directly or indirectly. Even <object>
may have <object type="text/...">. And special-casing the problem adds undue
obscurity.

As you said the interface is what matters... Thinking of top-to-bottom layout
(something that IE already does), nsArea could set a nice scenery because it
would have helped to get the vanilla nsRect out of the way, and would have
prepared people to think outside-the-rect-box. It could be defined to be
"interpreted" with the extra. (Eh, I am making the case for it, of course :-)

Comment 14

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

> For example, in the midst of processing/aligning a frame child list, one
> doesn't know which child frame is which

Right, that is definitely going to be fixed by adding a GetAscent() or
GetDescent() or GetBaseline() that will work on every frame.

> -  nscoord GetDescent() const;
> +  nscoord GetBaseline() const;
>
> do you buy this? might return the same as height (rather than returning 0 as
> GetDescent() would have done.)

[I just read the CSS3 text and line modules. Yow. Let's just ignore the
non-alphabetic baselines for now.]

The problem with GetBaseline(), if you intend it to be the offset from the top
or left edge of the frame to the alphabetic baseline, is that for non-text leaf
frames we'd have to check the current writing-mode and return either the height
(for horizontal inline flows) or 0 (for vertical inline flows).

How about "GetAlphabeticBaselineOffset()", which returns the offset from the
bottom edge of the frame (for horizontal inline flows) or the left edge of the
frame (for vertical inline flows) to the frame's alphabetic baseline? (i.e.,
'GetDescent()' with a better name and definition)

Comment 15

[rbs]

Iterating on what you suggest, what about |nsPoint GetBaselineOffset()|
and let the callee fill this anchor point according to the writing-mode so that 
callers use the returned (x,y) as they see fit.

Comment 16

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

But then leaf frames will (eventually) have to check the writing-mode. We may as
well avoid that and just let them return 0.

Comment 17

[Mats Palmgren (inactive)]

It seems there isn't anything concrete to fix here.  Closing.