footer text on nytimes.com inflated too much

VERIFIED FIXED in mozilla14

Status

()

Core
Layout
P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: madhava, Assigned: dbaron)

Tracking

(Blocks: 1 bug, {mobile})

unspecified
mozilla14
ARM
Android
mobile
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 beta+, fennec11+)

Details

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

Attachments

(6 attachments, 5 obsolete attachments)

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
(Reporter)

Description

6 years ago
As seen here: http://www.flickr.com/photos/madhava_work/6426486801/
Feel free to dupe this to another font inflation bug
Assignee: nobody → dbaron
Priority: -- → P2
(Assignee)

Updated

6 years ago
Blocks: 627842
Priority: P2 → --
(Assignee)

Comment 2

6 years ago
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]
(Assignee)

Comment 4

6 years ago
(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?
(Assignee)

Updated

6 years ago
Blocks: 711759
OS: Mac OS X → Android
Hardware: x86 → ARM
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
> 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+
(Assignee)

Comment 7

6 years ago
(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.

Updated

6 years ago
Blocks: 707195

Updated

6 years ago
Blocks: 713241
(Reporter)

Updated

6 years ago
Keywords: fennecnative-betablocker
(Assignee)

Comment 8

6 years ago
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.
Keywords: fennecnative-betablocker
(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.
Duplicate of this bug: 720567
Keywords: fennecnative-releaseblocker

Updated

5 years ago
Keywords: fennecnative-releaseblocker → fennecnative-betablocker

Comment 12

5 years ago
Beta blockers.
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
(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.

Updated

5 years ago
Duplicate of this bug: 718501
Blocks: 714080
Blocks: 719685
Blocks: 729961

Updated

5 years ago
Duplicate of this bug: 713241
(Assignee)

Updated

5 years ago
Duplicate of this bug: 711759
(Assignee)

Updated

5 years ago
Duplicate of this bug: 714080
(Assignee)

Updated

5 years ago
Duplicate of this bug: 719685
(Assignee)

Updated

5 years ago
Duplicate of this bug: 729961

Comment 21

5 years ago
David, do you have a status update here?
(Assignee)

Updated

5 years ago
Depends on: 743105
Duplicate of this bug: 735609

Comment 23

5 years ago
Looks like the dep is ready to land.
(Assignee)

Updated

5 years ago
Depends on: 743817
(Assignee)

Comment 24

5 years ago
Created attachment 614586 [details] [diff] [review]
Add a preference for the threshold at which we have enough text within a BFC to use font size inflation.  (, patch 1)
Attachment #614586 - Flags: review?(roc)
(Assignee)

Comment 25

5 years ago
Created attachment 614587 [details] [diff] [review]
Add a font inflation data structure per block formatting context.  (, patch 2)
Attachment #614587 - Flags: review?(roc)
(Assignee)

Comment 26

5 years ago
Created attachment 614588 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)
Attachment #614588 - Flags: review?(roc)
(Assignee)

Comment 27

5 years ago
Created attachment 614589 [details] [diff] [review]
Use the font inflation data to disable font inflation for small pieces of text.  (, patch 4)
Attachment #614589 - Flags: review?(roc)
(Assignee)

Comment 28

5 years ago
Created attachment 614590 [details] [diff] [review]
Add tests for font.size.inflation.lineThreshold preference and the associated threshold behavior.  (, patch 5)
Attachment #614590 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Attachment #614586 - Flags: review?(roc) → review+
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?
Attachment #614589 - Flags: review?(roc) → review+
(Assignee)

Comment 31

5 years ago
(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.
(Assignee)

Comment 33

5 years ago
(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!
Attachment #614590 - Flags: review?(roc) → review+
(Assignee)

Comment 36

5 years ago
(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*)?
(Assignee)

Comment 38

5 years ago
Created attachment 614946 [details] [diff] [review]
Add a font inflation data structure per block formatting context.  (, patch 2)
Attachment #614946 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #614587 - Attachment is obsolete: true
Attachment #614587 - Flags: review?(roc)
(Assignee)

Comment 39

5 years ago
Created attachment 614947 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)
Attachment #614947 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #614588 - Attachment is obsolete: true
Attachment #614588 - Flags: review?(roc)
(Assignee)

Comment 40

5 years ago
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)
(Assignee)

Comment 41

5 years ago
(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.
Attachment #614946 - Flags: review?(roc) → review+
(Assignee)

Comment 42

5 years ago
Created attachment 614954 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)
Attachment #614954 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #614947 - Attachment is obsolete: true
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?
(Assignee)

Comment 44

5 years ago
Created attachment 614974 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)
Attachment #614974 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #614954 - Attachment is obsolete: true
Attachment #614954 - Flags: review?(roc)
(Assignee)

Comment 45

5 years ago
(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.)
(Assignee)

Comment 46

5 years ago
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?
(Assignee)

Comment 48

5 years ago
(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.
(Assignee)

Comment 49

5 years ago
Created attachment 615213 [details] [diff] [review]
Add a font inflation data structure per block formatting context. (, patch 2)

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)
(Assignee)

Comment 50

5 years ago
Created attachment 615214 [details] [diff] [review]
Build font data structure by walking the necessary text. (, patch 3)

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)
Attachment #615213 - Flags: review?(roc) → review+
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+
(Assignee)

Comment 52

5 years ago
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.
(Assignee)

Comment 53

5 years ago
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
(Assignee)

Comment 54

5 years ago
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
Target Milestone: --- → mozilla14
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: mobile

Updated

5 years ago
Depends on: 747857

Updated

5 years ago
No longer depends on: 747857
(Assignee)

Updated

5 years ago
Depends on: 747857

Comment 56

5 years ago
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

Updated

5 years ago
Depends on: 768942
You need to log in before you can comment on or make changes to this bug.