Last Comment Bug 707855 - [fennec] [font inflation] mobile twitter fonts too large on latest nightly fennec mobile
: [fennec] [font inflation] mobile twitter fonts too large on latest nightly fe...
Status: RESOLVED FIXED
[font-inflation: larger than device w...
: qawanted
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: ARM Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Scott Johnson (:jwir3)
:
:
Mentors:
http://mobile.twitter.com
: 706151 (view as bug list)
Depends on:
Blocks: font-inflation
  Show dependency treegraph
 
Reported: 2011-12-05 18:10 PST by Asa Dotzler [:asa]
Modified: 2013-12-10 10:00 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
b707855 - Potential fix for this bug (5.87 KB, patch)
2011-12-19 13:30 PST, Scott Johnson (:jwir3)
dbaron: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Asa Dotzler [:asa] 2011-12-05 18:10:09 PST
if you visit http://mobile.twitter.com/ in the latest nightly native fennec builds the fonts are way too large. Could be from dbaron's awesome font inflation work.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-12-07 14:32:11 PST
Yeah, need to look at the page and see why they're being inflated.  This might be the issue of needing to cap the container width to the device width, which is something I meant to do.
Comment 2 Scott Johnson (:jwir3) 2011-12-19 13:30:01 PST
Created attachment 582935 [details] [diff] [review]
b707855 - Potential fix for this bug

Thanks for the assistance, dbaron. Let me know what things you think I should change in the patch (specifically the test, since we went over the patch together almost line-by-line).
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-12-19 13:41:20 PST
Comment on attachment 582935 [details] [diff] [review]
b707855 - Potential fix for this bug

>+  nscoord frameWidth = aPresContext->GetVisibleArea().width;

I'd call this iframeWidth rather than frameWidth just because "frame" could mean "nsIFrame" but here you mean <iframe> or <frame>.

>+Without the patch for bug 707855, we have a 450px container, and the minimum font size
>+at 15em per line is 44px. This means we map 0px-45px into 44px-45px, so 12px gets mapped
>+to 34px.

The minimum in that case is 30px, not 44px.  So you map 0px-45px into 30px-45px.  But the 34px is right.

>+With the patch, then we have a 240px container, so the minimum font size
>+at 15 em per line is 16px. So, we map 0px-45px into 16px-45px, so 12px gets
>+mapped to 24px.

0px-24px gets mapped to 16px-24px, so 12px should get mapped to 20px.  I'm not sure why you're seeing 24px.

>+  "== bug707855.html bug707855-ref.html",

You could call the files "container-width-clamping", perhaps?  If you're using mq, the easiest way to change the patch is to search-replace the patch file while it's not applied.

r=dbaron, but I'm still confused about the tests
Comment 4 Scott Johnson (:jwir3) 2011-12-19 15:04:10 PST
(In reply to David Baron [:dbaron] from comment #3)
> Comment on attachment 582935 [details] [diff] [review]
> b707855 - Potential fix for this bug

> I'd call this iframeWidth rather than frameWidth just because "frame" could
> mean "nsIFrame" but here you mean <iframe> or <frame>.
Done.

> The minimum in that case is 30px, not 44px.  So you map 0px-45px into
> 30px-45px.  But the 34px is right.
> 
> 0px-24px gets mapped to 16px-24px, so 12px should get mapped to 20px.  I'm
> not sure why you're seeing 24px.
Ok, I've changed those comments. I'm not sure it's running correctly on my phone... I will push it to try-server with your suggested 20px, and see if it runs as expected. That's probably a better way to verify that I have the correct settings.

> You could call the files "container-width-clamping", perhaps?  If you're
> using mq, the easiest way to change the patch is to search-replace the patch
> file while it's not applied.
Done.

Thanks for the comments. I'll land this as soon as I have verification that the tests are passing on try.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-12-19 15:31:07 PST
(In reply to Scott Johnson (:jwir3) from comment #4)
> (In reply to David Baron [:dbaron] from comment #3)
> > The minimum in that case is 30px, not 44px.  So you map 0px-45px into
> > 30px-45px.  But the 34px is right.
> > 
> > 0px-24px gets mapped to 16px-24px, so 12px should get mapped to 20px.  I'm
> > not sure why you're seeing 24px.
> Ok, I've changed those comments. I'm not sure it's running correctly on my
> phone... I will push it to try-server with your suggested 20px, and see if
> it runs as expected. That's probably a better way to verify that I have the
> correct settings.

The mochitests should run just fine on desktop, e.g., with:

TEST_PATH=layout/base/test/test_font_inflation_reftests.html make mochitest-plain
Comment 6 Mozilla RelEng Bot 2011-12-19 16:01:06 PST
Try run for a3e1383f7e75 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a3e1383f7e75
Results (out of 3 total builds):
    exception: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/sjohnson@mozilla.com-a3e1383f7e75
Comment 7 Mozilla RelEng Bot 2011-12-19 21:01:08 PST
Try run for 450055809f1d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=450055809f1d
Results (out of 27 total builds):
    success: 18
    warnings: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/sjohnson@mozilla.com-450055809f1d
Comment 8 Scott Johnson (:jwir3) 2011-12-20 14:38:05 PST
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adde248cc4b3
Comment 9 Ed Morley [:emorley] 2011-12-21 04:30:35 PST
https://hg.mozilla.org/mozilla-central/rev/adde248cc4b3
Comment 10 christian 2011-12-21 15:52:16 PST
[triage comment]
Holding off on approvals until it is confirmed to fix the issue.
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2011-12-22 14:40:26 PST
*** Bug 706151 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.