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

RESOLVED FIXED in mozilla14

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: John Hammink, Assigned: jwir3)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 beta+, fennec11+)

Details

(Whiteboard: readability, URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Component: DOM → General
Product: Core → Fennec Native
QA Contact: general → general
Target Milestone: mozilla11 → ---
Version: 11 Branch → unspecified
Blocks: 659188
Summary: Web API: Native fennec appears to truncate button text on test page → Native fennec truncates file control buttons (with Camera API enabled)

Comment 1

6 years ago
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

6 years ago
s/cameria/camera
Whiteboard: readability
Blocks: 627842
Duplicate of this bug: 708940
(Reporter)

Comment 4

6 years ago
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.
Assignee: nobody → dbaron
Priority: -- → P2
Duplicate of this bug: 712708
tracking-fennec: --- → 11+

Updated

5 years ago
Blocks: 721823
Keywords: fennecnative-releaseblocker

Comment 6

5 years ago
Release blockers.
Priority: P2 → P1
Depends on: 730289
blocking-fennec1.0: --- → +
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 731825
How does this look after the recent form control fixes?

Updated

5 years ago
Assignee: dbaron → sjohnson

Comment 9

5 years ago
(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

5 years ago
(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

5 years ago
Created attachment 604257 [details]
Screenshot of the fix
can we close this bug?
(Assignee)

Comment 13

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

Yep. Closing as wfm.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
Using a 2012-03-09 Nightly doesn't make this works for me on my test case.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 15

5 years ago
(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?
As simple as:
data:text/html,<input type='file' accept='image/*'>
Duplicate of this bug: 721823
morphing this bug to cover the more generic case
Status: REOPENED → NEW
Summary: Native fennec truncates file control buttons (with Camera API enabled) → fixed width buttons shouldn't have their text inflated
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.
Also, maybe the selects at the top of http://crash-stats.mozilla.org/ ?
Summary: fixed width buttons shouldn't have their text inflated → fixed width buttons (or form controls?) shouldn't have their text inflated
(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.

Updated

5 years ago
blocking-fennec1.0: + → beta+

Comment 22

5 years ago
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.
(Assignee)

Comment 23

5 years ago
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.

Updated

5 years ago
Blocks: 741393
(Assignee)

Comment 24

5 years ago
Created attachment 612376 [details] [diff] [review]
b708175
Attachment #612376 - Flags: review?(dbaron)
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...
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 on attachment 612376 [details] [diff] [review]
b708175

I'd probably like to take a brief look at the revisions (see above), so marking review-.
Attachment #612376 - Flags: review?(dbaron) → review-
(Assignee)

Comment 28

5 years ago
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!
Attachment #612376 - Attachment is obsolete: true
Attachment #613427 - Flags: review?(dbaron)
(Assignee)

Updated

5 years ago
Summary: fixed width buttons (or form controls?) shouldn't have their text inflated → Form controls with fixed width or height shouldn't have their text inflated
Comment on attachment 613427 [details] [diff] [review]
b708175 (v2)

r=dbaron
Attachment #613427 - Flags: review?(dbaron) → review+
(Assignee)

Comment 30

5 years ago
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.
(Assignee)

Comment 31

5 years ago
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d41ddb6b9bc
Target Milestone: --- → Firefox 14
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
Target Milestone: Firefox 14 → ---
https://hg.mozilla.org/mozilla-central/rev/6d41ddb6b9bc
https://hg.mozilla.org/mozilla-central/rev/412d3777596a
https://hg.mozilla.org/mozilla-central/rev/6d41ddb6b9bc
Status: NEW → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
This is backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Target Milestone: Firefox 14 → ---
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.
(Assignee)

Comment 37

5 years ago
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

5 years ago
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
(Assignee)

Comment 39

5 years ago
Re-landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1587faab5e5c
Target Milestone: --- → mozilla14
Based on the effect of this bug on form controls, we'd like to keep this a beta blocker
(Assignee)

Comment 41

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/1587faab5e5c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
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
(Assignee)

Updated

5 years ago
Blocks: 745993
(Assignee)

Comment 44

5 years ago
I've submitted bug 745993 to track this issue.
https://hg.mozilla.org/mozilla-central/rev/554a06caebf2
Depends on: 748434
Bug 748434 was filled regarding this issue. Due to this, I will close the bug as verified fixed.
Depends on: 757937
You need to log in before you can comment on or make changes to this bug.