Closed Bug 707855 Opened 8 years ago Closed 8 years ago

[fennec] [font inflation] mobile twitter fonts too large on latest nightly fennec mobile

Categories

(Core :: Layout, defect)

ARM
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox11 - ---

People

(Reporter: asa, Assigned: jwir3)

References

()

Details

(Keywords: qawanted, Whiteboard: [font-inflation: larger than device width])

Attachments

(1 file)

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.
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.
Whiteboard: [font-inflation: larger than device width]
Assignee: nobody → sjohnson
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).
Attachment #582935 - Flags: review?(dbaron)
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
Attachment #582935 - Flags: review?(dbaron) → review+
(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.
(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
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
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
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adde248cc4b3
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/adde248cc4b3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #582935 - Flags: approval-mozilla-aurora?
[triage comment]
Holding off on approvals until it is confirmed to fix the issue.
Duplicate of this bug: 706151
Attachment #582935 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: --- → ?
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.