Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla12

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: asa, Assigned: jwir3)

Tracking

({qawanted})

Trunk
mozilla12
ARM
Mac OS X
qawanted
Points:
---

Firefox Tracking Flags

(firefox11-)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

6 years ago
Whiteboard: [font-inflation: larger than device width]
(Assignee)

Updated

6 years ago
Assignee: nobody → sjohnson
(Assignee)

Comment 2

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

Comment 4

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

Comment 6

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

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

Comment 8

6 years ago
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adde248cc4b3
Target Milestone: --- → mozilla12
(Assignee)

Updated

6 years ago
tracking-firefox11: --- → ?

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/adde248cc4b3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Attachment #582935 - Flags: approval-mozilla-aurora?

Comment 10

6 years ago
[triage comment]
Holding off on approvals until it is confirmed to fix the issue.
Keywords: #relman/triage/needs-info
Keywords: qawanted
Duplicate of this bug: 706151
Attachment #582935 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
tracking-fennec: --- → ?
tracking-firefox11: ? → -
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.