Last Comment Bug 173354 - [FIX]margins are not computed correctly when they are set to % values
: [FIX]margins are not computed correctly when they are set to % values
Status: VERIFIED FIXED
: testcase
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 42417
  Show dependency treegraph
 
Reported: 2002-10-08 15:28 PDT by Vladimir Ermakov
Modified: 2002-10-26 01:09 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (962 bytes, text/html)
2002-10-08 15:28 PDT, Vladimir Ermakov
no flags Details
testcase w/o typos (961 bytes, text/html)
2002-10-08 15:31 PDT, Vladimir Ermakov
no flags Details
Testcase #2 (985 bytes, text/html)
2002-10-08 16:14 PDT, Mats Palmgren (vacation)
no flags Details
flushing helps make things haaaaapppppy (2.04 KB, patch)
2002-10-08 23:03 PDT, Boris Zbarsky [:bz]
caillon: review+
Details | Diff | Splinter Review
To inline or not, that is the question. I say not. (8.00 KB, patch)
2002-10-08 23:23 PDT, Boris Zbarsky [:bz]
caillon: review+
hjtoi-bugzilla: superreview+
asa: approval+
Details | Diff | Splinter Review

Description Vladimir Ermakov 2002-10-08 15:28:28 PDT
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 2002-10-08 15:28:55 PDT
Created attachment 102242 [details]
testcase
Comment 2 Vladimir Ermakov 2002-10-08 15:31:09 PDT
Created attachment 102243 [details]
testcase w/o typos

first one did not label properties properly.
Comment 3 Boris Zbarsky [:bz] 2002-10-08 16:13:33 PDT
Hmmm... we just use CalcMarginWidthFor() here...

Could that be broken?
Comment 4 Mats Palmgren (vacation) 2002-10-08 16:14:52 PDT
Created attachment 102257 [details]
Testcase #2

You get different results after the page is fully loaded.
Comment 5 Christopher Aillon (sabbatical, not receiving bugmail) 2002-10-08 16:23:31 PDT
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 2002-10-08 16:34:02 PDT
The values are 0 if the border of the outside DIV are removed.
Comment 7 Boris Zbarsky [:bz] 2002-10-08 16:35:03 PDT
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 (vacation) 2002-10-08 16:50:53 PDT
You're welcome. In return, maybe you can explain why 38% of 100px = 37.9375px ?
Comment 9 Boris Zbarsky [:bz] 2002-10-08 17:00:51 PDT
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) 2002-10-08 17:09:06 PDT
Err, I got confused.  Too much "spec-ulating" today about undefined behaviors
has gotten me out of whack.
Comment 11 Boris Zbarsky [:bz] 2002-10-08 23:03:04 PDT
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.
Comment 12 Christopher Aillon (sabbatical, not receiving bugmail) 2002-10-08 23:12:26 PDT
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 [:bz] 2002-10-08 23:23:31 PDT
Created attachment 102305 [details] [diff] [review]
To inline or not, that is the question.  I say not.
Comment 14 Christopher Aillon (sabbatical, not receiving bugmail) 2002-10-09 01:56:33 PDT
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) 2002-10-09 15:53:58 PDT
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] 2002-10-15 15:50:58 PDT
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 [:bz] 2002-10-15 20:49:50 PDT
fixed.
Comment 18 rbs 2002-10-15 21:56:59 PDT
   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 2002-10-18 19:05:02 PDT
Looks fixed. Verifying. (build 2002-10-18-08-trunk)
Comment 20 Boris Zbarsky [:bz] 2002-10-26 00:02:43 PDT
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 2002-10-26 00:25:41 PDT
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 [:bz] 2002-10-26 01:09:26 PDT
> 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.

Note You need to log in before you can comment on or make changes to this bug.