848973 - Document what "aFrame" means in nsStyleDisplay::IsBlockInside(nsIFrame* aFrame) & friends

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Up to now, I've been assuming that in calls like...
  styleDisplay->IsBlockInside(frame)
are supposed to be passing the frame that corresponds to the given styleDisplay -- that is, I was assuming that styleDisplay should equal frame->StyleDisplay().

However, it looks like that's not the case in usages within nsCSSFrameConstructor, so I think I was misunderstanding -- but it's not 100% clear.

e.g. this is very confusing:
> 4361   // If the frame is a block-level frame and is scrollable, then wrap it in a
> 4362   // scroll frame.
[...]
> 4366   if ((aParentFrame ? aDisplay->IsBlockInside(aParentFrame) :
> 4367                       aDisplay->IsBlockInsideStyle())
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#4361

I *think* we're asking whether the not-yet-constructed frame is going to be a block -- but the code there reads as if we're asking whether *aParentFrame* is a block.  And the IsBlockInside() declaration doesn't really help clear this up
> 1701   inline bool IsBlockInside(const nsIFrame* aFrame) const;
https://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#1701

From reading that (taking "aFrame"), I'd expect that we're asking whether that frame is a block.  You have to go in and read the implementation to figure out what aParentFrame is used for there. (We just check if it's a SVG text frame, and return early if it is.)

Filing this bug on adding documentation to IsBlockInside (and its various related methods in nsStyleStruct) to indicate the expectations on the passed-in frame, and possibly rename the frame to something clearer (perhaps aContextFrame? or aParentFrame, if it's always expected to be the parent of the StyleDisplay in question? (not sure if it is))

Comment 1

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

(In reply to Daniel Holbert [:dholbert] from comment #0)
> possibly rename the frame to something clearer (perhaps
> aContextFrame? or aParentFrame, if it's always expected to be the parent of
> the StyleDisplay in question? (not sure if it is))

yes, please!  (whatever the answer is)

Comment 2

[Daniel Holbert [:dholbert]]

Assigning to heycam, to be sure this doesn't fall off the radar -- I suspect he's in the best position to address this, since IIRC he wrote this code. :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

heycam is on vacation for a few more weeks.

roc, do you happen to recall how this was supposed to work?

Comment 4

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

(In reply to Daniel Holbert [:dholbert] from comment #0)
> Up to now, I've been assuming that in calls like...
>   styleDisplay->IsBlockInside(frame)
> are supposed to be passing the frame that corresponds to the given
> styleDisplay -- that is, I was assuming that styleDisplay should equal
> frame->StyleDisplay().

That's the intent.

> However, it looks like that's not the case in usages within
> nsCSSFrameConstructor, so I think I was misunderstanding -- but it's not
> 100% clear.

That looks like a bug. Not a serious one right now since SVG text frames are preffed off.

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> > However, it looks like that's not the case in usages within
> > nsCSSFrameConstructor, so I think I was misunderstanding -- but it's not
> > 100% clear.
> 
> That looks like a bug. Not a serious one right now since SVG text frames are
> preffed off.

It's a bug, but not one that would be causing trouble.  I think we'll never get into FindDisplayData for SVG text since FindSVGData will always either return fcdata for a <tspan> or to suppress the frame.  I think we can just change these tests to aDisplay->IsBlockInsideStyle().

One that is needed is here:

https://hg.mozilla.org/mozilla-central/file/tip/layout/base/nsCSSFrameConstructor.cpp#l10457

We're creating a first-letter frame, but we need to decide before creating it whether it is floating.  SVG text should not have floats, so we pass in aParentFrame as the frame to check whether it IsSVGText().

So we could either document the frame parameter to these methods as being "a frame to check whether it is SVG text in which case the returned value is appropriate for SVG text", or we could limit these calls to only passing in the exact frame the style struct is for, and for other cases like this one just check aParentFrame->IsSVGText() and then call IsFloatingStyle().  Any preference?

Comment 6

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

(In reply to Cameron McCormack (:heycam) from comment #5)
> So we could either document the frame parameter to these methods as being "a
> frame to check whether it is SVG text in which case the returned value is
> appropriate for SVG text", or we could limit these calls to only passing in
> the exact frame the style struct is for, and for other cases like this one
> just check aParentFrame->IsSVGText() and then call IsFloatingStyle().  Any
> preference?

The latter please :-)

Comment 7

[Daniel Holbert [:dholbert]]

Also: assuming we do the latter, let's please add an assertion to enforce it -- i.e., nsStyleDisplay::IsBlockInline()'s impl should assert that aFrame->StyleDisplay() == this"

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch

I think I found all the cases where the frame being passed in might not be the one that has the relevant style struct.  I added assertions in the style struct functions and got a green try run: https://tbpl.mozilla.org/?tree=Try&rev=d2a6a88bd932

Comment 9

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c04824522d46

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c47b3dda9c2d - at least 10.8 and Android have reftest bustage, it'll be a while before everything else has had a chance to run and fail to know if there's more than them.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: patch v2

So it took a bit of time to track down exactly why the assertions were being hit, since I couldn't trigger them locally.

Re-requesting review, since I made some non-trivial changes.  In the first hunk, I've just expanded the IsPositioned call since it works on the parent frame.  In nsCSSFrameConstructor::ConstructFrameFromItemInternal, I made an actual change to use maybeAbsoluteContainingBlock's display style struct rather than the one on the frame that gets maybe-wrapped by a scroll frame.  I'm not exactly sure about this (since I'm not sure what the intention of the existing code is), but it passes try.  The other changes are mostly simplifications where we wouldn't be inspecting style structs on an SVG text frame anyway.

Comment 12

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb8ac3f27a2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9cb8ac3f27a2