Closed Bug 705446 Opened 8 years ago Closed 8 years ago

text rendering too large for preformatted text

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 13
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +
fennec 11+ ---

People

(Reporter: automatedtester, Assigned: jwir3)

References

()

Details

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

Attachments

(1 file, 3 obsolete files)

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.
Not a dupe of bug 705278?
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
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
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.
Works for me using today's birch build (20111202) in a Samsung Galaxy Tab 10.1 and Android 3.1.
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
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.
Whiteboard: readablity → readability, [font-inflation: pre]
+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: nobody → sjohnson
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.
Release blockers.
Priority: P2 → P1
blocking-fennec1.0: --- → +
Status: NEW → ASSIGNED
Attached patch b705446 - Proposed patch (obsolete) — Splinter Review
Disabling font inflation in <pre> and <code> tags.
Attachment #601814 - Flags: review?(dbaron)
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-
(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?
(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?
Attached patch b705446 (v2) - Proposed patch (obsolete) — Splinter Review
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-
Attached patch b705446 (v3) - Proposed patch (obsolete) — Splinter Review
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-
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+
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4006a8627b6
Target Milestone: --- → Firefox 13
Duplicate of this bug: 732539
Merged to central this morning:
https://hg.mozilla.org/mozilla-central/rev/e4006a8627b6
Status: ASSIGNED → RESOLVED
Closed: 8 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
You need to log in before you can comment on or make changes to this bug.