627842 - (font-inflation) Allow minimum font size based on size of frame

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Benjamin Stover (:stechz)]

This is for readable text in Fennec. Enforcing a minimum font size often breaks page layouts, but what if we only enforce minimum sizes for text that has line breaks and base the font size on the width of the block that contains the text? This would be much less aggressive, and would allow minimum font sizes that, while too small to be read zoomed out, work just fine once you are zoomed in to the text (without having to pan back and forth).

Comment 1

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

"Text with line breaks" --- you mean white-space:normal and pre-wrap?

Would you need to also check that the containing block can expand in height somehow?

Comment 2

[Benjamin Stover (:stechz)]

(In reply to comment #1)
> "Text with line breaks" --- you mean white-space:normal and pre-wrap?

Sort of, but I mean any text that wraps. For instance, a white-space: normal <div> with a small bit of text that doesn't wrap would not be affected.

> Would you need to also check that the containing block can expand in height
> somehow?

That would be a good check, though I don't know how necessary it would be in practice.

--

I tried changing the font's size in GetFontMetricsForFrame, but this seems to happen after all of the text layout had been determined (line height is computed much earlier, and any links or spans within the text run would scrunch together).

The only approach I've come up with is rather hacky: change the actual style of the content node to use a different font size at a spot where we know the size of the text frame, and let the reflow code do its thing. We'd have to watch out for any blocks defined in "em" units.

Ideally, I hope there's some place between knowing the size of the text frame and actually performing the text run where I could change the font size and then let the frame recalculate its height and do the text run.

Roc: do you have any idea where to start on something like this?

Comment 3

[Matt Brubeck (:mbrubeck)]

This is our only proposal so far to fix problems with our current textZoom-based reflow implementation (see bug 611555 and dependencies).  Nominating for blocking-fennec.

Comment 4

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

(In reply to comment #2)
> Sort of, but I mean any text that wraps. For instance, a white-space: normal
> <div> with a small bit of text that doesn't wrap would not be affected.

Hmm, that adds complexity...

> I tried changing the font's size in GetFontMetricsForFrame, but this seems to
> happen after all of the text layout had been determined (line height is
> computed much earlier, and any links or spans within the text run would scrunch
> together).

GetFontMetricsForFrame is used for textrun construction, so that should work in terms of getting the right horizontal layout for spans. I would want to understand why it does not.

For vertical dimensions, the line-height should not be fixed until we've reflowed all the text, unless you're using the line-height property with values other than 'normal'. So you would want to check 'line-height' on the containing block and not do your magic resizing if line-height is other than 'normal' (the default).

> Roc: do you have any idea where to start on something like this?

GetFontMetricsForFrame was the right place to start.

You'll need to get dynamic changes right. When your text-zoom is enabled, nsBlockFrame::PrepareResizeReflow will need tryAndSkipLines to be false always.

Regarding your first comment, you can check mLineBreaker in BuildTextRunForFrames and pass something down to GetFontMetricsForFrame to let it know if there's a break opportunity in this text. You might get weird results though, e.g.
<b>He</b>llo Kitty
The first text frame won't know about any break opportunities, so it will be normal-sized, bu the second text frame will know about break opportunities so it can be resized. That would look terrible.

Perhaps you need to decide in nsBlockFrame::Reflow whether you're going to enable text resizing for the entire block or not. You could instantiate your own nsLineBreaker and gather all the text content and look for break opportunities, although that's going to mean duplicating some code for text layout :-(.

Hmm, another option would be to do two-pass reflow. How about this: optimistically assume that text resizing is OK, reflow the block with text resizing enabled, and then detect if there's overflow. If there is, disable text resizing and reflow again. No messing around with line breaking etc. How does that sound? Making this work with intrinsic width calculations (for table cells and floats) sounds hard ... can you get away with disabling text resizing in those situations?

Comment 5

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

BTW is this really a blocking bug? Because this is a major and potentially destabilizing new feature. Is this really a good place in the Fennec schedule to be doing this?

Comment 6

[Benjamin Stover (:stechz)]

After talking it out and thinking it over last night, I'm not the right person for this patch. Putting this bug back to unassigned.

Comment 7

[Doug Turner (:dougt)]

what roc said.

Comment 8

[Stuart Parmenter]

Would have been great to get this in sooner, but is something that we need to really get text to be readable.

Comment 9

[Stuart Parmenter]

We need to get this in as soon as possible so that we can get people testing it.

Comment 10

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

The last paragraph of comment #4, in more detail:
1) Add a state bit on nsBlockFrame to indicate whether "text resizing" is enabled for that block.
2) nsTextFrame::Reflow checks that bit on the nearest ancestor block. When it's set, allow GetFontMetricsForFrame to increase the text size to some minimum size. Record in the textframe state bits whether we have increased the text size.
3) [optional] when that bit is set, treat CSS line-height as "normal" no matter what the style says
4) When nsBlockFrame::Reflow finishes reflowing its lines, check whether the block contents overflow the block's content-box. (We may also need to check the content-boxes of ancestor frames here ... this needs more thought.) If there is overflow, check the children to see if any had their text size increased by text resizing. If they did, disable text resizing, dirty all lines (possibly just the lines containing text whose size was increased) and reflow the block contents again.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP m-c patch

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP m-b patch

Comment 13

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

(In reply to comment #11)
> Created attachment 514264 [details] [diff] [review]
> WIP m-c patch

Looks OK but doesn't do any of the hard layout stuff obviously :-)

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

right, this is cheating and doing the hard stuff on double tap in the front end

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Roc,changing the font size in GetFontMetricsForFrame doesn't seem to have any effect on the eventual layout of the page. I've mucked around a bit experimenting with other approaches, mostly using the previous patch and setting the min font size on the nsPresContext at various points in the layout/reflow of a block frame. That does have an effect on the layout, but winds up not doing what we want and most the time results in an eventual crash (typically when dealing with border images).

Comment 16

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

Attachment: hack patch

This patch hacks GetFontMetricsForFrame to use a font size of 100px. It definitely affects the layout of the UI, and if I load

<p>Hello <b>Kitty</b>
<p>Hello

it gets laid out "properly".

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP m-c patch

roc, thanks for the pointer. 

This patch sets a min font size based on the width of the block frame or first ancestor block frame. The value of that minimum is based on a scale factor saved in a pref.

With this, the font sizes are changing, but the layouts get out of whack (especially for larger font sizes). It looks like we may not be measuring line height correctly.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP m-b patch

this sets the pref mentioned for the m-c patch based on the window width. The scale factor was chosen unscientifically by trial and error.

Comment 19

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

+        nsStylePosition* pos = new (pc) nsStylePosition();
+        //const nsStylePosition *stylePos = bFrame->GetStylePosition();
+        nscoord width;
+        GetIntrinsicCoord(pos->mWidth, rc, bFrame, PROP_WIDTH, width);

Why create a new nsStylePosition here?

On what testcases are you not getting good line heights? I mentioned above that any pages that set CSS line-height to something other than 'normal' are not compatible with this approach.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

I'm using the nsStylePositon to get an nsStyleCoord to pass into GetIntrinsicCoord(). Calling bFrame->GetPrefWidth(rc) crashes for some unknown reason

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP m-c patch

still just mucking with this unscientifically, just checking if the parent is a block rather than searching for first block ancestor seems to work better

Comment 22

[Boris Zbarsky [:bzbarsky]]

> I'm using the nsStylePositon to get an nsStyleCoord

Uh... but that's just passing in the initial value (so "auto") given your code.  What's the point of that?  If you really just want to pass auto, pass auto explicitly instead of creating (and leaking, too) an nsStyleCoord.

> Calling bFrame->GetPrefWidth(rc) crashes for some unknown reason

Unknown in what sense?  No stack?

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #22)
> > I'm using the nsStylePositon to get an nsStyleCoord
> 
> Uh... but that's just passing in the initial value (so "auto") given your code.
>  What's the point of that?  If you really just want to pass auto, pass auto
> explicitly instead of creating (and leaking, too) an nsStyleCoord.

that sounds better, thanks

> > Calling bFrame->GetPrefWidth(rc) crashes for some unknown reason
> 
> Unknown in what sense?  No stack?

there's a stack and its a seg fault, but its not clear to me why. The top of the stack is bFrame->GetPRefWidth(rc) but bFrame and rc are both valid

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP m-c patch

Comment 25

[Boris Zbarsky [:bzbarsky]]

Can you attach the stack here?

Comment 26

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

Comment on attachment 515638 [details] [diff] [review]
WIP m-c patch

A few notes based on skimming through this patch:

 * nsHTMLReflowState doesn't need a new mFrame; it already has frame

 * CalcLineHeight and ComputeLineHeight should have the style context parameter replaced with a frame parameter, rather than the frame parameter added.  However, you'd need something else for nsComputedDOMStyle, into which this introduces a null-dereference crash, and which shouldn't honor this pref anyway.

 * the tree changes break tree styling pseudo-classes; GetFontMetricsForStyleContext should stay for the SVG and the tree code


That said, I think it's crazy to be trying this at this stage of a release:  this is something that's going to require user feedback both as to whether the algorithm is good and as to whether it doesn't break too much Web content.

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #26)
> That said, I think it's crazy to be trying this at this stage of a release: 
> this is something that's going to require user feedback both as to whether the
> algorithm is good and as to whether it doesn't break too much Web content.

I couldn't agree more

Comment 28

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

Also, I don't see what prevents this from causing infinite-growing loops when handling dynamic changes in intrinsically-sized content.

Comment 29

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

resetting this from + to ? to trigger reevaluation based on above comments

Comment 30

[Stuart Parmenter]

We really have to have this -- text is simply too small to read in most cases and we can't ship without it

Comment 31

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

(In reply to comment #0)
> This is for readable text in Fennec. Enforcing a minimum font size often breaks
> page layouts, but what if we only enforce minimum sizes for text that has line
> breaks and base the font size on the width of the block that contains the text?
> This would be much less aggressive, and would allow minimum font sizes that,
> while too small to be read zoomed out, work just fine once you are zoomed in to
> the text (without having to pan back and forth).

For what it's worth, this is still substantially more aggressive than what mobile Safari does.  It seems like mobile Safari does scaling up of text, dependent on the width of the block, when there are multiple (not necessarily consecutive) blocks within the same normal flow that have the same font-size and font-family.  Or something like that -- there are probably a bunch more conditions.  It's not a minimum font size, since the specified font size still matters.

But that's also substantially more complicated to implement.


So maybe we could try this -- but there's certainly a risk that it's going to break pages given that it's more aggressive than what others are doing.

Comment 32

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

(In reply to comment #28)
> Also, I don't see what prevents this from causing infinite-growing loops when
> handling dynamic changes in intrinsically-sized content.

It looks like what mobile Safari does is that it does shrink-wrapping width calculations at the non-grown font size, and then grows the font size based on the resulting width.  That's involved enough and breaks enough of our invariants that there's no way it's going to happen for this release (never mind the fact that I'm mostly unavailable the next week and a half).

Comment 33

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

(In reply to comment #32)
> (In reply to comment #28)
> > Also, I don't see what prevents this from causing infinite-growing loops when
> > handling dynamic changes in intrinsically-sized content.
> 
> It looks like what mobile Safari does is that it does shrink-wrapping width
> calculations at the non-grown font size, and then grows the font size based on
> the resulting width.  That's involved enough and breaks enough of our
> invariants that there's no way it's going to happen for this release (never
> mind the fact that I'm mostly unavailable the next week and a half).

I'm starting to think that doing the growing based on the container's width without this is probably hard, and it would be safer to base the growing on the amount of text (within a given BFC).  That complicates incremental layout, though.  (Either way is probably a substantial performance penalty.)

Comment 34

[Stuart Parmenter]

Could we do this as some sort of external pass post-reflow?  I realize it wouldn't be as efficient, but might get us what we want?

Comment 35

[Stuart Parmenter]

minusing for release -- would really like to get this in a short follow up (4.0.1?) if at all possible.  We're going to work on a compromise user pref to do some text reflow in the meantime

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

dbaron, I was told you were looking at this. If that's not true, please let me know

Comment 37

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

Attachment: work-in-progress part 3: apply font size inflation to text

I wanted to get your thoughts on this part of the patch before going too far down this path, which turns out to be a little messy.

The basic idea of what I'm planning is that the font size inflation has to be applied directly by the frames since it needs knowledge of container widths, so it has to happen both after style data computation and after intrinsic size calculation (which the text frame does part of).

My current approach is to do the reflow side of this inflation manually, and the painting side via a scale in the graphics context, though I'm open to other suggestions.  The frame's rect obviously has to include the scale, and I'm currently thinking mAscent should match the rect (though it could go either way).  It may be a little hard, and a source of further bugs, to disentangle which parts of the code want uninflated sizes and which parts (such as everything exposed to the outside of nsTextFrame) want inflated sizes.

I'm wondering if you have any thoughts on this approach or ideas to improve it.

If you're interested in the state of the other pieces, my full patch queue for this bug is:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4edb647579db/text-size-adjust
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4edb647579db/font-size-inflation-base
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4edb647579db/font-size-inflation-text
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4edb647579db/remove-unused-ctx-param
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4edb647579db/font-size-inflation-line-height

Comment 38

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

+  // REVIEW: He're we're assuming that drawing text with a transform
+  // scales it up by exactly the scale in the transform.  Given hinting,
+  // is that a safe assumption?  (It drastically simplifies this code,
+  // since we don't need two different text runs to implement font size
+  // inflation.)

Yeah, that's not a safe assumption and doing this will probably result in ugly text.

I think instead of trying to use the same textrun and scaling up during paint, we should compute an effective font size in Reflow() and do everything based on that effective font size, including generating a new text run if necessary. I think we should stop referring directly to mTextRun, and change EnsureTextRun to return a textrun as well as the iterator, and add a parameter to EnsureTextRun to indicate whether the requested textrun is the "normal" or "possibly-inflated" textrun. We should probably store the inflated textrun separately, if it's different from mTextRun, probably in a frame property (alongside the inflation factor). I would prefer to juggle two possibly-different textruns than to sometimes multiply or divide by the inflation factor.

Does that make sense?

Comment 39

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

Sounds reasonable, though I'm still wrapping my head around some of the text run caching code (which I think will need some changes for text frames to be associated with two different text runs which might expire at different times).

One difference, though:  my inclination would be that when the inflated and normal text runs are different, the inflated text run would be the mTextRun, and the un-inflated text run would be the odd one out (since it's only needed in GetMinWidth and GetPrefWidth, which may well never be called).

Comment 40

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

Makes sense.

Comment 41

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

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5da917ddb6b9/try-android/ has a test build based on the patches:

https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/723c04218e82/text-size-adjust
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/723c04218e82/constrained-height-bit
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/723c04218e82/font-size-inflation-base
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/723c04218e82/use-text-run-for-tab-width
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/723c04218e82/font-size-inflation-text
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/723c04218e82/remove-unused-ctx-param
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/723c04218e82/font-size-inflation-line-height

I haven't actually tried them out on a device yet, and I have no idea if the inflation factor is anywhere near reasonable (but it's a pref -- and the only one in about:config with "inflation" in the name -- the pref just gives the number of square characters per line that would be readable, with 0 disabling the inflation feature).  (I also want to try an alternative strategy for specifying the pref that might be less device-specific.)

There are a bunch of pretty obvious bugs (most obviously, with backgrounds/borders on inline elements and the size of text controls) that I want to work on today, then move on to implementing code to determine the inflation factor from the device size, and then look into further tuning of the heuristic.

Comment 42

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

updated try build at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4a1ca43eaef8/try-android/

Comment 43

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

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-376690d5e498/try-android/

Comment 44

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

(In reply to David Baron [:dbaron] from comment #43)
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 376690d5e498/try-android/

So I think it may be interesting for a small number of people to test this build.  There are two different preferences for enabling font size inflation; this build has it enabled with one of them, but it's more likely the other that will be more interesting (and I think it may even work as intended now, though verifying that will require a little more testing across different devices).  The preferences are described here:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/95399627c734/font-size-inflation-base#l346
interesting values for "font.size.inflation.emPerInch" are probably in the 8-10 range; interesting values for "font.size.inflation.emPerLine" are probably in the 20-30 range.  (That build has emPerLine set to a default of 30; setting emPerInch is probably a more interesting pref default, assuming it works.)

I'd note that I haven't figured out what the right thing to do with textfields is yet -- though I'm quite sure the current behavior isn't it.

Comment 45

[Martijn Wargers (dead)]

I've tested the build from comment 44 a little bit on my LG Optimus Black.
http://people.mozilla.com/~mwargers/tests/627842_minfontsize.htm
I noticed that with this build, the font in the first block gets inflated, but not with the second, small width block. I think that is expected with this build.

I think I noticed a problem with this build on http://news.google.com/m/news?ned=us
See screenshot:
http://people.mozilla.com/~mwargers/tests/627842_minfontsize.png
Certain links (the green links and the section links at the bottom) get inflated with this build.

Comment 46

[Martijn Wargers (dead)]

The "Search" placeholder text in the filter input is too big with this build on the LG Optimus Black: http://people.mozilla.com/~mwargers/tests/627842_minfontsize_aboutconfig.png

Comment 47

[Martijn Wargers (dead)]

Ok, I guess that is a problem with text fields in general as dbaron already noted in comment 44:
http://people.mozilla.org/~mwargers/tests/627842_minfontsize_inputs.htm

Comment 48

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

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #45)
> I've tested the build from comment 44 a little bit on my LG Optimus Black.
> http://people.mozilla.com/~mwargers/tests/627842_minfontsize.htm
> I noticed that with this build, the font in the first block gets inflated,
> but not with the second, small width block. I think that is expected with
> this build.
> 
> I think I noticed a problem with this build on
> http://news.google.com/m/news?ned=us
> See screenshot:
> http://people.mozilla.com/~mwargers/tests/627842_minfontsize.png
> Certain links (the green links and the section links at the bottom) get
> inflated with this build.

So this is an issue that I knew about but didn't realize would be as big of a problem:  that the code doesn't currently respond correctly to page resizes.  Since text size now depends on the block width, resizing a block needs to force reflow of all of its descendants.  I have a FIXME in my queue to do this.

The reason I think it's a problem is that we initially load the page at the default viewport width, then detect something (not sure what, though maybe it serves a <meta ... viewport> to Fennec but not desktop Firefox) that makes us resize the viewport narrower.

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #46)
> The "Search" placeholder text in the filter input is too big with this build
> on the LG Optimus Black:
> http://people.mozilla.com/~mwargers/tests/627842_minfontsize_aboutconfig.png

This is the issue with text inputs that I mentioned.

Comment 49

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

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bc948761af9f/try-android/ will soon have another round of builds with those issues fixed.  This will be a build based on:
https://hg.mozilla.org/mozilla-central/rev/3c684b4b8f68
plus the patches:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/text-size-adjust
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/constrained-height-bit
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/font-size-inflation-base
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/resize-cause-dirty
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/get-font-metrics-inflation
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/use-text-run-for-tab-width
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/font-size-inflation-text
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/remove-unused-ctx-param
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/font-size-inflation-line-height
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/font-size-inflation-reftests
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/font-size-inflation-inline
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/font-size-inflation-bullets
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/b599ff3b937b/font-size-inflation-other

Comment 50

[Martijn Wargers (dead)]

With the build from comment 49, the font size of about:home seems to be inflated, see:
http://people.mozilla.org/~mwargers/tests/627842_minfontsize/abouthome_minfontsizebuild.png
and compare with Aurora on the same LG Optimus Black phone:
http://people.mozilla.org/~mwargers/tests/627842_minfontsize/abouthome_aurora.png

Also, on http://news.google.com/m/news?ned=us , the fonts look bigger with this special build:
http://people.mozilla.org/~mwargers/tests/627842_minfontsize/googlenewsm_minfontsizebuild.png
Compare to the latest Aurora build:
http://people.mozilla.org/~mwargers/tests/627842_minfontsize/googlenewsm_aurora.png

Comment 51

[Martijn Wargers (dead)]

Also compare this build with a regular build on the file:/// page.
http://people.mozilla.org/~mwargers/tests/627842_minfontsize/file_minfontsizebuild.png
http://people.mozilla.org/~mwargers/tests/627842_minfontsize/file_minfontsizebuild.png

Also seeing weird things on http://people.mozilla.org/~mwargers/tests/627842_minfontsize_inputs.htm
The text inputs seem to get bigger now, but the text is small in there, like in normal builds. Suddenly, when I type in the text inputs, the text gets huge.

Comment 52

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

Attachment: Add support for -moz-text-size-adjust CSS property. (patch 1)

Comment 53

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

Attachment: Set an NS_FRAME_IN_CONSTRAINED_HEIGHT state bit on frames that are in a constrained space. (, patch 2)

Comment 54

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

Attachment: When reflowing a frame (such as text controls) that jumps from HTML layout into XUL layout and then jumps back to HTML on the child frame, link the parent reflow state chain correctly. (, patch 3)

Comment 55

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

Attachment: Implement computation of font size inflation for improved readibility of text on mobile devices. (, patch 4)

Comment 56

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

Attachment: When font size inflation is enabled, horizontal resizes of blocks must cause a full dirty reflow. (, patch 5)

Comment 57

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

Attachment: Add inflation parameter to nsLayoutUtils::GetFontMetricsFor* methods. (, patch 6)

Comment 58

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

Attachment: Use the text run's font group for the tab width instead of recomputing it from the frame. (, patch 7)

Comment 59

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

Attachment: Pass block's reflow state to nsTextFrame::UnionAdditionalOverflow. (, patch 8)

Comment 60

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

Attachment: Apply font size inflation to text. (, patch 9)

Comment 61

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

Attachment: Remove the unused context parameter to MeasureCharClippedText. (, patch 10)

Comment 62

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

Attachment: Apply font size inflation to line heights. (, patch 11)

Comment 63

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

Attachment: Apply font size inflation to heights of inlines. (, patch 12)

Comment 64

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

Attachment: Apply font size inflation to list bullets. (, patch 13)

Comment 65

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

Attachment: Pass nsFontMetrics to the GetEllipsis function rather than computing them again. (, patch 14)

Comment 66

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

Attachment: Make other users of font metrics (other than MathML and XUL) honor font size inflation. (, patch 15)

Comment 67

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

Attachment: Add reftest harness for testing font inflation and add reftests for basic features. (, patch 16)

Comment 68

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

Comment on attachment 569862 [details] [diff] [review]
Implement computation of font size inflation for improved readibility of text on mobile devices.  (, patch 4)

Review of attachment 569862 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +4413,5 @@
> +    byLine = aContainerWidth / sFontSizeInflationEmPerLine;
> +  }
> +  if (sFontSizeInflationMinTwips != 0) {
> +    // REVIEW: Is this giving us app units and sizes *not* counting
> +    // viewport scaling?

Yes. You probably should include nsIPresShell::GetResolution here.

There's also a question of what, if anything, we should do about CSS transforms. I think probably content inside a CSS transform should not get font-size inflation.

@@ +4431,5 @@
> +// size of the same text; the aStyleFontSize parameter here should
> +// always be a font size, and never a line height.
> +/* static */ float
> +nsLayoutUtils::FontSizeInflationInner(const nsIFrame *aFrame,
> +                                      nscoord aMinFontSize)

No aStyleFontSize parameter here...

@@ +4491,5 @@
> +   * count as a container, so that, for example, an indented quotation
> +   * didn't end up with a smaller font size.  However, it's hard to
> +   * distinguish these situations where we really do want the indented
> +   * thing to count as a container, so we don't try, and blocks are
> +   * always containers.

It seems to me that the ideal thing here would be to have anything that's not in a line-of-inlines be a container for font-size inflation. What do you think?

::: modules/libpref/src/init/all.js
@@ +1555,5 @@
> + * A value greater than zero enables font size inflation for
> + * pan-and-zoom UIs, so that if a block's width is scaled to match the
> + * device's width, the fonts in a block are at least the font size
> + * given.  The value given is in twips, i.e., 1/20 of a point, or 1/1440
> + * of an inch.

I'm not sure twips is the right measure here. Why did you choose it? Seems to me we should use something that corresponds closer to viewing angle ... CSS pixels (modulo scaling)?

Comment 69

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

Comment on attachment 569867 [details] [diff] [review]
Apply font size inflation to text.  (, patch 9)

Review of attachment 569867 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsLineLayout.h
@@ +370,5 @@
>     */
>    nsIFrame* GetLineContainerFrame() const { return mBlockReflowState->frame; }
> +  const nsHTMLReflowState* GetLineContainerRS() const {
> +    return mBlockReflowState;
> +  }

This hunk should go in the previous patch.

::: layout/generic/nsTextFrame.h
@@ +388,5 @@
>                                       PRUint32* aFlowEndInTextRun = nsnull);
> +  // Since we can't reference |this| in default arguments:
> +  gfxSkipCharsIterator EnsureTextRun(bool aInflated) {
> +    return EnsureTextRun(aInflated,
> +                         aInflated ? mFontSizeInflation : 1.0f);

Boolean parameters suck ... Here probably we could have EnsureTextRun, EnsureTextRunNoInflation? And use an enum for the cases where we want to pass the text run type around as a parameter?

@@ +417,5 @@
> +  void ClearTextRun(nsTextFrame* aStartContinuation, bool aInflated);
> +
> +  void ClearTextRuns() {
> +    ClearTextRun(nsnull, true);
> +    ClearTextRun(nsnull, false);

Pass a mask to ClearTextRun so we don't have to do two passes?

@@ +455,5 @@
>    // so if we create a new continuation, this is where that continuation should
>    // start.
>    PRInt32     mContentLengthHint;
>    nscoord     mAscent;
> +  float       mFontSizeInflation;

I think this should go in a frame property, with a state bit to indicate its presence.

Comment 70

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

Comment on attachment 569871 [details] [diff] [review]
Apply font size inflation to list bullets.  (, patch 13)

Review of attachment 569871 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBulletFrame.h
@@ +119,5 @@
>  
>    nsSize mIntrinsicSize;
>    nsSize mComputedSize;
>    PRInt32 mOrdinal;
> +  float mFontSizeInflation;

Maybe this should be a property as well ... the same property as for text frames?

Comment 71

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

Comment on attachment 569873 [details] [diff] [review]
Make other users of font metrics (other than MathML and XUL) honor font size inflation.  (, patch 15)

Review of attachment 569873 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +974,5 @@
>    // Set font and color
>    aRenderingContext.SetColor(GetStyleColor()->mColor);
>    nsRefPtr<nsFontMetrics> fm;
> +  nsLayoutUtils::GetFontMetricsForFrame(this, getter_AddRefs(fm),
> +    nsLayoutUtils::FontSizeInflationFor(this));

Maybe FontSizeInflationFor(nsIFrame*) should assert that the frame isn't being reflowed?

Comment 72

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

So if we use uninflated text sizes to determine intrinsic widths, then inflate, doesn't that mean a float or table cell could easily end up too small for its text content? Seems like maybe a container's inflation factor should have an upper bound of the container width divided by its intrinsic-min-width?

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 569858 [details] [diff] [review]
Add support for -moz-text-size-adjust CSS property.  (patch 1)

r=me

Comment 74

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 569863 [details] [diff] [review]
When font size inflation is enabled, horizontal resizes of blocks must cause a full dirty reflow.  (, patch 5)

>other than InitResizeFlags and nsFrame::Box

nsFrame::BoxReflow.

>down to the cell, and once we hit the cell we'll set the code we've

"we'll hit the code"?  Or some other verb?  I have no idea what "set" means there.

The code changes seem ok, but can the text-size-adjust style lead to sitations where font size inflation is enabled on the prescontext but the width change doesn't trigger it?  If so, maybe a followup bug on detecting those cases and skipping the dirty reflow in them?

r=me with the above

Comment 75

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #72)
> So if we use uninflated text sizes to determine intrinsic widths, then
> inflate, doesn't that mean a float or table cell could easily end up too
> small for its text content? Seems like maybe a container's inflation factor
> should have an upper bound of the container width divided by its
> intrinsic-min-width?

The whole point of the complexity here is that the inflation is not affecting intrinsic widths, for exactly that reason.

Comment 76

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

Suppose I have a block which is sized at its intrinsic-min-width ... say it has a 100-character word, 5px per character, so 500px wide. The user zooms that block to fill the screen. Say the screen is 250px wide. So we inflate the font size by some amount. Now that word doesn't fit, it'll overflow.

Maybe that's OK; I guess it can only happen when the content that needs to go on one line is just too long to be readable and fit on the screen, in which case letting it inflate to overflow is reasonable.

Comment 77

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

Forgot to mention earlier:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e0e833c5f6b7/try-android/
has builds based on the attached patches (on top of
https://hg.mozilla.org/mozilla-central/rev/767693e248aa ).

Comment 78

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

Responses to all of roc's review comments:

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68)
> Comment on attachment 569862 [details] [diff] [review] [diff] [details] [review]
> Implement computation of font size inflation for improved readibility of
> text on mobile devices.  (, patch 4)
> 
> Review of attachment 569862 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +4413,5 @@
> > +    byLine = aContainerWidth / sFontSizeInflationEmPerLine;
> > +  }
> > +  if (sFontSizeInflationMinTwips != 0) {
> > +    // REVIEW: Is this giving us app units and sizes *not* counting
> > +    // viewport scaling?
> 
> Yes. You probably should include nsIPresShell::GetResolution here.

I'm not sure if I should -- when does that get set to something other than 1.0?  In particular, if it gets set during panning/zooming on mobile, I don't want it.

> There's also a question of what, if anything, we should do about CSS
> transforms. I think probably content inside a CSS transform should not get
> font-size inflation.

Seems reasonable.  Do we have an efficient way to find out if a frame or one of its ancestors has a transform applied, or should I add a new one?  (If so, do you think it's a bad idea to turn the state bit I added into something inflation-specific, or is it worth having two bits that might have use elsewhere?)

> @@ +4431,5 @@
> > +// size of the same text; the aStyleFontSize parameter here should
> > +// always be a font size, and never a line height.
> > +/* static */ float
> > +nsLayoutUtils::FontSizeInflationInner(const nsIFrame *aFrame,
> > +                                      nscoord aMinFontSize)
> 
> No aStyleFontSize parameter here...

Oops; that was from an earlier version; I'll remove the comment.

> @@ +4491,5 @@
> > +   * count as a container, so that, for example, an indented quotation
> > +   * didn't end up with a smaller font size.  However, it's hard to
> > +   * distinguish these situations where we really do want the indented
> > +   * thing to count as a container, so we don't try, and blocks are
> > +   * always containers.
> 
> It seems to me that the ideal thing here would be to have anything that's
> not in a line-of-inlines be a container for font-size inflation. What do you
> think?

I think that's essentially what I've done -- although inline-blocks at similar things (but not form controls) establish their own containers.  What change were you suggesting?


> ::: modules/libpref/src/init/all.js
> @@ +1555,5 @@
> > + * A value greater than zero enables font size inflation for
> > + * pan-and-zoom UIs, so that if a block's width is scaled to match the
> > + * device's width, the fonts in a block are at least the font size
> > + * given.  The value given is in twips, i.e., 1/20 of a point, or 1/1440
> > + * of an inch.
> 
> I'm not sure twips is the right measure here. Why did you choose it? Seems
> to me we should use something that corresponds closer to viewing angle ...
> CSS pixels (modulo scaling)?

It's possible that some way to attempt to measure viewing angle would make sense at the very small end of devices, but I think beyond that devices are likely to be held at a comfortable arm's length -- I really don't think we should be assuming that a user will be holding a 8" tablet at twice the distance as a 4" phone -- I think it's far more likely they'll be close to the same distance.

I chose twips rather than points because I thought people might want fractions of a point, and we don't have float preferences.


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #71)
> Comment on attachment 569873 [details] [diff] [review] [diff] [details] [review]
> Make other users of font metrics (other than MathML and XUL) honor font size
> inflation.  (, patch 15)
> 
> Review of attachment 569873 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsImageFrame.cpp
> @@ +974,5 @@
> >    // Set font and color
> >    aRenderingContext.SetColor(GetStyleColor()->mColor);
> >    nsRefPtr<nsFontMetrics> fm;
> > +  nsLayoutUtils::GetFontMetricsForFrame(this, getter_AddRefs(fm),
> > +    nsLayoutUtils::FontSizeInflationFor(this));
> 
> Maybe FontSizeInflationFor(nsIFrame*) should assert that the frame isn't
> being reflowed?

It does, with some exceptions for XUL boxes (where that's ok, since size is set at the start of reflow).

Comment 79

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

(In reply to Boris Zbarsky (:bz) from comment #74)
> Comment on attachment 569863 [details] [diff] [review] [diff] [details] [review]
> When font size inflation is enabled, horizontal resizes of blocks must cause
> a full dirty reflow.  (, patch 5)
> 
> >other than InitResizeFlags and nsFrame::Box
> 
> nsFrame::BoxReflow.

right

> >down to the cell, and once we hit the cell we'll set the code we've
> 
> "we'll hit the code"?  Or some other verb?  I have no idea what "set" means
> there.

Yeah.  Or maybe "once we *reach* the cell we'll *hit* the code we've added here".

> The code changes seem ok, but can the text-size-adjust style lead to
> sitations where font size inflation is enabled on the prescontext but the
> width change doesn't trigger it?  If so, maybe a followup bug on detecting
> those cases and skipping the dirty reflow in them?

I don't immediately see how, since there could be a descendant with the opposite text-size-adjust style that does have inflation.

Comment 80

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #76)
> Suppose I have a block which is sized at its intrinsic-min-width ... say it
> has a 100-character word, 5px per character, so 500px wide. The user zooms
> that block to fill the screen. Say the screen is 250px wide. So we inflate
> the font size by some amount. Now that word doesn't fit, it'll overflow.
> 
> Maybe that's OK; I guess it can only happen when the content that needs to
> go on one line is just too long to be readable and fit on the screen, in
> which case letting it inflate to overflow is reasonable.

I think it's ok; I also don't see an alternative.  If we want inflation to be a function of width, then width can't be a function of inflation.

Comment 81

[Boris Zbarsky [:bzbarsky]]

> Or maybe "once we *reach* the cell we'll *hit* the code we've added here".

Sounds good.

Comment 82

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

(In reply to David Baron [:dbaron] from comment #78)
> I'm not sure if I should -- when does that get set to something other than
> 1.0?  In particular, if it gets set during panning/zooming on mobile, I
> don't want it.

It gets set during zooming, but don't you want the "visual" font-size here, which is affected by zooming?

> > There's also a question of what, if anything, we should do about CSS
> > transforms. I think probably content inside a CSS transform should not get
> > font-size inflation.
> 
> Seems reasonable.  Do we have an efficient way to find out if a frame or one
> of its ancestors has a transform applied, or should I add a new one?  (If
> so, do you think it's a bad idea to turn the state bit I added into
> something inflation-specific, or is it worth having two bits that might have
> use elsewhere?)

I think you should probably add a new state bit.

You can probably ignore translation-only transforms.

> > @@ +4491,5 @@
> > > +   * count as a container, so that, for example, an indented quotation
> > > +   * didn't end up with a smaller font size.  However, it's hard to
> > > +   * distinguish these situations where we really do want the indented
> > > +   * thing to count as a container, so we don't try, and blocks are
> > > +   * always containers.bringing
> > 
> > It seems to me that the ideal thing here would be to have anything that's
> > not in a line-of-inlines be a container for font-size inflation. What do you
> > think?
> 
> I think that's essentially what I've done -- although inline-blocks at
> similar things (but not form controls) establish their own containers.  What
> change were you suggesting?

I think I was suggesting inline-block, inline-box and inline-table should not establish their own containers. Having them behave inconsistently with form elements makes me a little uncomfortable. Can you outline your reasoning for why they should establish their own containers?

> > ::: modules/libpref/src/init/all.js
> > @@ +1555,5 @@
> > > + * A value greater than zero enables font size inflation for
> > > + * pan-and-zoom UIs, so that if a block's width is scaled to match the
> > > + * device's width, the fonts in a block are at least the font size
> > > + * given.  The value given is in twips, i.e., 1/20 of a point, or 1/1440
> > > + * of an inch.
> > 
> > I'm not sure twips is the right measure here. Why did you choose it? Seems
> > to me we should use something that corresponds closer to viewing angle ...
> > CSS pixels (modulo scaling)?
> 
> It's possible that some way to attempt to measure viewing angle would make
> sense at the very small end of devices, but I think beyond that devices are
> likely to be held at a comfortable arm's length -- I really don't think we
> should be assuming that a user will be holding a 8" tablet at twice the
> distance as a 4" phone -- I think it's far more likely they'll be close to
> the same distance.

I don't mean viewing angle of the entire screen, but angle subtended at the eye by the unit. CSS px is approximately a constant angle subtended at the eye, as you know. So if we want text to appear to be at least a certain size, CSS px seems like a suitable unit.

Comment 83

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

(In reply to David Baron [:dbaron] from comment #80)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #76)
> > Suppose I have a block which is sized at its intrinsic-min-width ... say it
> > has a 100-character word, 5px per character, so 500px wide. The user zooms
> > that block to fill the screen. Say the screen is 250px wide. So we inflate
> > the font size by some amount. Now that word doesn't fit, it'll overflow.
> > 
> > Maybe that's OK; I guess it can only happen when the content that needs to
> > go on one line is just too long to be readable and fit on the screen, in
> > which case letting it inflate to overflow is reasonable.
> 
> I think it's ok; I also don't see an alternative.  If we want inflation to
> be a function of width, then width can't be a function of inflation.

A possible alternative would be to choose the inflation factor so that the intrinsic-min-width multiplied by the inflation factor doesn't exceed the container width.

I think we probably shouldn't worry about this, though.

Comment 84

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

(In reply to David Baron [:dbaron] from comment #78)
> > There's also a question of what, if anything, we should do about CSS
> > transforms. I think probably content inside a CSS transform should not get
> > font-size inflation.
> 
> Seems reasonable.  Do we have an efficient way to find out if a frame or one
> of its ancestors has a transform applied, or should I add a new one?  (If
> so, do you think it's a bad idea to turn the state bit I added into
> something inflation-specific, or is it worth having two bits that might have
> use elsewhere?)

Actually, now that I've implemented this, it's a little less reasonable, since the inflation is per-block, and CSS transforms can be on an inline.  But I suppose I'll live with:  inflation will be disabled if the block is transformed, but for a transform on an inline, inflation will still apply.  Or should I scratch the whole idea instead, or try to change things so that we don't assume it doesn't vary within a block (I'd need to go back and look at what depends on that)?

Comment 85

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

(In reply to David Baron [:dbaron] from comment #84)
> Actually, now that I've implemented this, it's a little less reasonable,
> since the inflation is per-block, and CSS transforms can be on an inline. 
> But I suppose I'll live with:  inflation will be disabled if the block is
> transformed, but for a transform on an inline, inflation will still apply.

I think that's totally fine.

Comment 86

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

I've debugged most of the test failures I've hit, but now I've come to one I'm not sure (immediately, anyway) what to do about.  The failure is the abort that the reflow state chain is consistent with frame parentage (in nsLayoutUtils::InflationMinFontSizeFor(const nsHTMLReflowState &aReflowState)), failing on layout/reftests/bugs/409084-1a.html.  It's failing because there's a codepath in table reflow that chains the cell reflow state directly to a row group reflow state without a row reflow state in between, in particular the call in nsTableRowGroupFrame::SplitSpanningCells() to row->ReflowCellFrame().  (There's a row reflow state constructed earlier in the function, inside a condition that that call isn't inside of.)

Comment 87

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

(In reply to David Baron [:dbaron] from comment #86)
> (There's a row reflow state constructed earlier in the function, inside a
> condition that that call isn't inside of.)

Er, sorry, there's a row reflow state constructed earlier in nsTableRowGroupFrame::SplitRowGroup, which is the caller of SplitSpanningCells, but inside a condition not containing the call to SplitSpanningCells.

Comment 88

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

Just make the row reflow state construction unconditional, and pass it down? Seems like the logical thing to do.

Comment 89

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

Attachment: Don't duplicate frame state bits, so that we can assert about NS_FRAME_IN_REFLOW during painting. (, patch 3.5)

Comment 90

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

Attachment: Add the NS_FRAME_INSIDE_TRANSFORM for frames that are inside of a CSS transform. (, patch 3.75)

Comment 91

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

Attachment: Don't construct cell reflow states with a row group reflow state as their parent; instead, always link in a table row reflow state as appropriate. (, patch 3.875)

Comment 92

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

Attachment: Implement computation of font size inflation for improved readibility of text on mobile devices. (, patch 4)

Comment 93

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #88)
> Just make the row reflow state construction unconditional, and pass it down?
> Seems like the logical thing to do.

Yeah, I was worried about the performance of doing that, but now that I've looked at the code more, I'm not.  (We'd still only construct the reflow state for a row that we're trying to split or push.)

Comment 94

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

Attachment: Run existing tests without font size inflation, even when it is enabled. (, patch 17)

Comment 95

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

David, I think you didn't respond to comment #69?

Comment 96

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

Parts of comment #82 as well.

Comment 97

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #95)
> David, I think you didn't respond to comment #69?

Right; I'm still working on updating patch 9.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #96)
> Parts of comment #82 as well.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #82)
> (In reply to David Baron [:dbaron] from comment #78)
> > I'm not sure if I should -- when does that get set to something other than
> > 1.0?  In particular, if it gets set during panning/zooming on mobile, I
> > don't want it.
> 
> It gets set during zooming, but don't you want the "visual" font-size here,
> which is affected by zooming?

What sort of zooming are you talking about?  I absolutely do *not* want this to be affected by the zooming/panning that happens on mobile -- the whole point is choosing a font size so that after the user zooms a block to fill the width of the device, the text is large enough to be readable -- and to not break the zoom/pan model, i.e., apply the inflation to all zooms.

> > > There's also a question of what, if anything, we should do about CSS
> > > transforms. I think probably content inside a CSS transform should not get
> > > font-size inflation.
> > 
> > Seems reasonable.  Do we have an efficient way to find out if a frame or one
> > of its ancestors has a transform applied, or should I add a new one?  (If
> > so, do you think it's a bad idea to turn the state bit I added into
> > something inflation-specific, or is it worth having two bits that might have
> > use elsewhere?)
> 
> I think you should probably add a new state bit.
> 
> You can probably ignore translation-only transforms.

This is done in the revised patch 4.

> > > @@ +4491,5 @@
> > > > +   * count as a container, so that, for example, an indented quotation
> > > > +   * didn't end up with a smaller font size.  However, it's hard to
> > > > +   * distinguish these situations where we really do want the indented
> > > > +   * thing to count as a container, so we don't try, and blocks are
> > > > +   * always containers.bringing
> > > 
> > > It seems to me that the ideal thing here would be to have anything that's
> > > not in a line-of-inlines be a container for font-size inflation. What do you
> > > think?
> > 
> > I think that's essentially what I've done -- although inline-blocks at
> > similar things (but not form controls) establish their own containers.  What
> > change were you suggesting?
> 
> I think I was suggesting inline-block, inline-box and inline-table should
> not establish their own containers. Having them behave inconsistently with
> form elements makes me a little uncomfortable. Can you outline your
> reasoning for why they should establish their own containers?

I could go either way on this, I suppose, but basically because I think inline-block tends to be used in ways that are only "inline" in a layout sense, not a conceptual sense, such as a bunch of inline blocks all in a row with no other inline content.

> > > ::: modules/libpref/src/init/all.js
> > > @@ +1555,5 @@
> > > > + * A value greater than zero enables font size inflation for
> > > > + * pan-and-zoom UIs, so that if a block's width is scaled to match the
> > > > + * device's width, the fonts in a block are at least the font size
> > > > + * given.  The value given is in twips, i.e., 1/20 of a point, or 1/1440
> > > > + * of an inch.
> > > 
> > > I'm not sure twips is the right measure here. Why did you choose it? Seems
> > > to me we should use something that corresponds closer to viewing angle ...
> > > CSS pixels (modulo scaling)?
> > 
> > It's possible that some way to attempt to measure viewing angle would make
> > sense at the very small end of devices, but I think beyond that devices are
> > likely to be held at a comfortable arm's length -- I really don't think we
> > should be assuming that a user will be holding a 8" tablet at twice the
> > distance as a 4" phone -- I think it's far more likely they'll be close to
> > the same distance.
> 
> I don't mean viewing angle of the entire screen, but angle subtended at the
> eye by the unit. CSS px is approximately a constant angle subtended at the
> eye, as you know. So if we want text to appear to be at least a certain
> size, CSS px seems like a suitable unit.

I have no idea what you're proposing here.

Comment 98

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

> > I think you should probably add a new state bit.
> > 
> > You can probably ignore translation-only transforms.
>
> This is done in the revised patch 4.

You're not ignoring translation-only transforms in patch 3.75.

There's another issue involving subdocuments. Right now you're not propagating the has-transform flag down into subdocuments. We could fix that, but in bug 699351 it looks like mobile devs are going to implement mobile zooming using transforms on the <browser> element, and we don't want to disable font-size inflation in there.

So maybe we should just ignore transforms after all.

> I have no idea what you're proposing here.

I think ideally we want the "minimum visual font size" to be specified in a unit that's proportional to the angle subtended at the eye, rather than a physical unit. A physical unit value that's a good minimum font size at one viewing distance will be incorrect for another viewing distance.

When no zooming of any kind is present, CSS pixels are a reasonable unit. However, I don't see anything in our code that actually ensures the device-pixel-to-CSS-pixel ratio is set correctly on mobile. So let's leave this alone for now. If we decide later that a viewing-angle-based pref is necessary, we can add it as an extra pref.

> I could go either way on this

Me too, so let's not change it.

Comment 99

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #98)
> > > I think you should probably add a new state bit.
> > > 
> > > You can probably ignore translation-only transforms.
> >
> > This is done in the revised patch 4.
> 
> You're not ignoring translation-only transforms in patch 3.75.

Oops, yes, I missed the last line there.

Ignoring translation-only transforms is hard, though, since changing a transform from translation-only to non-translation-only need not trigger reflow.  If we wanted this, we'd need to make it trigger reflow again, which I really don't want to do.  So I think that's the wrong thing to do.

> There's another issue involving subdocuments. Right now you're not
> propagating the has-transform flag down into subdocuments. We could fix
> that, but in bug 699351 it looks like mobile devs are going to implement
> mobile zooming using transforms on the <browser> element, and we don't want
> to disable font-size inflation in there.
> 
> So maybe we should just ignore transforms after all.

Yeah, I think that's probably best.

I think it makes sense to stop doing inflation when there are transforms *between* the containing block for the inflation and the element being transformed.  However, that's exactly the opposite of what I described in comment 84, so I think what I've done for ignoring transforms is entirely useless.

So I'll pull patch 3.75 and revise patch 4 again to match.

> > I have no idea what you're proposing here.
> 
> I think ideally we want the "minimum visual font size" to be specified in a
> unit that's proportional to the angle subtended at the eye, rather than a
> physical unit. A physical unit value that's a good minimum font size at one
> viewing distance will be incorrect for another viewing distance.
> 
> When no zooming of any kind is present, CSS pixels are a reasonable unit.
> However, I don't see anything in our code that actually ensures the
> device-pixel-to-CSS-pixel ratio is set correctly on mobile. So let's leave
> this alone for now. If we decide later that a viewing-angle-based pref is
> necessary, we can add it as an extra pref.

ok.  (I'm still not sure how you're proposing we figure out how far the device is from the user's eyes.)

Comment 100

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

> However, I don't see anything in our code that actually ensures the device-pixel
> -to-CSS-pixel ratio is set correctly on mobile.

There is browser.viewport.scaleRatio, but it only feeds into calculations in getScaleRatio() in browser.js, and there's no way to access that directly from layout of course. We probably should fix that, but not here.

Comment 101

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

(In reply to David Baron [:dbaron] from comment #99)
> ok.  (I'm still not sure how you're proposing we figure out how far the
> device is from the user's eyes.)

With the sensors on some devices these days, we could directly detect it!

Short of that, I think we'd hardcode an estimate for particular device types or use system settings. We need to do this for CSS px to work well across devices anyway.

Comment 102

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #69)
> @@ +417,5 @@
> > +  void ClearTextRun(nsTextFrame* aStartContinuation, bool aInflated);
> > +
> > +  void ClearTextRuns() {
> > +    ClearTextRun(nsnull, true);
> > +    ClearTextRun(nsnull, false);
> 
> Pass a mask to ClearTextRun so we don't have to do two passes?

Looking at ClearTextRun, this doesn't seem so productive, since what ClearTextRun does is get the text run and do a bunch of things with it.  It seems far less invasive to just call it twice, unless I'm missing some other way to do this.

Comment 103

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

Attachment: Don't construct cell reflow states with a row group reflow state as their parent; instead, always link in a table row reflow state as appropriate. (, patch 3.875)

Comment 104

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

Attachment: Apply font size inflation to text. (, patch 9)

Comment 105

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

Attachment: Make other users of font metrics (other than MathML and XUL) honor font size inflation. (, patch 15)

Comment 106

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

Comment on attachment 574212 [details] [diff] [review]
Don't construct cell reflow states with a row group reflow state as their parent; instead, always link in a table row reflow state as appropriate.  (, patch 3.875)

The previous version of this patch was actually wrong, since it was constructing the reflow state for the wrong row.  This is just the simple approach of constructing a new reflow state right where we need it, which I think is the best we can do.

Comment 107

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

Comment on attachment 574214 [details] [diff] [review]
Apply font size inflation to text.  (, patch 9)

This is revised per your previous comments (see also comment 102).

Comment 108

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

Comment on attachment 574215 [details] [diff] [review]
Make other users of font metrics (other than MathML and XUL) honor font size inflation.  (, patch 15)

You reviewed this one before, but I needed to fix up the nsListControlFrame changes substantially, so they need re-review.  (The changes in the old patch were trivial; the new ones are less so.)

Comment 109

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

Comment on attachment 574214 [details] [diff] [review]
Apply font size inflation to text.  (, patch 9)

Review of attachment 574214 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +4104,5 @@
> +    return true;
> +  }
> +  FrameProperties props = Properties();
> +  if (props.Get(UninflatedTextRunProperty()) == aTextRun) {
> +    props.Delete(UninflatedTextRunProperty());

Can we avoid this Get() by checking TEXT_HAS_FONT_INFLATION first? I think so, so let's.

Comment 110

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #109)
> Comment on attachment 574214 [details] [diff] [review] [diff] [details] [review]
> Apply font size inflation to text.  (, patch 9)
> 
> Review of attachment 574214 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsTextFrameThebes.cpp
> @@ +4104,5 @@
> > +    return true;
> > +  }
> > +  FrameProperties props = Properties();
> > +  if (props.Get(UninflatedTextRunProperty()) == aTextRun) {
> > +    props.Delete(UninflatedTextRunProperty());
> 
> Can we avoid this Get() by checking TEXT_HAS_FONT_INFLATION first? I think
> so, so let's.

I'll try it.  This code was definitely fragile, and I no longer remember all of what I knew about it when I wrote this.

Comment 111

[Mozilla RelEng Bot]

Try run for beb6bb1a500f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=beb6bb1a500f
Results (out of 277 total builds):
    success: 253
    warnings: 24
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-beb6bb1a500f

Comment 112

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

I've made 2 minor changes to diagnose and fix test failures that I don't feel need re-review:

 (1) I changed patch 16 to output diagnostics for reftest failures in a format that
     reftest-analyzer can read (and put the function used to do that in WindowSnapshot.js)

 (2) I changed the following in patch 6:
     +  font.size *= aInflation;
     into:
     +  font.size = NSToCoordRound(font.size * aInflation);
     to avoid letting floating point error load to picking the wrong nscoord when there's an
     exactly correct nscoord.

Comment 113

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9abc8ef4626
https://hg.mozilla.org/integration/mozilla-inbound/rev/d62512892555
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f89ee5d08f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c511af7d8f58
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a6a560a1492
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0ec1183d19
https://hg.mozilla.org/integration/mozilla-inbound/rev/46669afabd15
https://hg.mozilla.org/integration/mozilla-inbound/rev/74f32abaa8c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f197554cf989
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2405eb5b90
https://hg.mozilla.org/integration/mozilla-inbound/rev/b48954598d7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a82577259c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0235d1541e58
https://hg.mozilla.org/integration/mozilla-inbound/rev/450f2557d3a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e24d196602bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/03c2ea0eeba3
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aca4ef8e538
https://hg.mozilla.org/integration/mozilla-inbound/rev/11ec362e780b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b92a3b96446

Need followups for:
 * XUL and MathML
 * further testing of the size detection code across devices
 * figuring out UI for the prefs, if any
 * settling on default values for the prefs
 * the FIXME/REVIEW remaining in the landed patches

Comment 114

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7c7dc8193692

https://tbpl.mozilla.org/php/getParsedLog.php?id=7395819&tree=Mozilla-Inbound for Windows crashtest assertions in 478131-1.html, https://tbpl.mozilla.org/php/getParsedLog.php?id=7396505&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=7395344&tree=Mozilla-Inbound for failures in both halves of Android reftests. (Outside chance it also turns the frequent timeouts in Android crashtests into permanent timeouts in C1, retriggers for that are still going.)

Comment 115

[Phil Ringnalda (:philor)]

Come to think of it, I don't remember ever seeing a crashtest on Android clearly failing, probably the "2400 seconds without output" during 446328.html four times in a row is its charming way of saying that it crashed.

Comment 116

[Mozilla RelEng Bot]

Try run for 541a6a13580c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=541a6a13580c
Results (out of 289 total builds):
    exception: 11
    success: 163
    warnings: 19
    failure: 96
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-541a6a13580c

Comment 117

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

I fixed the Windows crashtest assertion with the added aInflation != 1.0f test in https://hg.mozilla.org/mozilla-central/rev/e233695b1310 ; the problem was that round-tripping large numbers through floats was lossy.

Fixed the Android R3 and C1 problems when I finally realized that my problem was that I hadn't fully disabled inflation while running reftests, since I additionally needed to disable it in remotereftest.py.  See the second file's worth of changes in https://hg.mozilla.org/mozilla-central/rev/cf764be32bc3

Relanded:
https://hg.mozilla.org/mozilla-central/rev/ada794061c14
https://hg.mozilla.org/mozilla-central/rev/1aaacbcb3b61
https://hg.mozilla.org/mozilla-central/rev/4751e5b548a7
https://hg.mozilla.org/mozilla-central/rev/e229fd69d8b1
https://hg.mozilla.org/mozilla-central/rev/3d486b1ad76a
https://hg.mozilla.org/mozilla-central/rev/d3e230bec2f9
https://hg.mozilla.org/mozilla-central/rev/a10a1bc3cb40
https://hg.mozilla.org/mozilla-central/rev/e233695b1310
https://hg.mozilla.org/mozilla-central/rev/b0eb968eceb5
https://hg.mozilla.org/mozilla-central/rev/2a00ab5642c1
https://hg.mozilla.org/mozilla-central/rev/236ba3dfe647
https://hg.mozilla.org/mozilla-central/rev/d090012f8439
https://hg.mozilla.org/mozilla-central/rev/d3724c31c3a0
https://hg.mozilla.org/mozilla-central/rev/738725e75362
https://hg.mozilla.org/mozilla-central/rev/51ebc0da58ba
https://hg.mozilla.org/mozilla-central/rev/ff09e8e9bd8a
https://hg.mozilla.org/mozilla-central/rev/ccabd715b232
https://hg.mozilla.org/mozilla-central/rev/eb173d230150
https://hg.mozilla.org/mozilla-central/rev/cf764be32bc3

I should probably get to the point where we can run crashtests and reftests with inflation enabled.

The reftest failures are all "expected" issues:  in most cases, rounding issues resulting from inflation to a non-integer-pixel font size.  We can fix these with adjustments to the tests (in most cases, -moz-text-size-adjust: none, though in the case of auto-offset-inline-block-1-ref.html and perhaps a few others, by splitting elements in a way that text run splits will match between test and reference).

The crashtest failure is more worrying:  it's probably a sign of something wrong with the patch, but it's really hard to figure out what, and I think I've got a better chance of figuring it out once the patch is landed since then I'll be getting crash data.  The basic problem with crashtests is that a crashtest run that includes all three of layout/base/, layout/forms/, and layout/generic/ leads to the crashtest run disconnecting.  (Perhaps due to a crash?  Android crashtest/reftest runs have pretty much useless diagnostics.)  But if I run any one of those directories alone, it doesn't crash.  So it's not clear how to isolate the problem to a useful testcase, and there's no way to get a crash stack or anything close out of tinderbox (since it just reports the issue as a disconnected tegra).


Additional followup (in addition to comment 113 and the above):
  It's turned on by default in mobile/xul/app/mobile.js .  But that probably doesn't apply to the native UI, so I'd need to write something doing that and land it on birch (after the rest of this merges to birch, presumably).

Comment 118

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

I merged mozilla-central to birch, and then pushed the same default pref patch that I'd pushed for the XUL UI for the Android UI as well:
https://hg.mozilla.org/projects/birch/rev/ce03d0240a30

Comment 119

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

One more followup, from today's Platform meeting:
[2011-11-29 11:08:17] <mounir> dbaron: button elements are small and that's weird :(

Comment 120

[j.j.]

At least "-moz-text-size-adjust" needs documentation.
http://dbaron.org/log/20111126-font-inflation

Comment 121

[Jean-Yves Perrier [:teoli]]

I've documented this (as well as MS and Webkit behavior, as far as I was able to collect the information): https://developer.mozilla.org/en/CSS/text-size-adjust

Two questions: 
1) MS ignore -ms-text-size-adjust if the viewport is set via <meta>. This is not the case of Gecko, is that right?
2) Will this be in Firefox 11 for mobile? (Should I update Firefox 11 for developers?)

Comment 122

[Kevin Brosnan [Ex-Mozilla]]

This will ship in the first Firefox mobile native release. Currently that is targeting Firefox 13.