708175 - Form controls with fixed width or height shouldn't have their text inflated

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[John Hammink]

Attachment: Screenshot, truncated button text

While the testpage at http://people.mozilla.com/~jhammink/webapi_test_pages/CameraAPIdemo.html  is rendered correctly in nightly xul on Android; it is not rendered correctly on latest nightly - the buttons are truncated as shown in the attachment.

Comment 1

[Doug Turner (:dougt)]

John, is this really a cameria api bug?  Do you see this with other buttons?  This sounds alot like a readablity bug and nothing to do with Camera API.

Comment 2

[Doug Turner (:dougt)]

s/cameria/camera

Comment 4

[John Hammink]

This may not be camera api bug - however the "capture" button appears only when this is enabled.  In any case, seems to be a dupe.

Comment 6

[JP Rosevear [:jpr]]

Release blockers.

Comment 8

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

How does this look after the recent form control fixes?

Comment 9

[JP Rosevear [:jpr]]

(In reply to David Baron [:dbaron] from comment #8)
> How does this look after the recent form control fixes?

Url is http://people.mozilla.com/~jhammink/webapi_test_pages/CameraAPIdemo.html for testing.

Comment 10

[arky [:arky]]

(In reply to JP Rosevear [:jpr] from comment #9)
> (In reply to David Baron [:dbaron] from comment #8)
> > How does this look after the recent form control fixes?
> 
> Url is
> http://people.mozilla.com/~jhammink/webapi_test_pages/CameraAPIdemo.html for
> testing.

It looks good on nightly!  Thanks for the fix. Attaching a screenshot.

Comment 11

[arky [:arky]]

Attachment: Screenshot of the fix

Comment 12

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

can we close this bug?

Comment 13

[Scott Johnson (:jwir3)]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> can we close this bug?

Yep. Closing as wfm.

Comment 14

[Mounir Lamouri (:mounir)]

Using a 2012-03-09 Nightly doesn't make this works for me on my test case.

Comment 15

[JP Rosevear [:jpr]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #14)
> Using a 2012-03-09 Nightly doesn't make this works for me on my test case.

What is the URL for your test case?

Comment 16

[Mounir Lamouri (:mounir)]

As simple as:
data:text/html,<input type='file' accept='image/*'>

Comment 18

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

morphing this bug to cover the more generic case

Comment 19

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

Sounds like:
 (a) Mounir's going to land changes that cover the Camera case, but
 (b) there's a more generic problem here that can be seen, e.g., on the "Search" button at the very top of the Bugzilla header:  we need to make inflation vs. not-inflation decisions on a finer-grained basis than block-by-block when there are inline-blocks or form controls with fixed widths involved -- i.e., we may need to disable inflation in those cases.  This change should be relatively straightforward, I think.

Comment 20

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

Also, maybe the selects at the top of http://crash-stats.mozilla.org/ ?

Comment 21

[Mounir Lamouri (:mounir)]

(In reply to David Baron [:dbaron] from comment #19)
> Sounds like:
>  (a) Mounir's going to land changes that cover the Camera case, but

It is landed. Should be in today's Nightly.

Comment 22

[JP Rosevear [:jpr]]

The latest nightly has one button, "Browse" but I do not see the spacing as per comment #11, the 'e' in Browse is slightly cutoff by the button edge.

Comment 23

[Scott Johnson (:jwir3)]

Attachment: Screenshots of different font inflations,crash-tests

I noticed an interesting situation with the font inflation on the test cases. Sometimes, when things are asked to inflate //more//, the font controls are rendered correctly.

See the attached image - The rendering at 120 minTwips results in select boxes on the crash-stats.mozilla.org website being cut off, as described by this bug. But, if I increase the font inflation to 500 min twips, it increases the size, but it actually fits in the rendering of the box itself. 

With 15 em per line and 8 em per line, I can't get it to rendering in the cut-off way that I see with 120 minTwips.

Similar issues happen with the file input dialog mentioned in comment 16.

Comment 24

[Scott Johnson (:jwir3)]

Attachment: b708175

Comment 25

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

Comment on attachment 612376 [details] [diff] [review]
b708175

># HG changeset patch
># Parent 869edbbfad819b65543cf38ada8d1a8e1005cdf9
># User Scott Johnson <[e-mail]>
>Bug 708175:
>
>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>--- a/layout/base/nsLayoutUtils.cpp
>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -4665,16 +4665,38 @@ nsLayoutUtils::FontSizeInflationInner(co
>     return 1.0;
>   }
> 
>   if (aMinFontSize <= 0) {
>     // No need to scale.
>     return 1.0;
>   }
> 
>+  // If between this current frame and its font inflation container there is
>+  // a non-inline element with fixed width, then we should not inflate fonts

"fixed width" -> "fixed width or height" (and rewrap)

>+  // for this frame.
>+  for (const nsIFrame* f = aFrame;
>+       f && !IsContainerForFontSizeInflation(f);
>+       f = f->GetParent()) {
>+    nsIContent* content = f->GetContent();
>+    // Also, if there is more than one frame corresponding to a single
>+    // content node, we want the outermost one.
>+    if (!(f->GetParent() &&
>+          f->GetParent()->GetContent() == content) &&
>+        f->GetType() != nsGkAtoms::inlineFrame) {
>+      nsStyleCoord stylePosWidth = f->GetStylePosition()->mWidth;
>+      nsStyleCoord stylePosHeight = f->GetStylePosition()->mHeight;
>+      if (stylePosWidth.GetUnit() != nsStyleUnit::eStyleUnit_Auto ||
>+          stylePosHeight.GetUnit() != nsStyleUnit::eStyleUnit_Auto) {

Drop the "nsStyleUnit::" (twice).


>diff --git a/layout/base/tests/font-inflation/input-text-1.html b/layout/base/tests/font-inflation/input-text-1.html
>--- a/layout/base/tests/font-inflation/input-text-1.html
>+++ b/layout/base/tests/font-inflation/input-text-1.html
>@@ -1,11 +1,10 @@
>-<!DOCTYPE HTML>
>+<!DOCTYPE html>
> <style>
>-div { font-size: 12px; line-height: 1.0; width: 450px }
>-input { font-size: 12px; width: 200px }
>-input { height: 50px }
>+div { font-size: 12px; line-height: 1.0; width: 450px; }
>+input { font-size: 12px; }

Leave the DOCTYPE line and the div{} line as-is.



I'm still working on figuring out what changed with the rest of the tests...

Comment 26

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

So before this patch:
 input-text-1 had width: 200px, height: 50px
 input-text-2 had size="15", height: 50px
 input-text-3 had no width constraint, height: 50px

After the patch, we have:
 input-text-1 - no width constraint, no height constraint
 input-text-2 - size="15", height: 50px
 input-text-3 - no width constraint, height: 50px
 input-text-4 - width: 200px, height: 50px
 input-text-5 - width: 200px, no height constraint
 input-text-6 - no width constraint, no height constraint

So I think in your new world tests 1 and 6 are the same, and you've lost test coverage of inflating a size="15" (since the height now makes it uninflated).

Could you redo the refactoring of the tests so that:
 - you have one set input-text-{1,2,3}-height that are like the current tests but have different references
 - you have another set input-text-{1,2,3}-noheight that have the height removed, so the references are like they are now
 - the copying is represented in hg as a file copy/move? 


r=dbaron with that fixed, comment 25 fixed, and a useful commit message describing what changed

Comment 27

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

Comment on attachment 612376 [details] [diff] [review]
b708175

I'd probably like to take a brief look at the revisions (see above), so marking review-.

Comment 28

[Scott Johnson (:jwir3)]

Attachment: b708175 (v2)

(In reply to David Baron [:dbaron] from comment #25)

> "fixed width" -> "fixed width or height" (and rewrap)
Fixed.

> Drop the "nsStyleUnit::" (twice).
Fixed.

> Leave the DOCTYPE line and the div{} line as-is.
Fixed.

(In reply to David Baron [:dbaron] from comment #26)
> Could you redo the refactoring of the tests so that:
>  - you have one set input-text-{1,2,3}-height that are like the current
> tests but have different references
Fixed. I also looked over the select-combobox and textarea tests. I made textarea-1 and textarea-1-ref have a fixed height as well (as that is how the original test was, and it's not necessary to change it).

>  - you have another set input-text-{1,2,3}-noheight that have the height
> removed, so the references are like they are now
Fixed.

>  - the copying is represented in hg as a file copy/move? 
Done.

(In reply to David Baron [:dbaron] from comment #27)
> I'd probably like to take a brief look at the revisions (see above), so
> marking review-.
Ok, I'm posting this patch for review again. Please let me know if there are other things that you see & I'll make sure they get fixed promptly.

Thanks for looking this over, David!

Comment 29

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

Comment on attachment 613427 [details] [diff] [review]
b708175 (v2)

r=dbaron

Comment 30

[Scott Johnson (:jwir3)]

Just a quick update on why this hasn't landed yet - I originally went to land it yesterday, but because of bug 743817, I had to perform a local (manual) merge of my tests. 

After performing this merge, I noticed that there were two tests that required assertions that were passing unexpectedly. The lines from the reftest.list file:

asserts-if(gtk2Widget,1-2) test-pref(font.size.inflation.emPerLine,15) != input-checkbox.html input-checkbox.html
asserts-if(gtk2Widget,1-2) test-pref(font.size.inflation.emPerLine,15) != input-radio.html input-radio.html

I'm not sure these asserts are necessary, but I'm going to check on try, just to be sure. That and the merge are what is taking so long for this to land.

Comment 31

[Scott Johnson (:jwir3)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d41ddb6b9bc

Comment 32

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/412d3777596a

because select-combobox-3.html was failing consistently on Android:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10829919&tree=Mozilla-Inbound

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/6d41ddb6b9bc
https://hg.mozilla.org/mozilla-central/rev/412d3777596a

Comment 34

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/6d41ddb6b9bc

Comment 35

[(no longer active)]

This is backed out.

Comment 36

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

Scott -- did you get a chance to look at the images for the reftest failures?  (You should be able to look at them via https://tbpl.mozilla.org/ .)

Depending on what's going on, it may make sense to fix the test in some way, to reland with the text marked as known failing, or something else.

Comment 37

[Scott Johnson (:jwir3)]

Yes.  It's just an issue with a vertical spacing on Android reftests. I'm not sure how to run them locally, so I'm waiting for try to complete to verify it's fixed before re-landing. it will be landed this evening.

Comment 38

[Mozilla RelEng Bot]

Try run for 854537415763 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=854537415763
Results (out of 14 total builds):
    exception: 1
    success: 11
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-854537415763

Comment 39

[Scott Johnson (:jwir3)]

Re-landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1587faab5e5c

Comment 40

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

Based on the effect of this bug on form controls, we'd like to keep this a beta blocker

Comment 41

[Scott Johnson (:jwir3)]

(In reply to Mark Finkle (:mfinkle) from comment #40)
> Based on the effect of this bug on form controls, we'd like to keep this a
> beta blocker

Was this a comment that was supposed to go on bug 707917? I'm a bit confused about this comment, because the patch for this bug has landed on inbound and is waiting to be merged to m-c. So, I was under the impression that it was already a beta-blocker and I didn't realize we were considering making it into a non-blocker.

Comment 42

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1587faab5e5c

Comment 43

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

There was a mistake in the reftest.list for the test changes in this bug that caused some of the tests to be run twice and some not at all.  I mostly fixed this, except that one of the tests failed, so I commented it out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/554a06caebf2

Comment 44

[Scott Johnson (:jwir3)]

I've submitted bug 745993 to track this issue.

Comment 45

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/554a06caebf2

Comment 46

[Cristian Nicolae (:xti)]

Bug 748434 was filled regarding this issue. Due to this, I will close the bug as verified fixed.