Font sizes on Tinderbox are too large

VERIFIED FIXED

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: RyanVM, Assigned: dbaron)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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?
(Reporter)

Updated

11 years ago
Summary: Font sizes on Tinderbox are too large → Font sizes on Tinderbox are too large with new textframe

Comment 1

11 years ago
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]

Comment 2

11 years ago
Created attachment 264989 [details]
test case

the text in the second cell is wrapped in a <tt>
(Reporter)

Comment 3

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

Comment 4

11 years ago
Hopefully this is set properly now
Assignee: nobody → server-ops
OS: Windows XP → All
QA Contact: thebes → justin
Hardware: PC → All
(Reporter)

Comment 5

11 years ago
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/

Comment 9

11 years ago
I doubt this has anything to do with the TinderBox server(s). The attached test case shows the problem on my side.
(Reporter)

Comment 10

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

Comment 11

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

Comment 13

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

Updated

11 years ago
Flags: blocking1.9?

Updated

11 years ago
Duplicate of this bug: 380849
(Assignee)

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 15

11 years ago
So I'm puzzled as to why anything would be setting eCSSUnit_Initial values...
Something in the generic font code, perhaps?
(Assignee)

Comment 17

11 years ago
Created attachment 265037 [details] [diff] [review]
patch
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+
(Assignee)

Comment 19

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 20

11 years ago
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?
(Reporter)

Comment 21

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

Comment 22

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

Comment 23

11 years ago
Created attachment 265089 [details] [diff] [review]
Reftest

Simple reftest for this bug
Attachment #265089 - Flags: review?
(Reporter)

Comment 24

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

Comment 25

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

Comment 26

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

Comment 27

11 years ago
Not sure why they'd render differently even with the bug, but ok.
(Assignee)

Comment 28

11 years ago
Ah, right, the initial came from quirks.css, which has table { font-size: -moz-initial; }
(Reporter)

Comment 29

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

Updated

10 years ago
Attachment #265089 - Flags: review? → review?(dbaron)
(Assignee)

Comment 30

10 years ago
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+
(Reporter)

Comment 31

10 years ago
Created attachment 304667 [details] [diff] [review]
Address dbaron's comments

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+
(Reporter)

Comment 32

10 years ago
Forgot to mention, reftest.list will need to be updated as well at the time of checkin.
Keywords: checkin-needed
(Reporter)

Updated

10 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 33

10 years ago
Created attachment 304668 [details] [diff] [review]
Same as v2, only with unix line endings

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.