As seen here: http://www.flickr.com/photos/madhava_work/6426486801/

Feel free to dupe this to another font inflation bug

In some sense, this is inflation doing what it's supposed to do. That said, this is text that's intended to fit on one line, so it doesn't really need inflation. I think iOS Safari has a character threshold for its inflation, so you need at least a certain size run of text in the same style before it triggers inflation. That said, this is probably more than enough text to trigger that. I'm curious if there's some other basis we could use to determine that it's ok not to inflate the text here.

I actually don't think this is too big. Instead, I'm more interested in the nytimes news articles being smaller (i.e. not inflated). Any idea why these //weren't// inflated? We could probably detect a situation like this, because these appear to be list elements. So, we could maybe detect a horizontally-laid out list, and suppress the inflation there?

(In reply to Scott Johnson (:jwir3) from comment #3) > I actually don't think this is too big. Instead, I'm more interested in the > nytimes news articles being smaller (i.e. not inflated). Any idea why these > //weren't// inflated? Why what exactly isn't inflated? > We could probably detect a situation like this, because these appear to be > list elements. So, we could maybe detect a horizontally-laid out list, and > suppress the inflation there? Detect how?

Basically, to fix this, we need to detect the situation where the text wouldn't be wrapped if it is not inflated. I'd suggest that we may actually want to allow 2 lines of text. I think this is in turn going to require caching information about font inflation (as will bug 706609). I'm thinking that I want to use 2 frame state bits: a. one to say whether something is a container for font size inflation b. the other to say whether the nearest ancestor-or-self container gets inflated Then, for each container, we'll have the chance to do a small amount of work (scanning text and its styles) to figure out (b) above, probably at frame-construction time. One issue here is that this can't actually just be per-container -- the text scanning would have to run across containers, since we want: <p>A lot of text</p> <p>Just a few words</p> to yield the same inflation for the two paragraphs, not small text for the one-liner. I suspect that, when doing this scanning, it may be sufficient to look at the intrinsic widths of the text and (for finding contiguous styles) at the styles of the block.

> I suspect that, when doing this scanning, it may be sufficient to look at > the intrinsic widths of the text and (for finding contiguous styles) at the > styles of the block. Actually, I don't think so, because this needs to handle parent/child relationships well. So I think it needs to look at the styles of the contiguous pieces of text.

(In reply to David Baron [:dbaron] from comment #6) > > I suspect that, when doing this scanning, it may be sufficient to look at > > the intrinsic widths of the text and (for finding contiguous styles) at the > > styles of the block. > > Actually, I don't think so, because this needs to handle parent/child > relationships well. So I think it needs to look at the styles of the > contiguous pieces of text. Except we probably need to look at more than text -- need to look at things like form controls, which can be inflated.

Brief status update: this is the next thing I'm planning to work on, but it requires a bit of research and investigation to figure out what to do and then how to do it (neither of which I know precisely), so there's basically zero chance I'm going to get it done before I leave for vacation and a CSS WG meeting.

David, Since you're on vacation, if you'd like, I can start on some of these things (e.g. adding the frame state bits and seeing if I can work through getting some of this set at frame construction time), and then we can get back together about it after you're vacation and CSS WG meetings.

(In reply to Scott Johnson (:jwir3) from comment #9) > (e.g. adding the frame state bits and seeing if I can work through getting > some of this set at frame construction time) Ah. I didn't realize that you had already added that as part of bug 706609. I'll look into seeing if I can utilize it for this (and possibly other) cases.

Beta blockers.

OK, to fix both this and bug 707195, I'm starting to think that maybe the right approach is to: * scan all of the text in each BFC, and for each font size used, gather: + the largest width at which that font size was used + the amount of text at that font size Then, to fix bug 706193, we wouldn't do inflation at all when there isn't a sufficient amount of text to merit it. To fix bug 707195, we'd keep the inflation ratio consistent within the BFC, and for each font size apply the largest width of it and the font sizes smaller than it. I still need to work out how to do dynamic updating.

(In reply to David Baron [:dbaron] from comment #13) > OK, to fix both this and bug 707195, I'm starting to think that maybe the > right approach is to: > > * scan all of the text in each BFC, and for each font size used, gather: > + the largest width at which that font size was used Except I don't see how we'd know the widths at the appropriate time (now that I'm trying to implement it). > + the amount of text at that font size > > Then, to fix bug 706193, we wouldn't do inflation at all when there isn't a > sufficient amount of text to merit it. > > To fix bug 707195, we'd keep the inflation ratio consistent within the BFC, > and for each font size apply the largest width of it and the font sizes > smaller than it. I'm inclined to give up on trying to fix bug 707195 (i.e., wontfix it) and try to approach this bug alone.

David, do you have a status update here?

Looks like the dep is ready to land.

Comment on attachment 614587 [details] [diff] [review] Add a font inflation data structure per block formatting context. (, patch 2) Review of attachment 614587 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +495,5 @@ > + NS_ASSERTION(aFrame->GetStateBits() & NS_FRAME_FONT_INFLATION_CONTAINER, > + "should only call this on containers"); > + nsIAtom *fType = aFrame->GetType(); > + return (aFrame->IsFrameOfType(nsIFrame::eBlockFrame) && > + (aFrame->GetStateBits() & NS_BLOCK_FLOAT_MGR)) || Do we need the IsFrameOfType check here? @@ +499,5 @@ > + (aFrame->GetStateBits() & NS_BLOCK_FLOAT_MGR)) || > + aFrame->IsFrameOfType(nsIFrame::eXULBox) || > + // For cells, NS_BLOCK_FLOAT_MGR is set on the anon box inside, > + // which isn't an inflation container at all (since its parent > + // has the same content node). Rearrange this expression so that we only call GetType if we need it, and so that we check everything else before doing IsFrameOfType calls. Maybe it would be better to simply have a new IsFrameOfType bit here?

Comment on attachment 614588 [details] [diff] [review] Build font data structure by walking the necessary text. (, patch 3) Review of attachment 614588 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +3572,5 @@ > CoordNeedsRecalc(metrics->mAscent); > } > + > + if (GetStateBits() & NS_FRAME_FONT_INFLATION_FLOW_ROOT) { > + nsFontInflationData::MarkFontInflationDataDirty(this); Why do we need to do this since you fully recreate font inflation data on every reflow anyway?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > Do we need the IsFrameOfType check here? Yes, since NS_BLOCK_FLOAT_MGR is a class-private bit. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > Why do we need to do this since you fully recreate font inflation data on > every reflow anyway? Where do I fully recreate them on every reflow?

Given the code changes, we'd like to see this land before beta and we still thinks this blocks.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > Rearrange this expression so that we only call GetType if we need it, and so > that we check everything else before doing IsFrameOfType calls. > > Maybe it would be better to simply have a new IsFrameOfType bit here? I think if we're worried about performance, it makes more sense to just spread the setting-the-bit logic out and put it in the various constructors of various frame classes as needed (e.g., with the !GetParent() in nsFrame, with an is-inflation-container check and the space manager check in nsBlockFrame, and with only an is-inflation-container check in nsBoxFrame and nsLeafBoxFrame, nsHTMLScrollFrame, nsXULScrollFrame, and nsTableCellFrame). Does that sound ok to you?

(In reply to David Baron [:dbaron] from comment #31) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > > Why do we need to do this since you fully recreate font inflation data on > > every reflow anyway? > > Where do I fully recreate them on every reflow? When we do this in nsHTMLReflowState::Init? if (frame->GetStateBits() & NS_FRAME_FONT_INFLATION_FLOW_ROOT) { // Destroy and re-create our font inflation data. nsFontInflationData::CreateFontInflationDataFor(*this); }

(In reply to David Baron [:dbaron] from comment #33) > I think if we're worried about performance, it makes more sense to just > spread the setting-the-bit logic out and put it in the various constructors > of various frame classes as needed (e.g., with the !GetParent() in nsFrame, > with an is-inflation-container check and the space manager check in > nsBlockFrame, and with only an is-inflation-container check in nsBoxFrame > and nsLeafBoxFrame, nsHTMLScrollFrame, nsXULScrollFrame, and > nsTableCellFrame). Does that sound ok to you? Yes!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34) > (In reply to David Baron [:dbaron] from comment #31) > > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > > > Why do we need to do this since you fully recreate font inflation data on > > > every reflow anyway? > > > > Where do I fully recreate them on every reflow? > > When we do this in nsHTMLReflowState::Init? > > if (frame->GetStateBits() & NS_FRAME_FONT_INFLATION_FLOW_ROOT) { > // Destroy and re-create our font inflation data. > nsFontInflationData::CreateFontInflationDataFor(*this); > } Hmmm. I should remove that.

Comment on attachment 614588 [details] [diff] [review] Build font data structure by walking the necessary text. (, patch 3) Review of attachment 614588 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFontInflationData.cpp @@ +146,5 @@ > + } > + > + PRUint32 len = frames.Length(); > + nsHTMLReflowState *reflowStates = > + static_cast<nsHTMLReflowState*>(malloc(sizeof(nsHTMLReflowState) * len)); moz_xmalloc? @@ +192,5 @@ > + } > + > + nsIFrame *nca = NearestCommonAncestorFirstInFlow(firstInflatableDescendant, > + lastInflatableDescendant, > + bfc); How important is this? How often is nca not equal to bfc? @@ +229,5 @@ > + // Goes in a different set of inflation data. > + continue; > + } > + > + if (kid->GetType() == nsGkAtoms::textFrame) { This doesn't handle XUL textboxes but I guess that's OK. @@ +265,5 @@ > + prevWS = true; > + } else { > + ++len; > + prevWS = false; > + } Move this out to something in nsTextFrameUtils I think ... say ComputeApproximateLengthWithWhitespaceCompression(nsStyleText*, nsIContent*)?

Comment on attachment 614947 [details] [diff] [review] Build font data structure by walking the necessary text. (, patch 3) oops, sorry, raced with comment 37

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37) > @@ +192,5 @@ > > + } > > + > > + nsIFrame *nca = NearestCommonAncestorFirstInFlow(firstInflatableDescendant, > > + lastInflatableDescendant, > > + bfc); > > How important is this? How often is nca not equal to bfc? I think at the top level of the page it can be kinda important, since pages are often "fixed width", but the fixed width is unlikely to be on the root and may not be on the body either.

Comment on attachment 614954 [details] [diff] [review] Build font data structure by walking the necessary text. (, patch 3) Review of attachment 614954 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFontInflationData.cpp @@ +131,5 @@ > + > + while (aFrame1 != aFrame2) { > + aFrame1 = aFrame1->GetParent()->GetFirstInFlow(); > + aFrame2 = aFrame2->GetParent()->GetFirstInFlow(); > + } Wouldn't it be simpler and faster to build two nsAutoTArrays of ancestor nsIFrame*s the way nsLayoutUtils::DoCompareTreePosition does? ::: layout/generic/nsHTMLReflowState.cpp @@ +318,5 @@ > + > + if (frame->GetStateBits() & NS_FRAME_FONT_INFLATION_FLOW_ROOT) { > + // Destroy and re-create our font inflation data. > + nsFontInflationData::EnsureFontInflationDataFor(*this); > + } Fix comment, since you're not destroying and recreating the data! In fact, why do we need this here? Can't we just call EnsureFontInflationDataFor from ShouldInflateFontsForContainer?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43) > why do we need this here? Can't we just call EnsureFontInflationDataFor from > ShouldInflateFontsForContainer? No, because it needs container widths. (I think it's avoidable, but doing so would be pretty complicated.)

Comment on attachment 614974 [details] [diff] [review] Build font data structure by walking the necessary text. (, patch 3) Turns out I have some try server orange to deal with related to the reflow state setup in patch 3, though.

What ensures that font inflation data is recomputed if some ancestor container width changes, during a resize reflow?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47) > What ensures that font inflation data is recomputed if some ancestor > container width changes, during a resize reflow? Right. That's why I had that code there. I should probably dirty the width information and the text size information separately.

The changes to this patch are pretty minor, to fix some assertions during crashtests/reftests. The most straightforward way to fix them (and I'm not 100% sure I've got them all, but I think it gets the first 10 or so that were hit, with 2 changes) was to make the following two changes which I think are both correct: * make any floating or abs-pos elements that are inflation containers also be flow roots (unfortunately, I couldn't find a point during frame initialization when NS_FRAME_OUT_OF_FLOW is set reliably, so I just used disp->IsFloating() || disp->IsAbsolutelyPositioned(). * make SVG outer frames and foreignObject frames always be both inflation containers and flow roots This avoids hitting some assertions related to reflow state construction, which these things expect to be done in peculiar ways.

This has some pretty major changes to address the comments. Mainly: * width recomputation and text rescanning are now separate (addressing comment 48, which was a mistake in addressing comment 36) * I added the optimization to stop scanning text when we cross the threshold, which will avoid scanning all of large swaths of text

Comment on attachment 615214 [details] [diff] [review] Build font data structure by walking the necessary text. (, patch 3) Review of attachment 615214 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFontInflationData.cpp @@ +196,5 @@ > + > + nsIFrame *firstInflatableDescendant = > + FindEdgeInflatableFrameIn(bfc, eFromStart), > + *lastInflatableDescendant = > + FindEdgeInflatableFrameIn(bfc, eFromEnd); May as well move this call to the if (firstInflatableDescendant) path.

https://tbpl.mozilla.org/?tree=Try&rev=e62c45257f01 has a few more failures I need to deal with (a bunch of assertions that are a regression from the SVG change in comment 49, I think because it was missing one additional piece needed), and a crash that looks like a null dereference so probably ought to be easy. I'll look more closely in the morning.

Two small changes to fix those failures: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/34274b741da6 https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/2858f41baab4 and a new try run: https://tbpl.mozilla.org/?tree=Try&rev=2038675d9c88

https://hg.mozilla.org/integration/mozilla-inbound/rev/30dce13b71d0 https://hg.mozilla.org/integration/mozilla-inbound/rev/9499f6b28add https://hg.mozilla.org/integration/mozilla-inbound/rev/9cf58850cf26 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c18caf33991 https://hg.mozilla.org/integration/mozilla-inbound/rev/e44a95efa5f0

https://hg.mozilla.org/mozilla-central/rev/30dce13b71d0 https://hg.mozilla.org/mozilla-central/rev/9499f6b28add https://hg.mozilla.org/mozilla-central/rev/9cf58850cf26 https://hg.mozilla.org/mozilla-central/rev/0c18caf33991 https://hg.mozilla.org/mozilla-central/rev/e44a95efa5f0

The footer text on nytimes.com now looks good. Marking this as verified fixed on build: Firefox 15.0a1 (2012-05-27) Device: LG Optimus 2X (Android 2.2.2)