747857 - Text input suddenly grows in size after typing certain amount of characters

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

Steps to reproduce:
- Visit testcase
- Type a letter (for example 'q') a couple of times


Expected result:
- The text input stays at the same size

Actual result:
- The text input suddenly grows in size after typing 8 times the letter 'q'.

Reproduce this on the Samsung Galaxy Nexus and the Samsung Galaxy SII

See also video:
http://www.youtube.com/watch?v=IgrpchjAtWQ

This regressed between 2012-04-17 and 2012-04-18, regression from bug 706193, I think.

Comment 1

[Martijn Wargers (dead)]

I'm also seeing the link suddenly grow when tapping somewhere on this page:
http://people.mozilla.org/~mwargers/tests/tapping/681640_tapevents.html

Comment 2

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

I wrote a patch for this on the airplane a few days ago, but then forgot to test that the tests pass and post it.  Hopefully I'll get back to that soon.
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/d87bc5f40a15/line-threshold-form-controls

Comment 3

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

Attachment: patch

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620436 [details] [diff] [review]
patch

Review of attachment 620436 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFontInflationData.cpp
@@ +316,5 @@
> +      nsIFrame *optionChild = option->GetFirstPrincipalChild();
> +      if (optionChild && optionChild->GetType() == nsGkAtoms::textFrame) {
> +        optionResult = nsTextFrameUtils::
> +          ComputeApproximateLengthWithWhitespaceCompression(
> +            optionChild->GetContent(), optionChild->GetStyleText());

Don't we need to iterate through the child frames of <option> in case there are multiple text nodes separated by comments?

@@ +380,5 @@
> +        // need to exclude the display frame.
> +        nscoord fontSize = kid->GetStyleFont()->mFont.size;
> +        PRInt32 charCount = CharCountOfLargestOption(
> +          static_cast<nsComboboxControlFrame*>(kid)->GetDropDown());
> +        mTextAmount += charCount * fontSize;

Is this really the right thing to do for non-auto-width replaced elements?

Comment 5

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> ::: layout/generic/nsFontInflationData.cpp
> @@ +316,5 @@
> > +      nsIFrame *optionChild = option->GetFirstPrincipalChild();
> > +      if (optionChild && optionChild->GetType() == nsGkAtoms::textFrame) {
> > +        optionResult = nsTextFrameUtils::
> > +          ComputeApproximateLengthWithWhitespaceCompression(
> > +            optionChild->GetContent(), optionChild->GetStyleText());
> 
> Don't we need to iterate through the child frames of <option> in case there
> are multiple text nodes separated by comments?

I suppose so.

> @@ +380,5 @@
> > +        // need to exclude the display frame.
> > +        nscoord fontSize = kid->GetStyleFont()->mFont.size;
> > +        PRInt32 charCount = CharCountOfLargestOption(
> > +          static_cast<nsComboboxControlFrame*>(kid)->GetDropDown());
> > +        mTextAmount += charCount * fontSize;
> 
> Is this really the right thing to do for non-auto-width replaced elements?

I may at some future point want to try to consider widths on inline blocks and replaced elements, but for now I'm just trying to *not* consider form control inputs.

Comment 6

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

Attachment: patch

revised to iterate the children of <option>

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8125b89352d

Comment 8

[Ed Morley [:emorley]]

Sorry, backed this out for test failures in 40-50% of the compilable pushes since this landed (which is too high for me to be happy merging inbound into mozilla-central):

https://hg.mozilla.org/integration/mozilla-inbound/rev/308f1163a388

See bug 751808 for logs :-)

Comment 9

[(no longer active)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/308f1163a388

Comment 10

[Ed Morley [:emorley]]

Just to help explain the crashes in bug 752168 - this had ended up being merged to mozilla-central for 3-4 hours before the backout was also merged (we had a 250+ changeset backlog on mozilla-inbound due to CPG, so it slipped through with the efforts to lower that as soon as possible), so ended up being included in the Nightly respin, which is how crashes were occurring in the wild. Today's Nightly will include the backout.

Comment 11

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

(In reply to Ed Morley [:edmorley] from comment #10)
> Just to help explain the crashes in bug 752168 - this had ended up being
> merged to mozilla-central for 3-4 hours before the backout was also merged
> (we had a 250+ changeset backlog on mozilla-inbound due to CPG, so it
> slipped through with the efforts to lower that as soon as possible), so
> ended up being included in the Nightly respin, which is how crashes were
> occurring in the wild. Today's Nightly will include the backout.

I don't think those crashes are at all related to this bug.

Comment 12

[Ed Morley [:emorley]]

Ah sorry, I was just going by Scoobidiver's added dependency :-)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79f21117ea59

Comment 14

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

That was via a mozilla-inbound push that I forgot to note here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79f21117ea59

Comment 15

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

Comment on attachment 620759 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): bug 706193
User impact if declined: font inflation changes when typing in or selecting values in form controls
Testing completed (on m-c, etc.):  on mozilla-central for a bit
Risk to taking this patch (and alternatives if risky): given the mozilla-central testing, reltaively low
String changes made by this patch: none

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b7ed656b15f

Comment 17

[Kevin Brosnan [Ex-Mozilla]]

The test case in comment 1 is fixed the test case in the URL field still grows in size. Tested in Nightly, Aurora and 14 Beta 2.

Comment 18

[Kevin Brosnan [Ex-Mozilla]]

In triage I was asked to mark this bug fixed and open a new bug 757616.