nsBlockFrame.cpp has this GetSkipSides check right now: > 907 if (GetSkipSides() & NS_SIDE_TOP) { > 908 heightExtras.top = 0; However -- NS_SIDE_TOP is the value *zero*, as asserted here: > 1458 MOZ_STATIC_ASSERT(NS_SIDE_TOP == 0 && NS_SIDE_RIGHT == 1 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#1458 So that check in nsBlockFrame is currently checking "if (GetSkipSides() & 0)", which is always false, since anything & 0 is false. It **really** wants to be checking GetSkipSides() & (1 << NS_SIDE_TOP)) -- GetSkipSides returns its value as a bitfield, with the bits shifted by NS_SIDE_* values.

The line in question is here: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=14929e61959d#907 and it looks like it was added in http://hg.mozilla.org/mozilla-central/rev/6f0219923a28#l1.44 for Bug 626395.

The lines in question has now changed to: >1043 if (GetLogicalSkipSides() & (LOGICAL_SIDE_B_START)) { >1044 blockDirExtras.BStart(wm) = 0; http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=8bc5711c589e#1043 and the LOGICAL_SIDE_* values are side *bits* confusingly enough... so this is now fixed. (I'll address this in general in bug 1028460 so that any misuse will fail to compile.)

(In reply to Mats Palmgren (:mats) from comment #2) > and the LOGICAL_SIDE_* values are side *bits* confusingly enough... Why is that confusing? I did it that way because I thought bits were *less* confusing than shift values, but if I'm missing something it can always change back.

(In reply to Simon Montagu :smontagu from comment #3) > Why is that confusing? I did it that way because I thought bits were *less* > confusing than shift values, but if I'm missing something it can always > change back. I prefer bits as well, but the *naming* makes it error prone because: NS_SIDE_TOP -- a bit position, usage for skip-sides: (1 << NS_SIDE_TOP) LOGICAL_SIDE_B_START -- an error if used for shifting yet, they both have *_SIDE_* in the name so it's easy to make the mistake that former is physical, the latter is logical, but otherwise the same thing, i.e. to be used for shifting. See bug 1028460 for an example of that. In that bug, I'm changing the 'int' type to a proper SkipSides type so that making such a mistake will cause a compile error. And perhaps soon (as writing-mode changes makes margins etc logical) we won't need the physical GetSkipSides(), so maybe renaming the values isn't needed at this point.