The titles of the articles bleed out of the divs in a slightly zoomed in view. See screenshot Repro: 1) nightly fennec, 12/6/11 build, Samsung Galaxy S 2 2) launch URL in portrait view 3) Verify titles overflow out of divs Expected: - title fits within div Actual: - title bleeds out of div

Does it get better if you set the font.size.inflation.minTwips pref to 0?

yeah, its better.

So we need to figure out if there's some characteristic of the page that makes it reasonable to disable inflation for those divs. (It's not immediately clear to me why they'd overflow when they were inflated -- figuring that out might lead to an answer.)

So, what I think is happening here is that the containing div doesn't change size with the text. I can reproduce this same effect when I manually inflate the text size on desktop to 4.9em (using DOM inspector).(See attached screenshot). I'm wondering if we should have a condition to inflate the text size only so far that the containing width will allow, if the width doesn't change with the size of the text.

I believe this has been resolved with bug 707855. Please reopen if you're still seeing this issue.

Oops, I spoke too soon. It looks like the content I was looking at on the page when I tested it simply didn't exhibit the behavior.

So, from my debugging, I am finding that the following happens when the marketwatch page is loaded with visual debugging turned on, and emPerLine set to '8' (taken from a marketwatch snapshot earlier today): http://imgur.com/gKVLb Now, when I try to inspect the element in question using the inspector and click the 'style' button, as soon as the inspector comes up on the right side, the page resizes itself to what should be the correct sizing (i.e. it fits within the specified frame and content container): http://imgur.com/wJQQJ (Note that the blue border indicates the frame of the text content, and the red border is being drawn for the font inflation container.) So it looks like the frame sizes are all correct, but for some reason, the font size inflation isn't respecting the sizing of the container...

I think I may have found the problem. Inside of nsLayoutUtils::FontSizeInflationInner(), the following appears: // To scale 0-1.5 times min to instead be 1-1.5 times min, we want // to the desired multiple of min to be 1 + (ratio/3) (where ratio // is our input's multiple of min). The scaling needed to produce // that is that divided by |ratio|, or: return (1.0f / ratio) + (1.0f / 3.0f); I don't think this is correct. If we have the following: original font size: 960 minimum font size (calculated): 6220 ratio = 0.154341 (1.0f/ratio) + (1.0f/3.0f) = 6.812500 So this comes way out of the range 1 - 1.5. In order to get this into the range 1 - 1.5, I modified this return statement to be: return (1.0f + (ratio * 0.5f)) Now, we get FontSizeInflationInner() returning 1.077170 for the same case, which is within the desired range, and works well with the site mentioned in this bug. David: Am I missing something here? Is there a reason we want this result to return such a high value, or is the change I made seem more correct to you?

Beta blockers.

David, can you answer comment #9?

(In reply to JP Rosevear [:jpr] from comment #11) > David, can you answer comment #9? Hi JP: David and I have been working closely on this bug. comment 9 describes a solution that I previously thought would work, but it's actually not on the right track. As David explained to me on IRC (which I didn't post here, sorry), the FontSizeInflationInner() function returns a different ratio than is originally computed. The original ratio that's computed inside of this method is the ratio of the current font size to the minimum font size, but the ratio that's returned from the function is the ratio of the inflated font size to the original font size. So, that's where I was getting confused. We're still working on this bug, but in order to update everyone watching this bug on what's happening, I'll post an abbreviated transcript of an email between myself and David: --- ORIGINAL EMAIL (from Scott to David) --- After some debugging, and some thought, I'm still having some difficulty making sense of what's happening in the nsTextFrame class that could be causing the problems exhibited in bug 708187. I was hoping I might be able to give you some idea of what I have done thus far, and perhaps ellicit some aid in tracking down the problem. After our last meeting, I began looking into possible reflow problems. Although I know it's not a permenant fix, I wanted to see if perhaps adding the following to the end of nsTextFrame::SetFontSizeInflation() would actually fix the problem: nsIPresShell* shell = PresContext()->GetPresShell(); if (shell && !shell->IsReflowLocked()) { shell->FrameNeedsReflow(this, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY); } The idea being that once the font size inflation has changed, reflow, if not already in progress, would be initiated. If you add this code just before the end of the nsTextFrame::SetFontSizeInflation() in nsTextFrameThebes.cpp, it does (initially) work to alleviate the problem on titles that seem to show the problem on marketwatch.com. Unfortunately, as can be seen in the following reduced test-case: http://people.mozilla.org/~sjohnson/b708187/ If you mouse-over the boundary between "Greece shunted to back-burner" and "Day ago losses recouped", with a font-inflation setting of 8 em per line (abnormally large, so that I can see behavior on desktop), you'll begin to see that sometimes, during paint, the text size for "Day ago losses recouped" increases significantly. (If you had it on the setting I recommended, then my printf statements tell me that it's increasing in ratio size from what it should be, 1.434607, to an abnormally large size, 2.53880). The reason I know that it's during paint is because of the debugging I've done to this point. What seems to happen is that, during ReflowText(), the font inflation ratio is calculated as I would expect (that is, at the 1.434... ratio for the frame containing the text "Day Ago losses..."), but during PaintText(), it *sometimes* will get an incorrect font inflation ratio of 2.53... (which, incidentally, happens to be the same ratio as that of the frame containing the text "Greece shunted to back burner"). My thinking was that perhaps there might be something incorrect in how the frame's ratio was being cached, so I switched it from an entry in Properties() to a full member variable, with no success, but this did help me debug where it was getting modified. It so happens that in BOTH reflow and painting, the same basic stack happens to be: http://people.mozilla.org/~sjohnson/b708187/stack-frame.txt This is really strange to me, because when I look at this, it seems like the font inflation is calculated in the call to EnsureTextRun(aWhichTextRun), where if aWhichTextRun == eInflated, then it uses GetFontSizeInflation(), which is already set to a bogus value. Just for kicks, I thought about trying to recalculate the font size inflation with the code: f->SetFontSizeInflation(nsLayoutUtils::FontSizeInflationFor(f, nsLayoutUtils::eInReflow)); at ~ line 2390 in nsTextFrameThebes (just before the line f->SetTextRun(...)). Unfortunately, FontSizeInflationFor() always seems to return 1.0 here. --- RESPONSE (From David to Scott) --- So one thing that's weird about text frames, and much of what makes this code interesting, is that a lot of stuff is managed by text runs, which are same-style runs of text within the same block... which are allowed to cross frame boundaries. This lets us do things like make correct ligatures even crossing a <span> boundary. Text runs are constructed lazily, but when we do construct them, we set the appropriate pointers for all the frames associated with the text run. So when I added font size inflation, it gets constructed sort of the same way. The basic idea, if my memory serves correctly, is that the frame that's asking for the text run computes its inflation (e.g., the aInflation parameter to EnsureTextRun), and then if the frame doesn't already have a text run, we: * make the text run, passing that already computed aInflation through the process * assign that aInflation to all the frames that the text run is associated with at the time we assign the text run to them Perhaps part of what's going wrong here is that we're extending the text run across frames that actually want to end up with different inflation? In that case, maybe the problem is that there's a condition missing from BuildTextRunsScanner::ContinueTextRunAcrossFrames ? --- END EMAIL --- So, this last part is what I'm currently looking at trying, to see if I can come up with a workable solution.

I am adding a check inside of ContinueTextRunAcrossFrames() to not continue a text run if font inflation is different between two frames. This would normally happen in a case like that of attachment 606358 [details]. This only gets us part of the way there, though... if you load attachment 606358 [details] with this patch applied and enable font inflation to 12 emPerLine, it works as expected (i.e. the text does not extend outside of frame boundaries, and the two words "Hello" and "Worldz" are inflated, but at different levels). If you refresh, though, the text from "Worldz" grows larger, and extends beyond its frame. This is happening because, somehow, mInflation inside of the BuildTextRunsScanner is being set to almost double what it should be set at, and thus inside of FlushFrames(), this is applied to the newly built text run, resulting in a larger-than-expected font. I'm still working on this last component, but I would like feedback to make sure I'm on the right track.

I think I've found a fix for this, but it requires us to not cache the inflation inside of BuildTextRunsScanner. I'm not sure if recalculating font size inflation from within BuildTextRunForFrame is too expensive. If it's not too expensive, then I will remove the mInflation member variable within BuildTextRunsScanner, too.

(In reply to Scott Johnson (:jwir3) from comment #14) > Created attachment 606363 [details] [diff] [review] > b708187 (v1) - WIP > > I am adding a check inside of ContinueTextRunAcrossFrames() to not continue > a text run if font inflation is different between two frames. This would > normally happen in a case like that of attachment 606358 [details]. So why is this case relevant, given that the line: return fontStyle1->mFont.BaseEquals(fontStyle2->mFont) && will already cause us to return false in such cases since the font sizes are different? > This only gets us part of the way there, though... if you load attachment > 606358 [details] with this patch applied and enable font inflation to 12 > emPerLine, it works as expected (i.e. the text does not extend outside of > frame boundaries, and the two words "Hello" and "Worldz" are inflated, but > at different levels). > > If you refresh, though, the text from "Worldz" grows larger, and extends > beyond its frame. This is happening because, somehow, mInflation inside of > the BuildTextRunsScanner is being set to almost double what it should be set > at, and thus inside of FlushFrames(), this is applied to the newly built > text run, resulting in a larger-than-expected font. Maybe the issue isn't that they're in the same text run but that BuildTextRunsScanner is carrying information that should have been for one text run across multiple text runs?

(In reply to Scott Johnson (:jwir3) from comment #15) > Created attachment 606753 [details] [diff] [review] > b708187 (v2) - WIP > > I think I've found a fix for this, but it requires us to not cache the > inflation inside of BuildTextRunsScanner. I'm not sure if recalculating font > size inflation from within BuildTextRunForFrame is too expensive. > > If it's not too expensive, then I will remove the mInflation member variable > within BuildTextRunsScanner, too. Yeah, this is definitely the right track. I think the changes to ContinueTextRunAcrossFrames aren't needed (see previous comment -- I collided with the new patch being attached). However, I think you should remove the code that's no longer needed -- this will also make sure that it's not being used incorrectly anywhere else. Also, this sort of thing: + if (presContext && presContext->PresShell()->IsReflowLocked()) { + fontInflation = + nsLayoutUtils::FontSizeInflationFor(firstFrame, + nsLayoutUtils::eInReflow); + } else { + fontInflation = + nsLayoutUtils::FontSizeInflationFor(firstFrame, + nsLayoutUtils::eNotInReflow); + } is better written by putting either nsLayoutUtils::eInReflow or eNotInReflow into a variable and then calling FontSizeInflationFor only once.

Comment on attachment 606753 [details] [diff] [review] b708187 (v2) - WIP see previous comment

I'm not 100% finished with the reftest yet, specifically it seems that in the inflated case, the word spacing between the two words "hello" and "worldz" is significantly larger than the spacing between the words on the reference test case. I tried adding the css property "word-spacing", but the two tests need different word spacing, which concerns me a bit. I'm still looking into this, but in the meantime, hopefully you can take a look at the actual code.

Fixed the reftest.

Comment on attachment 607404 [details] [diff] [review] b708187 (v4) >+ nsPresContext* presContext = firstFrame->PresContext(); >+ nsLayoutUtils::WidthDetermination widthDeter = nsLayoutUtils::eNotInReflow; >+ >+ if (presContext && presContext->PresShell()->IsReflowLocked()) { >+ widthDeter = nsLayoutUtils::eInReflow; >+ } A few things here. nsIFrame::PresContext() is guaranteed to return non-null. (The fact that there's no "Get" at the beginning of the name says so -- though there are also a few methods we haven't removed the "Get" from that we need to.) So don't null-check presContext. Also, you should add an assertion to make sure that the current inflation container is correct for the in-reflow case. In particular, you'd need to, #ifdef DEBUG, loop starting from firstFrame until you hit a frame for which nsLayoutUtils::IsContainerForFontSizeInflation returns true. When you hit that frame, you should add an NS_ASSERTION that what you found is equal to presContext->mCurrentInflationContainer. This makes sure that we don't do the eInReflow when we're reflowing some other subtree (since the code for the eInReflow case depends on state that's pushed/popped and that is thus specific to what's currently being reflowed). r=dbaron with that

(In reply to David Baron [:dbaron] from comment #21) > A few things here. Sure. I'll make these changes. > (The fact that there's no "Get" at the beginning of the name says > so -- though there are also a few methods we haven't removed the "Get" from > that we need to.) Oh, ok. I didn't know that the lack of the "Get" before an accessor function name indicated that it can't return null. I was unsure when a function can return null and when it can't, so this helps a lot to know. Thanks for the tip!

I was having an issue after adding the assertion mentioned in comment 21, so I modified the patch so that it waits to compute font inflation in the text run scanner, based on whether presContext->mCurrentInflationContainer is null. I think what was happening is that presContext->mCurrentInflationContainer wasn't getting set, because reflow for a given frame hadn't happened yet. This was, at times, setting off the assertion. I'm posting the new patch for review again, just as a sanity check that I didn't do something crazy.

Comment on attachment 608079 [details] [diff] [review] b708187 (v5) Oh, I see the problem, I think. Before this patch BuildTextRunsScanner had two variables related to inflation: mWhichTextRun and mInflation. When mWhichTextRun is nsTextFrame::eNotInflated then mInflation is 1.0. You should copy that logic into the new patch, by looking at mWhichTextRun (which you're not removing) when you decide what inflation to use. When it's eNotInflated you should just use 1.0; when it's eInflated you should call nsLayoutUtils::FontSizeInflationFor. Then you shouldn't need your new shouldComputeFontInflation variable. It's possible there's still a problem where the inflation container isn't right. But if there is once you've changed that, I think you'll just need to debug what it is.

(In reply to David Baron [:dbaron] from comment #24) > Comment on attachment 608079 [details] [diff] [review] > b708187 (v5) > > Oh, I see the problem, I think. > > Before this patch BuildTextRunsScanner had two variables related to > inflation: mWhichTextRun and mInflation. When mWhichTextRun is > nsTextFrame::eNotInflated then mInflation is 1.0. > > You should copy that logic into the new patch, by looking at mWhichTextRun > (which you're not removing) when you decide what inflation to use. When > it's eNotInflated you should just use 1.0; when it's eInflated you should > call nsLayoutUtils::FontSizeInflationFor. Then you shouldn't need your new > shouldComputeFontInflation variable. Bingo. That was the problem. Thanks for pointing that out, David, I appreciate it. I added the requested assertion now, and it passes as expected.

Comment on attachment 608131 [details] [diff] [review] b708187 (v6) > // Now build the textrun > nsTextFrame* firstFrame = mMappedFlows[0].mStartFrame; >+ nsPresContext* presContext = firstFrame->PresContext(); >+ nsLayoutUtils::WidthDetermination widthDeter = nsLayoutUtils::eNotInReflow; >+ >+ if (presContext->PresShell()->IsReflowLocked()) { >+ widthDeter = nsLayoutUtils::eInReflow; >+ } >+ >+#ifdef DEBUG >+ if (mWhichTextRun == nsTextFrame::eInflated && >+ widthDeter == nsLayoutUtils::eInReflow) { >+ // Make sure that the font inflation container is correct. >+ nsIFrame* inflationContainer = nsnull; >+ for (nsIFrame* f = firstFrame; f; f = f->GetParent()) { >+ if (nsLayoutUtils::IsContainerForFontSizeInflation(f)) { >+ inflationContainer = f; >+ break; >+ } >+ } >+ >+ NS_ASSERTION(inflationContainer == presContext->mCurrentInflationContainer, >+ "Current inflation container for text frame is wrong"); >+ } >+#endif >+ >+ float fontInflation = mWhichTextRun == nsTextFrame::eInflated ? >+ nsLayoutUtils::FontSizeInflationFor(firstFrame, widthDeter) : >+ 1.0f; I think it would be much clearer if you did: float fontInflation; if (mWhichTextRun == nsTextFrame::eNotInflated) { fontInflation = 1.0f; } else { ... PUT ALL THE REST OF THE CODE QUOTED ABOVE HERE... ... (but without the mWhichTextRun checks) ... } r=dbaron with that

As mentioned in the meeting yesterday afternoon, I'm seeing a few crashtest failures after pushing to try, related to this new assertion (see https://tbpl.mozilla.org/?tree=Try&rev=8de6cdc9eada ) After some debugging, I think these have to do with two separate CSS properties being set: 1) direction: RTL and 2) -moz-column-count/-moz-column-width. It seems, from my initial debugging, that presContext->mCurrentInflationContainer is being set to the proper inflation container for a given frame (we'll call this frame "X"), then it's being set in nsHTMLReflowState::InitConstraints to the inflation container for the next frame to be reflowed, and then (sometime later) the BuildTextRunsScanner is constructed for frame "X". Unfortunately, this expects that the inflation container is something different that what it actually is... it seems to have been advanced more quickly than expected. (See http://pastebin.mozilla.org/1532174 for a GDB example on layout/generic/crashtests/42774-1.html). I'm still digging into this to see what could be causing it, but I thought I would update the bug with rationale as to why this hasn't landed yet.

(In reply to Scott Johnson (:jwir3) from comment #27) > As mentioned in the meeting yesterday afternoon, I'm seeing a few crashtest > failures after pushing to try, related to this new assertion (see > https://tbpl.mozilla.org/?tree=Try&rev=8de6cdc9eada ) > > After some debugging, I think these have to do with two separate CSS > properties being set: 1) direction: RTL and 2) > -moz-column-count/-moz-column-width. It seems, from my initial debugging, > that presContext->mCurrentInflationContainer is being set to the proper > inflation container for a given frame (we'll call this frame "X"), then it's > being set in nsHTMLReflowState::InitConstraints to the inflation container > for the next frame to be reflowed, and then (sometime later) the > BuildTextRunsScanner is constructed for frame "X". Unfortunately, this > expects that the inflation container is something different that what it > actually is... it seems to have been advanced more quickly than expected. > (See http://pastebin.mozilla.org/1532174 for a GDB example on > layout/generic/crashtests/42774-1.html). > > I'm still digging into this to see what could be causing it, but I thought I > would update the bug with rationale as to why this hasn't landed yet. Would the assertion: + NS_ASSERTION(inflationContainer == presContext->mCurrentInflationContainer, + "Current inflation container for text frame is wrong"); stop firing if you make it: inflationContainer->GetFirstInFlow() == presContext->mCurrentInflationContainer->GetFirstInFlow() (If so, does it still keep working if you remove one of the GetFirstInFlow calls... perhaps the second one first?) (I realized this after writing similar things while working on bug 706193.)

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/dbaa67729172

Verified fixed on 3-29-2012 nightly fennec. Changed all the font sizes, and titles dont overflow anymore! Galaxy Nexus, ,4.0.2 and Droid Pro, 2.3.3.

Comment on attachment 608131 [details] [diff] [review] b708187 (v6) [Approval Request Comment] Regression caused by (bug #): font-inflation User impact if declined: readability on mobile will be impaired for some sights when using font-inflation Testing completed (on m-c, etc.): mozilla-central Risk to taking this patch (and alternatives if risky): fairly low, but there is an assertion that was added in this patch that apparently fails at times, when in debug mode (bug 740199) String changes made by this patch: none

Comment on attachment 608131 [details] [diff] [review] b708187 (v6) (In reply to Scott Johnson (:jwir3) from comment #32) > Risk to taking this patch (and alternatives if risky): fairly low, but there > is an assertion that was added in this patch that apparently fails at times, > when in debug mode (bug 740199) Although this touches shared code, my understanding is that font inflation is only utilized in the mobile product. If that's not the case, please shoot me an email before landing on Aurora 13.

Pushed to aurora in a 4-step process: 1) Accidentally pushed to beta instead of aurora: https://hg.mozilla.org/releases/mozilla-beta/rev/c0e54be58660 2) *face palm* 3) Backed out of beta: https://hg.mozilla.org/releases/mozilla-beta/rev/b2c909d51375 4) Correctly pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/a923cadf7b8d

And it looks like this didn't stick, and had to be backed out due to bustage: https://hg.mozilla.org/releases/mozilla-aurora/rev/51b7a6faae5d

So, it looks like something in bug 704179 is required in this bug. The problem I'm seeing is that it doesn't look like nsPresShell::IsReflowLocked() is returning true when in reflow for an nsSVGForeignObjectFrame, as is the case when loading layout/reftests/bugs/490177-1.svg. This has been fixed (or appears to be fixed) within the patches for bug 704179, but since this is a 130K patch, it's probably not likely it'll be able to be backported to aurora. I'm in the process of digging through these patches to determine exactly what fixed this and causes it not to fail in debug mode with the patches for that bug.

In the previous comment, I meant "bug 734079" when I accidentally said "bug 704179".

Also, here is a stack trace of the crash I am seeing on aurora (tbpl for some reason has a bad stack trace, so I ran it locally): https://etherpad.mozilla.org/jM7raZTXnU