Closed Bug 380825 Opened 16 years ago Closed 16 years ago

Font sizes on Tinderbox are too large

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: RyanVM, Assigned: dbaron)

References

()

Details

Attachments

(3 files, 2 obsolete files)

With the new textframe enabled, the fonts in the main Tinderbox table (showing the build stats) are larger than they are without the new textframe.
Flags: blocking1.9?
Summary: Font sizes on Tinderbox are too large → Font sizes on Tinderbox are too large with new textframe
I'm actually seeing the same thing with regular Mac nightly builds.
That changed between 2007051504 (larger text size) and 2007051404

Something with text wrapped inside <tt>, in quirksmode only.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a5pre) Gecko/20070515 Minefield/3.0a5pre ID:2007051504 [cairo]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a5pre) Gecko/20070514 Minefield/3.0a5pre ID:2007051404 [cairo]
Attached file test case
the text in the second cell is wrapped in a <tt>
You're right, my timing for switching to the new textframe again was just really bad. It does indeed happen on the regular trunk too.

Switching components to the proper ones.
Component: GFX: Thebes → Server Operations: Tinderbox Maintenance
Flags: blocking1.9?
Product: Core → mozilla.org
Version: Trunk → other
Hopefully this is set properly now
Assignee: nobody → server-ops
OS: Windows XP → All
QA Contact: thebes → justin
Hardware: PC → All
Hopefully this is set properly now
Summary: Font sizes on Tinderbox are too large with new textframe → Font sizes on Tinderbox are too large
Tinderbox Maintenance is for tinderbox clients that fall over, while this is an
issue with the tinderbox server. You could maybe argue this should be in
Webtools::Tinderbox.
Component: Server Operations: Tinderbox Maintenance → Server Operations
How is this an issue with the Tinderbox server?  Please explain.
I made the assumption that Gecko became more conformant to standards to cause this, but that's premature. Apologies.

Could someone figure out the regression window using 
  http://hourly-archive.localgho.st/
I doubt this has anything to do with the TinderBox server(s). The attached test case shows the problem on my side.
Yes guys, I was being a retard. I'm finding the regression range now and I'll fix things accordingly.
Component: Server Operations → Server Operations: Tinderbox Maintenance
Yes guys, I was being a retard. I'm finding the regression range now and I'll fix things accordingly.
Per comments 9-11, This is not server ops.
Assignee: server-ops → nobody
Component: Server Operations: Tinderbox Maintenance → General
Product: mozilla.org → Firefox
QA Contact: justin → general
Version: other → Trunk
OK, the regression window is:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1179203220&maxdate=1179207239

dbaron, I'm guessing your checking for bug 377521 is the culprit?

Sorry for all the spam, guys.
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
Flags: blocking1.9?
Duplicate of this bug: 380849
Flags: blocking1.9? → blocking1.9+
So I'm puzzled as to why anything would be setting eCSSUnit_Initial values...
Something in the generic font code, perhaps?
Attached patch patchSplinter Review
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #265037 - Flags: superreview?(bzbarsky)
Attachment #265037 - Flags: review?(bzbarsky)
Comment on attachment 265037 [details] [diff] [review]
patch

Right. Fun...
Attachment #265037 - Flags: superreview?(bzbarsky)
Attachment #265037 - Flags: superreview+
Attachment #265037 - Flags: review?(bzbarsky)
Attachment #265037 - Flags: review+
Checked in to trunk.

I never did figure out why we were getting eCSSUnit_Initial, but I wouldn't be surprised if it goes away after bug 380915.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Anyone have a suggestion for a reftest for this? I was trying to make one with a table like in philippe's testcase and just a lone <tt>Hello World</tt>, but the indentation (from the left of the page) differs for a table.
Flags: in-testsuite?
I was thinking about making one using <pre> for the reference and <tt> for the test, but I haven't actually attempted it to see if there's visual parity.
My idea is comment #21 doesn't work. <pre> has the same issues <tt> has, not to mention that with <pre>, a newline apparently is inserted after the text even though I didn't ask for one.
Attached patch Reftest (obsolete) — Splinter Review
Simple reftest for this bug
Attachment #265089 - Flags: review?
I guess <tt> could have been used instead of <pre> for the reference, but I doubt it really matters either way. dbaron, what's your take?
I don't see how this tests the bug, which is related to crossing the monospace and non-monospace font size preferences.  I'd think to test that you'd want to use a mochitest, and actually set (and then reset) the preferences during the test.
I don't understand. The reference renders the same both before and after this bug regressed and after the fix landed. The test renders the same before the regression and after the fix, but not while it was regressed. In other words, it fails during the regression range and passes now that the bug is fixed. Isn't that the point of a reftest? I understand that a mochitest may be useful, but it seems to me that the reftest is still testing the specific regression pointed out in this bug.
Not sure why they'd render differently even with the bug, but ok.
Ah, right, the initial came from quirks.css, which has table { font-size: -moz-initial; }
Do you want me to submit a patch with the reference case using <tt> instead of <pre>? I know it shouldn't really matter, but I'm thinking it would be more consistent.
Attachment #265089 - Flags: review? → review?(dbaron)
Comment on attachment 265089 [details] [diff] [review]
Reftest

OK, if you add margin:0 to both table and pre, r=dbaron.
Attachment #265089 - Flags: review?(dbaron) → review+
Attached patch Address dbaron's comments (obsolete) — Splinter Review
Same as the last reftest, but including a style="margin: 0" on the <table> and <pre> elements. Carrying over previous r+.
Attachment #265089 - Attachment is obsolete: true
Attachment #304667 - Flags: review+
Forgot to mention, reftest.list will need to be updated as well at the time of checkin.
Keywords: checkin-needed
Status: RESOLVED → VERIFIED
Stupid DOS line endings...
Attachment #304667 - Attachment is obsolete: true
Attachment #304668 - Flags: review+
RCS file: /cvsroot/mozilla/layout/reftests/bugs/380825-1-ref.html,v
done
Checking in layout/reftests/bugs/380825-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/380825-1-ref.html,v  <--  380825-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/380825-1.html,v
done
Checking in layout/reftests/bugs/380825-1.html;
/cvsroot/mozilla/layout/reftests/bugs/380825-1.html,v  <--  380825-1.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.360; previous revision: 1.359
done
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.