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

VERIFIED FIXED

Status

()

Core
DOM: CSS Object Model
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Vladimir Ermakov, Assigned: bz)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

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

Comment 1

15 years ago
Created attachment 102242 [details]
testcase
(Reporter)

Comment 2

15 years ago
Created attachment 102243 [details]
testcase w/o typos

first one did not label properties properly.
Hmmm... we just use CalcMarginWidthFor() here...

Could that be broken?
Assignee: jst → bzbarsky
Blocks: 42417
OS: Windows 2000 → All
Hardware: PC → All
Created attachment 102257 [details]
Testcase #2

You get different results after the page is fully loaded.
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...
(Reporter)

Comment 6

15 years ago
The values are 0 if the border of the outside DIV are removed.
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!
You're welcome. In return, maybe you can explain why 38% of 100px = 37.9375px ?
Keywords: testcase
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...
Err, I got confused.  Too much "spec-ulating" today about undefined behaviors
has gotten me out of whack.
Created attachment 102304 [details] [diff] [review]
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.
Attachment #102304 - Flags: review+
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.
Created attachment 102305 [details] [diff] [review]
To inline or not, that is the question.  I say not.
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.
Attachment #102305 - Flags: review+
Comment on attachment 102305 [details] [diff] [review]
To inline or not, that is the question.  I say not.

sr=heikki
Attachment #102305 - Flags: superreview+
(Assignee)

Updated

15 years ago
Summary: margins are not computed correctly when they are set to % values → [FIX]margins are not computed correctly when they are set to % values

Comment 16

15 years ago
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)
Attachment #102305 - Flags: approval+
fixed.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 18

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

Comment 19

15 years ago
Looks fixed. Verifying. (build 2002-10-18-08-trunk)
Status: RESOLVED → VERIFIED
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

15 years ago
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.
> 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.
You need to log in before you can comment on or make changes to this bug.