173354 - [FIX]margins are not computed correctly when they are set to % values

Status: VERIFIED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Vladimir Ermakov]

if you set margins to % values (margin-right: 38%) when computed using
getComputedStyle the value is not correct, even though it is layed out correctly.
See testcase.

Comment 1

[Vladimir Ermakov]

Attachment: testcase

Comment 2

[Vladimir Ermakov]

Attachment: testcase w/o typos

first one did not label properties properly.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmmm... we just use CalcMarginWidthFor() here...

Could that be broken?

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #2

You get different results after the page is fully loaded.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Actually, I think that this bug is partially invalid.  We are taking a
percentage of an auto margin.  That auto margin will not be computed until later
on.  Certainly not until we have finished loading the body (which Mats has shown).

That said, we should certainly not be returning negative values...

Comment 6

[Vladimir Ermakov]

The values are 0 if the border of the outside DIV are removed.

Comment 7

[Boris Zbarsky [:bzbarsky]]

You mean we're taking a percentage of a _width_ right?  Look at the definition
of %-values for margins...

Perhaps all we need to do is flush before computing margins (in
nsComputedDOMStyle::GetMarginWidthFor).  We don't do that now...

I'll try that and see what happens, unless caillon beats me to it.

Mats, thanks for the testcase!

Comment 8

[Mats Palmgren (inactive)]

You're welcome. In return, maybe you can explain why 38% of 100px = 37.9375px ?

Comment 9

[Boris Zbarsky [:bzbarsky]]

It's 37.9286 for me.  Basically, the problem is that layout does all its
calculations in twips, using integer arithmetic.  There are 20 twips per point.
 So at 72 dpi, there are 20 twips per pixel.  I'm at 100dpi.  I think we force
it up to a So there are 72/100*20 = 14.4 twips per pixel.  

Now 100px == 1440 twips (on my machine).  38% of that is 547.2 twips, so layout
will have a value of 547 twips for that length (integers, remember).  Now we
convert back.  547/14.4 != 38, since we rounded off that 0.2 twip...

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Err, I got confused.  Too much "spec-ulating" today about undefined behaviors
has gotten me out of whack.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: flushing helps make things haaaaapppppy

The first hunk just fixes some debug-me-or-caillon bustage. The other two are
actually relevant to this bug.

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 102304 [details] [diff] [review]
flushing helps make things haaaaapppppy

I swear, the first part really compiles for me!  :-)

r=caillon for the patch, though we should probably consider adding a private
void FlushPendingNotifications() to our class, since we do this exact same
thing in several places.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: To inline or not, that is the question. I say not.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 102305 [details] [diff] [review]
To inline or not, that is the question.  I say not.

Right, we shouldn't inline in this case.  r=caillon, thanks for doing the extra
work.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 102305 [details] [diff] [review]
To inline or not, that is the question.  I say not.

sr=heikki

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 102305 [details] [diff] [review]
To inline or not, that is the question.  I say not.

a=asa for checkin to 1.2beta or final (on behalf of drivers)

Comment 17

[Boris Zbarsky [:bzbarsky]]

fixed.

Comment 18

[rbs]

   const nsStyleText *text = nsnull;
   GetStyleData(eStyleStruct_Text, (const nsStyleStruct*&)text, aFrame);
 
-  // Flush all pending notifications so that our frames are up to date
-  nsCOMPtr<nsIDocument> document;
-  mContent->GetDocument(*getter_AddRefs(document));
-  if (document) {
-    document->FlushPendingNotifications();
-  }
+  FlushPendingReflows();

[paranoia] Might have been safer/consistent to flush before GetStyleData()
everywhere.

Comment 19

[Vladimir Ermakov]

Looks fixed. Verifying. (build 2002-10-18-08-trunk)

Comment 20

[Boris Zbarsky [:bzbarsky]]

rbs, are there really times when flushing will affect what GetStyleData returns?
 I can see frame construction forcing it, I guess; I see, to recall that table
code does something funky... is that what you meant?

Comment 21

[rbs]

Yep that's what I meant. If the flush didn't affect the styledata, you wouldn't
have got this bug in the first place.

Since what you found was that the flush was required to guarantee up-to-date
style data, it may happen that the flush may then hit hard on the style data --
possibly moving from a shared styledata slot to a brand new styledata slot.
Since it trivial to swap the order of the calls, I thought worthwhile to mention
it to avoid unforeseen inaccuracies -- albeit it is paranoia.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> If the flush didn't affect the styledata, you wouldn't have got this bug in the
> first place.

Sure we would have.  We're not actually looking at styledata here, but at frame
rects.  And flushes most certainly affect those....

I agree with the paranoia; the next time I touch this code I'll reorder the calls.