380825 - Font sizes on Tinderbox are too large

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 1

[philippe (part-time)]

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

[philippe (part-time)]

Attachment: test case

the text in the second cell is wrapped in a <tt>

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Hopefully this is set properly now

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Hopefully this is set properly now

Comment 6

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 7

[Dave Miller [:justdave]]

How is this an issue with the Tinderbox server?  Please explain.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

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

[philippe (part-time)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Yes guys, I was being a retard. I'm finding the regression range now and I'll fix things accordingly.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Yes guys, I was being a retard. I'm finding the regression range now and I'll fix things accordingly.

Comment 12

[Dave Miller [:justdave]]

Per comments 9-11, This is not server ops.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I'm puzzled as to why anything would be setting eCSSUnit_Initial values...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Something in the generic font code, perhaps?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265037 [details] [diff] [review]
patch

Right. Fun...

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 20

[Adam Guthrie]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Attachment: Reftest

Simple reftest for this bug

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Not sure why they'd render differently even with the bug, but ok.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, right, the initial came from quirks.css, which has table { font-size: -moz-initial; }

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 265089 [details] [diff] [review]
Reftest

OK, if you add margin:0 to both table and pre, r=dbaron.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Attachment: 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+.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Forgot to mention, reftest.list will need to be updated as well at the time of checkin.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Attachment: Same as v2, only with unix line endings

Stupid DOS line endings...

Comment 34

[Reed Loden [:reed]]

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