Closed Bug 706889 Opened 8 years ago Closed 8 years ago

checkboxes and radio buttons are extremely small when text around them is inflated

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
fennec 11+ ---

People

(Reporter: nhirata, Assigned: mats)

References

()

Details

(Whiteboard: readability, [font-inflation: form controls][inbound])

Attachments

(4 files)

Attached image screenshot
1. http://people.mozilla.com/~nhirata/html_tp/formsninput.html

Expeced: the checkbox and radio button should match the size of the text
Actual: the checkboxes and radio buttons are extremely small

Note:
Galaxy S II, Android 2.3, Build 20111201
Seems like a font inflation bug to me. Can you use the preference to disable font inflation and test again?
Hi Mark, how do I disable font inflation?
font.size.inflation.minTwips
font.size.inflation.minTwips 0 caused the checkboxes and radio buttons to be normal again.  Font inflation issue.
Whiteboard: readability
Whiteboard: readability → readability, DUPEME
Seems like we should be inflating checkboxes and radio buttons for font size inflation.  It also looks like dropdown listboxes need work, and we may need to revisit buttons as well.
Er, bug 706609 covers dropdowns; this one should cover radios and checkboxes.
Whiteboard: readability, DUPEME → readability, [font-inflation: form controls]
Assignee: nobody → dbaron
Priority: -- → P1
Duplicate of this bug: 711755
Summary: checkboxes and radio buttons are extremely small on the Galaxy S II → checkboxes and radio buttons are extremely small when text around them is inflated
Assignee: dbaron → nobody
tracking-fennec: --- → 11+
Assignee: nobody → sjohnson
Assignee: sjohnson → matspal
Attached patch Like so?Splinter Review
nsGfxCheckboxControlFrame and nsGfxRadioControlFrame are the only classes
that inherits nsFormControlFrame AFAICT, so its Reflow method seems the
right place to fix both.
Attachment #590035 - Flags: review?(dbaron)
The right border is missing for the checkboxes due to an unrelated pixel
rounding bug (it occurs also without the patch).
Comment on attachment 590035 [details] [diff] [review]
Like so?

>+  nsLeafFrame::Reflow(aPresContext, aDesiredSize, aReflowState, aStatus);
>+

use nsresult rv = nsLeafFrame::Reflow(...);
and then
if (NS_FAILED(rv)) {
  return rv;
}

>+  if (nsLayoutUtils::FontSizeInflationEnabled(aPresContext)) {
>+    float inflation = nsLayoutUtils::FontSizeInflationFor(aReflowState);

add NS_ASSERTION(inflation >= 1.0f, "we assume inflation >= 1 with the simple handling of overflow areas below");

>+    aDesiredSize.width *= inflation;
>+    aDesiredSize.height *= inflation;
>+    aDesiredSize.UnionOverflowAreasWithDesiredBounds();
>+    FinishAndStoreOverflow(&aDesiredSize);
>+  }
>+  return NS_OK;
> }

r=dbaron with that
Attachment #590035 - Flags: review?(dbaron) → review+
It would be good to also add a reftest in layout/base/tests/test_font_inflation_reftests.html and layout/base/tests/font-inflation/
(In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #11)
> use nsresult rv = nsLeafFrame::Reflow(...);

Fixed.

> >+    float inflation = nsLayoutUtils::FontSizeInflationFor(aReflowState);
> 
> add NS_ASSERTION(inflation >= 1.0f, "we assume inflation >= 1 with the
> simple handling of overflow areas below");

The documentation for nsLayoutUtils::FontSizeInflationFor now says:

   * Return the font size inflation *ratio* for a given frame.  This is
   * the factor by which font sizes should be inflated; it is never
   * smaller than 1.

so I didn't add an assertion.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c30c5187e3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d526d7ec680
Flags: in-testsuite+
Whiteboard: readability, [font-inflation: form controls] → readability, [font-inflation: form controls][inbound]
Target Milestone: --- → Firefox 12
Mats, be sure to get this onto whatever branch 11 is on (aurora now, will be beta in a couple hours)
Comment on attachment 590035 [details] [diff] [review]
Like so?

Please approve for the branch Fx11 is on.
Attachment #590035 - Flags: approval-mozilla-beta?
Attachment #590035 - Flags: approval-mozilla-aurora?
(In reply to Mats Palmgren [:mats] from comment #17)
> Comment on attachment 590035 [details] [diff] [review]
> Like so?
> 
> Please approve for the branch Fx11 is on.

This patch doesn't look specific to mobile - if that's true, what risk to desktop does this carry?
The patch doesn't do anything unless font-size inflation is enabled.
Font-size inflation is enabled by default for mobile.  It's disabled by default
for desktop but you can enable it in about:config.  The risk is low even when
enabled - the worst that can happen is that radio/checkbox controls have wrong size.
No longer blocks: 707361
Duplicate of this bug: 707361
Comment on attachment 590035 [details] [diff] [review]
Like so?

[Triage Comment]
Thanks Mats - approved for Aurora 12 and Beta 11.
Attachment #590035 - Flags: approval-mozilla-beta?
Attachment #590035 - Flags: approval-mozilla-beta+
Attachment #590035 - Flags: approval-mozilla-aurora?
Attachment #590035 - Flags: approval-mozilla-aurora+
Verified fixed on build:
Firefox 11 (tinderbox build): 	1328738704/	08-Feb-2012 15:44
20120208140504
Device: LG Optimus 2X (Android 2.2.2)
Status: RESOLVED → VERIFIED
The reftests for this bug don't actually test this bug, since they're != reftests, and there's a huge difference between them in that the text is inflated in one but not the other, so they'd pass even without this fix.  This patch needs better tests.
Flags: in-testsuite+ → in-testsuite?
(But please wait until bug 743817 lands before fixing that.)
You need to log in before you can comment on or make changes to this bug.