Closed Bug 706193 Opened 13 years ago Closed 12 years ago

footer text on nytimes.com inflated too much

Categories

(Core :: Layout, defect, P1)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
mozilla14
Tracking Status
blocking-fennec1.0 --- beta+
fennec 11+ ---

People

(Reporter: madhava, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: mobile, Whiteboard: readability, [font-inflation: list])

Attachments

(6 files, 5 obsolete files)

15.86 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.46 KB, patch
roc
: review+
Details | Diff | Splinter Review
23.72 KB, patch
roc
: review+
Details | Diff | Splinter Review
17.21 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.24 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.25 KB, patch
roc
: review+
Details | Diff | Splinter Review
Feel free to dupe this to another font inflation bug
Assignee: nobody → dbaron
Priority: -- → P2
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.
Priority: -- → P1
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?
Whiteboard: readability → readability, [font-inflation: list]
(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?
OS: Mac OS X → Android
Hardware: x86 → ARM
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.
tracking-fennec: --- → 11+
(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.
Blocks: 707195
Blocks: 713241
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.
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED
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.
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
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*)?
Attachment #614587 - Attachment is obsolete: true
Attachment #614587 - Flags: review?(roc)
Attachment #614588 - Attachment is obsolete: true
Attachment #614588 - Flags: review?(roc)
Comment on attachment 614947 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)

oops, sorry, raced with comment 37
Attachment #614947 - Flags: review?(roc)
(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?
Attachment #614954 - Attachment is obsolete: true
Attachment #614954 - Flags: review?(roc)
(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.
Attachment #614974 - Flags: review?(roc)
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.
Attachment #615213 - Flags: review?(roc)
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
Attachment #614974 - Attachment is obsolete: true
Attachment #615214 - Flags: review?(roc)
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.
Attachment #615214 - Flags: review?(roc) → review+
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.
Keywords: mobile
Depends on: 747857
No longer depends on: 747857
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)
Status: RESOLVED → VERIFIED
Depends on: 768942
You need to log in before you can comment on or make changes to this bug.