Closed
Bug 705446
Opened 13 years ago
Closed 12 years ago
text rendering too large for preformatted text
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox14 verified, firefox15 verified, blocking-fennec1.0 +, fennec11+)
VERIFIED
FIXED
Firefox 13
People
(Reporter: automatedtester, Assigned: jwir3)
References
()
Details
(Whiteboard: readability, [font-inflation: pre])
Attachments
(1 file, 3 obsolete files)
4.41 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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: font-inflation
Comment 1•13 years ago
|
||
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.)
Updated•13 years ago
|
Whiteboard: readablity
m.facebook.com is hit particularly severely by this issue- some text is HUGE, other is too tiny to read.
Updated•13 years ago
|
Assignee: nobody → dbaron
Priority: -- → P2
Comment 5•13 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•13 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•13 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•13 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•13 years ago
|
Whiteboard: readablity → readability, [font-inflation: pre]
Comment 9•13 years ago
|
||
Another one: http://wikipedia.org
Comment 10•13 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
Updated•12 years ago
|
tracking-fennec: --- → 11+
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → sjohnson
Comment 12•12 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.
Updated•12 years ago
|
Keywords: fennecnative-releaseblocker
Updated•12 years ago
|
blocking-fennec1.0: --- → +
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•12 years ago
|
||
Disabling font inflation in <pre> and <code> tags.
Attachment #601814 -
Flags: review?(dbaron)
Assignee | ||
Updated•12 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•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e4006a8627b6
Target Milestone: --- → Firefox 13
Assignee | ||
Comment 27•12 years ago
|
||
Merged to central this morning: https://hg.mozilla.org/mozilla-central/rev/e4006a8627b6
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 30•12 years ago
|
||
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
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•