text rendering too large for preformatted text

VERIFIED FIXED in Firefox 14

Status

()

Firefox for Android
General
P1
normal
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: automatedtester, Assigned: jwir3)

Tracking

unspecified
Firefox 13
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox14 verified, firefox15 verified, blocking-fennec1.0 +, fennec11+)

Details

(Whiteboard: readability, [font-inflation: pre], URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
When looking at code on Github with Fennec on Nexus S the text is about 400%+ bigger than the rest of the text on the page. It cannot be zoomed out of. Only the page zooms out and still looks large in comparison.
Blocks: 627842
Not a dupe of bug 705278?
Not necessarily.
We should probably turn off inflation for preformatted blocks.

Also, we should constrain the width-of-block used for the inflation calculations to be no longer the width of the frame that the block is in.  (Not sure if that's relevant here, but I realized I meant to do that but didn't.)
Whiteboard: readablity

Comment 4

6 years ago
m.facebook.com is hit particularly severely by this issue- some text is HUGE, other is too tiny to read.
Assignee: nobody → dbaron
Priority: -- → P2

Comment 5

6 years ago
Is this the same issue I have been seeing with pages in the FLOSS squirrelmail webmail, where they use a frameset with the left frame being the folder list and the right being either message list or message display, and in the message display the font now gets large enough to overflow the frame and I need to pan right and left to read the text?
If this is not the same, I'm happy to file another bug, it's definitely caused by the new font size inflation code in any case.

Comment 6

6 years ago
Works for me using today's birch build (20111202) in a Samsung Galaxy Tab 10.1 and Android 3.1.
(Reporter)

Comment 7

6 years ago
On my Nexus S and on my Transformer it is still an issue.

Github code is a good test for the larger text. I am using the same version as :Gabriela
(Reporter)

Comment 8

6 years ago
https://github.com/travis-ci/travis-hub/blob/master/lib/travis/hub.rb is a good example of it not rendering correctly as well as the one I added when creating this bug.
(Assignee)

Updated

6 years ago
Whiteboard: readablity → readability, [font-inflation: pre]
Another one: http://wikipedia.org

Comment 10

6 years ago
+1 on wikipedia.org  I've noticed the same issue on today's build.
This bug is about what's described in comment 0; many of the other problems mentioned are probably better off reported as other bugs.
Assignee: dbaron → nobody
tracking-fennec: --- → 11+
(Assignee)

Updated

6 years ago
Assignee: nobody → sjohnson

Comment 12

6 years ago
What I've observed about this bug is that text appears too large tends NOT to be within UL, table tags and form fields. It wouldn't be so bad if all the text was too large but when some text is wicked large and the rest is really tiny, it's a serious usability issue.
Keywords: fennecnative-releaseblocker

Comment 13

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

Comment 14

6 years ago
Created attachment 601814 [details] [diff] [review]
b705446 - Proposed patch

Disabling font inflation in <pre> and <code> tags.
Attachment #601814 - Flags: review?(dbaron)
(Assignee)

Updated

6 years ago
Summary: text rendering extremely large in certain parts of the page → text rendering too large for preformatted text
Comment on attachment 601814 [details] [diff] [review]
b705446 - Proposed patch

You shouldn't check tag names (and if you did, you should compare atom pointers -- the point of atoms is that there's only one atom for a given string).

Instead, look at frame->GetStyleText()->mWhiteSpace
Attachment #601814 - Flags: review?(dbaron) → review-
(Assignee)

Comment 16

6 years ago
(In reply to David Baron [:dbaron] from comment #15)
> Comment on attachment 601814 [details] [diff] [review]
> b705446 - Proposed patch
> 
> You shouldn't check tag names (and if you did, you should compare atom
> pointers -- the point of atoms is that there's only one atom for a given
> string).
> 
> Instead, look at frame->GetStyleText()->mWhiteSpace

Yes, I originally used the 'IsWhitespaceSignificant() and IsNewlineSignificant()', but these also seem to be true in <textarea>s?
(Assignee)

Comment 17

6 years ago
(In reply to Scott Johnson (:jwir3) from comment #16)
> Yes, I originally used the 'IsWhitespaceSignificant() and
> IsNewlineSignificant()', but these also seem to be true in <textarea>s?

And, it seems, in <input type="text"> elements. Here is the original code I had to detect if preformatted text is the font inflation container:

  const nsStyleText* textStyle = aFrame->GetStyleText();
  bool preformatted = false;
  if (textStyle) {
    preformatted = textStyle->NewlineIsSignificant();
    preformatted = preformatted || textStyle->WhiteSpaceIsSignificant();
  }

Is this more what you were thinking, David?
(Assignee)

Comment 18

6 years ago
Created attachment 601840 [details] [diff] [review]
b705446 (v2) - Proposed patch

Take two on this patch - I'm using the style information as suggested, but I do a similar check for input using nsStyleUserInterface->mUserInput to verify it's not a frame representing either a <input type="text"> or <textarea> element.
Attachment #601814 - Attachment is obsolete: true
Attachment #601840 - Flags: review?(dbaron)
Oh, no, it's not what I was thinking -- you should be patching ShouldInflateFontsForContainer.  Then you'll be testing the frame that's the container rather than its text frame descendant, which I think is the right thing, and you won't need to worry about text inputs (since they're never containers).  (And you actually do want to inflate <code> in the typical case, since it's in the middle of a run of other text.)
Comment on attachment 601840 [details] [diff] [review]
b705446 (v2) - Proposed patch

see comment 19
Attachment #601840 - Flags: review?(dbaron) → review-
(Assignee)

Comment 21

6 years ago
Created attachment 602027 [details] [diff] [review]
b705446 (v3) - Proposed patch

Third time is a charm (hopefully).
Attachment #601840 - Attachment is obsolete: true
Attachment #602027 - Flags: review?(dbaron)
Comment on attachment 602027 [details] [diff] [review]
b705446 (v3) - Proposed patch

Closer, except there's no reason you can't just add an && to the boolean expression that's already there.

But while you're doing it, factor out the aFrame->GetStyleText() to a variable above that expression, since you'll be changing it from used once to used twice.  (I'd call it styleText rather than textStyle just since that's a more common pattern.)
Attachment #602027 - Flags: review?(dbaron) → review-
(Assignee)

Comment 23

6 years ago
Created attachment 602039 [details] [diff] [review]
b705446 (v4) - Proposed patch
Attachment #602027 - Attachment is obsolete: true
Attachment #602039 - Flags: review?(dbaron)
Comment on attachment 602039 [details] [diff] [review]
b705446 (v4) - Proposed patch

># HG changeset patch
># Parent 1c3b291d08306cf3535e1a1f262ce8567f99e218
># User Scott Johnson <sjohnson@mozilla.com>
>Bug 705446: Disable font inflation for preformatted text.
>
>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>--- a/layout/base/nsLayoutUtils.cpp
>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -4711,19 +4711,26 @@ ShouldInflateFontsForContainer(const nsI
> {
>   // We only want to inflate fonts for text that is in a place
>   // with room to expand.  The question is what the best heuristic for
>   // that is...
>   // For now, we're going to use NS_FRAME_IN_CONSTRAINED_HEIGHT, which
>   // indicates whether the frame is inside something with a constrained
>   // height (propagating down the tree), but the propagation stops when
>   // we hit overflow-y: scroll or auto.
>-  return aFrame->GetStyleText()->mTextSizeAdjust !=
>-           NS_STYLE_TEXT_SIZE_ADJUST_NONE &&
>-         !(aFrame->GetStateBits() & NS_FRAME_IN_CONSTRAINED_HEIGHT);
>+  const nsStyleText* styleText = aFrame->GetStyleText();
>+
>+  bool shouldInflate = styleText->mTextSizeAdjust !=
>+                       NS_STYLE_TEXT_SIZE_ADJUST_NONE &&
>+                       !(aFrame->GetStateBits() &
>+                         NS_FRAME_IN_CONSTRAINED_HEIGHT) &&
>+                         // We also want to disable font inflation for containers that have
>+                         // preformatted text.
>+                         !(styleText->WhiteSpaceIsSignificant());
>+  return shouldInflate;

No need for |shouldInflate| -- just go back to |return| and fix the indentation back so that styleText->mTextSizeAdjust, !(aFrame->GetStateBits(), and !(styleText-WhiteSpaceIsSignificant() all start at the same points (but other things are more indented).  Also don't parenthesize !styleText->WhiteSpaceIsSignificant().

Finally (I meant to mention this last time, but forgot), you really want to be testing WhiteSpaceCanWrap() rather than !WhiteSpaceIsSignificant().  Because what matters here is that it's text that we can't rewrap.

r=dbaron with that
Attachment #602039 - Flags: review?(dbaron) → review+
(Assignee)

Comment 25

6 years ago
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4006a8627b6
Target Milestone: --- → Firefox 13
(Assignee)

Updated

6 years ago
Duplicate of this bug: 732539
(Assignee)

Comment 27

6 years ago
Merged to central this morning:
https://hg.mozilla.org/mozilla-central/rev/e4006a8627b6

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 732290
Duplicate of this bug: 728560
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-28)
Firefox 14.0a2 (2012-05-28)

Device: Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
status-firefox14: --- → verified
status-firefox15: --- → verified
You need to log in before you can comment on or make changes to this bug.