Last Comment Bug 708175 - Form controls with fixed width or height shouldn't have their text inflated
: Form controls with fixed width or height shouldn't have their text inflated
Status: RESOLVED FIXED
readability
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: mozilla14
Assigned To: Scott Johnson (:jwir3)
:
Mentors:
http://people.mozilla.com/~jhammink/w...
: 708940 712708 721823 731825 (view as bug list)
Depends on: 730289 748434 757937
Blocks: 741393 font-inflation 659188 721823 745993
  Show dependency treegraph
 
Reported: 2011-12-06 20:26 PST by John Hammink
Modified: 2012-05-29 16:54 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta+
11+


Attachments
Screenshot, truncated button text (1.39 MB, image/jpeg)
2011-12-06 20:26 PST, John Hammink
no flags Details
Screenshot of the fix (49.36 KB, image/png)
2012-03-08 16:44 PST, arky [:arky]
no flags Details
Screenshots of different font inflations,crash-tests (346.57 KB, image/png)
2012-03-27 16:40 PDT, Scott Johnson (:jwir3)
no flags Details
b708175 (14.77 KB, patch)
2012-04-04 15:48 PDT, Scott Johnson (:jwir3)
dbaron: review-
Details | Diff | Review
b708175 (v2) (17.91 KB, patch)
2012-04-09 16:16 PDT, Scott Johnson (:jwir3)
dbaron: review+
Details | Diff | Review

Description John Hammink 2011-12-06 20:26:11 PST
Created attachment 579579 [details]
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) 2011-12-08 10:55:14 PST
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) 2011-12-08 10:55:35 PST
s/cameria/camera
Comment 3 Kevin Brosnan [:kbrosnan] 2011-12-08 22:27:39 PST
*** Bug 708940 has been marked as a duplicate of this bug. ***
Comment 4 John Hammink 2011-12-12 09:42:26 PST
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 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-21 18:21:55 PST
*** Bug 712708 has been marked as a duplicate of this bug. ***
Comment 6 JP Rosevear [:jpr] 2012-02-23 19:25:57 PST
Release blockers.
Comment 7 Scott Johnson (:jwir3) 2012-03-01 14:36:19 PST
*** Bug 731825 has been marked as a duplicate of this bug. ***
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-03-02 11:37:01 PST
How does this look after the recent form control fixes?
Comment 9 JP Rosevear [:jpr] 2012-03-08 07:04:37 PST
(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] 2012-03-08 16:44:02 PST
(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] 2012-03-08 16:44:47 PST
Created attachment 604257 [details]
Screenshot of the fix
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-08 22:18:43 PST
can we close this bug?
Comment 13 Scott Johnson (:jwir3) 2012-03-08 22:49:47 PST
(In reply to Mark Finkle (:mfinkle) from comment #12)
> can we close this bug?

Yep. Closing as wfm.
Comment 14 Mounir Lamouri (:mounir) 2012-03-09 06:16:16 PST
Using a 2012-03-09 Nightly doesn't make this works for me on my test case.
Comment 15 JP Rosevear [:jpr] 2012-03-09 21:39:51 PST
(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) 2012-03-10 05:56:03 PST
As simple as:
data:text/html,<input type='file' accept='image/*'>
Comment 17 Brad Lassey [:blassey] (use needinfo?) 2012-03-12 14:16:22 PDT
*** Bug 721823 has been marked as a duplicate of this bug. ***
Comment 18 Brad Lassey [:blassey] (use needinfo?) 2012-03-20 16:06:08 PDT
morphing this bug to cover the more generic case
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-03-20 16:07:36 PDT
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-7 (review requests must explain patch) 2012-03-20 16:27:28 PDT
Also, maybe the selects at the top of http://crash-stats.mozilla.org/ ?
Comment 21 Mounir Lamouri (:mounir) 2012-03-21 03:00:06 PDT
(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] 2012-03-23 08:32:56 PDT
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) 2012-03-27 16:40:39 PDT
Created attachment 609948 [details]
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) 2012-04-04 15:48:11 PDT
Created attachment 612376 [details] [diff] [review]
b708175
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-04-05 16:50:57 PDT
Comment on attachment 612376 [details] [diff] [review]
b708175

># HG changeset patch
># Parent 869edbbfad819b65543cf38ada8d1a8e1005cdf9
># User Scott Johnson <sjohnson@mozilla.com>
>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-7 (review requests must explain patch) 2012-04-06 15:59:04 PDT
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-7 (review requests must explain patch) 2012-04-06 15:59:45 PDT
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) 2012-04-09 16:16:02 PDT
Created attachment 613427 [details] [diff] [review]
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-7 (review requests must explain patch) 2012-04-09 16:39:41 PDT
Comment on attachment 613427 [details] [diff] [review]
b708175 (v2)

r=dbaron
Comment 30 Scott Johnson (:jwir3) 2012-04-11 09:15:22 PDT
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) 2012-04-11 16:13:29 PDT
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d41ddb6b9bc
Comment 32 Matt Brubeck (:mbrubeck) 2012-04-11 20:12:01 PDT
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 34 :Ehsan Akhgari (out sick) 2012-04-12 10:36:29 PDT
https://hg.mozilla.org/mozilla-central/rev/6d41ddb6b9bc
Comment 35 :Ehsan Akhgari (out sick) 2012-04-12 10:52:31 PDT
This is backed out.
Comment 36 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-04-12 15:30:25 PDT
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) 2012-04-12 15:43:38 PDT
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 2012-04-12 17:45:24 PDT
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/sjohnson@mozilla.com-854537415763
Comment 39 Scott Johnson (:jwir3) 2012-04-13 00:36:41 PDT
Re-landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1587faab5e5c
Comment 40 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-13 10:41:02 PDT
Based on the effect of this bug on form controls, we'd like to keep this a beta blocker
Comment 41 Scott Johnson (:jwir3) 2012-04-13 11:22:58 PDT
(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] 2012-04-14 06:21:14 PDT
https://hg.mozilla.org/mozilla-central/rev/1587faab5e5c
Comment 43 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-04-16 15:35:39 PDT
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) 2012-04-16 15:51:07 PDT
I've submitted bug 745993 to track this issue.
Comment 45 Marco Bonardo [::mak] 2012-04-17 07:41:54 PDT
https://hg.mozilla.org/mozilla-central/rev/554a06caebf2
Comment 46 Cristian Nicolae (:xti) 2012-05-23 09:34:05 PDT
Bug 748434 was filled regarding this issue. Due to this, I will close the bug as verified fixed.

Note You need to log in before you can comment on or make changes to this bug.