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

Status: VERIFIED FIXED

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

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: 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

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Seems like a font inflation bug to me. Can you use the preference to disable font inflation and test again?

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Hi Mark, how do I disable font inflation?

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

font.size.inflation.minTwips

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

font.size.inflation.minTwips 0 caused the checkboxes and radio buttons to be normal again.  Font inflation issue.

Comment 5

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

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.

Comment 6

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

Er, bug 706609 covers dropdowns; this one should cover radios and checkboxes.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Like so?

nsGfxCheckboxControlFrame and nsGfxRadioControlFrame are the only classes
that inherits nsFormControlFrame AFAICT, so its Reflow method seems the
right place to fix both.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Screenshot with patch, zoomed in

Comment 10

[Mats Palmgren (inactive)]

Attachment: Screenshot with patch, not zoomed

The right border is missing for the checkboxes due to an unrelated pixel
rounding bug (it occurs also without the patch).

Comment 11

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

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

Comment 12

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

It would be good to also add a reftest in layout/base/tests/test_font_inflation_reftests.html and layout/base/tests/font-inflation/

Comment 13

[Mats Palmgren (inactive)]

(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.

Comment 14

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c30c5187e3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d526d7ec680

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/9c30c5187e3e
https://hg.mozilla.org/mozilla-central/rev/4d526d7ec680

Comment 16

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

Mats, be sure to get this onto whatever branch 11 is on (aurora now, will be beta in a couple hours)

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 590035 [details] [diff] [review]
Like so?

Please approve for the branch Fx11 is on.

Comment 18

[Alex Keybl [:akeybl]]

(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?

Comment 19

[Mats Palmgren (inactive)]

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.

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 590035 [details] [diff] [review]
Like so?

[Triage Comment]
Thanks Mats - approved for Aurora 12 and Beta 11.

Comment 22

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ad5d9c45278
https://hg.mozilla.org/releases/mozilla-beta/rev/6a71fe8b7ada

Comment 23

[Andreea Pod]

Verified fixed on build:
Firefox 11 (tinderbox build): 	1328738704/	08-Feb-2012 15:44
20120208140504
Device: LG Optimus 2X (Android 2.2.2)

Comment 24

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

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.

Comment 25

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

(But please wait until bug 743817 lands before fixing that.)