Last Comment Bug 97361 - [MARGIN-C][FIX]WRMB:frames in the given url page could not be displayed correctly
: [MARGIN-C][FIX]WRMB:frames in the given url page could not be displayed corre...
Status: RESOLVED FIXED
checked into trunk,0.9.2 branch, 0.9....
: testcase, topembed
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: P2 normal (vote)
: mozilla0.9.5
Assigned To: Marc Attinasi
: Amarendra Hanumanula
:
Mentors:
http://www.perfumeisus.com/
Depends on:
Blocks: 99225
  Show dependency treegraph
 
Reported: 2001-08-28 14:28 PDT by Amarendra Hanumanula
Modified: 2001-09-06 21:17 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (198 bytes, text/html)
2001-08-28 17:33 PDT, Mats Palmgren (:mats)
no flags Details
PATCH: quirk.css rule to handle margin collapsing of empty P in body and TD (664 bytes, patch)
2001-08-30 14:27 PDT, Marc Attinasi
no flags Details | Diff | Splinter Review
PATCH: includes the :empty rules for the other margin-collapsing cases we already handled (also includes the changes for bug 97351) (2.48 KB, patch)
2001-09-04 14:34 PDT, Marc Attinasi
pierre: review+
hyatt: superreview+
asa: approval+
Details | Diff | Splinter Review

Description Amarendra Hanumanula 2001-08-28 14:28:33 PDT
The given Url http://www.perfumeisus.com/ page is generated by javascript 
document.write in frameset.. But the header frame could not be displayed 
completly

Steps to reproduce:

1.  Go to http://www.perfumeisus.com
2. upper left corner top header could not be displayed correctly
3. there is an 800 number listed which is missing on NS6.1
4. the drop down boxes are different shapes and the fonts are different, also

Build Id : 2001082803
Platform : all
Comment 1 Amarendra Hanumanula 2001-08-28 14:35:36 PDT
 Adding WRMB to summary and topmebed keyword
Comment 2 Mats Palmgren (:mats) 2001-08-28 17:33:57 PDT
Created attachment 47419 [details]
Testcase
Comment 3 Mats Palmgren (:mats) 2001-08-28 18:00:41 PDT
Collapsing margins for <P></P> does not seem to work. This is the BODY case,
bug 46230 is the same but for TD. The following quirk fixes it:
body p:empty:first-node, td p:empty:first-node, td p:empty:last-node {
  margin-top: 0;
  margin-bottom: 0;
}

Maybe we should do this for TH as well, see bug 96966.
Comment 4 Marc Attinasi 2001-08-30 11:02:52 PDT
The patch fixes this. I'm not sure I like using a style rule to do this (it
would be more efficient in the code), and I'm also not sure why this is proposed
as a QUIRK (shouldn;t this be happening in std mode too?) - but I'm happy to
take it, with a nice comment in the css file indicating that this should
probably be temporary. I'd like some clarification on the quirk /vs. standard
mode application of the rule though, if possible.
Comment 5 Marc Attinasi 2001-08-30 13:16:56 PDT
Updating various fields: patch tested and looks fine. Thanks so much Mats!
Comment 6 Marc Attinasi 2001-08-30 14:27:41 PDT
Created attachment 47704 [details] [diff] [review]
PATCH: quirk.css rule to handle margin collapsing of empty P in body and TD
Comment 7 David Baron :dbaron: ⌚️UTC-7 2001-08-30 14:35:39 PDT
I think you should change the descendant combinators to child combinators -
otherwise these changes could come at a significant performance cost.  The other
similar rules use child combinators.  If you make that change, r=dbaron.

We also (at some point) need to look at making the :first-node and :last-node
selectors perform better by storing the necessary information in
SelectorMatchesData *lazily*.
Comment 8 Pierre Saslawsky 2001-08-30 15:05:43 PDT
What David said...  r=pierre with:
  body > p:empty:first-node,
  td   > p:empty:first-node,
  td   > p:empty:last-node
Comment 9 Marc Attinasi 2001-08-30 17:33:59 PDT
OK, ready for 0.9.2 branch, nominating for 0.9.4 as well
Comment 10 Mats Palmgren (:mats) 2001-08-31 15:08:02 PDT
While you're at it, please have a look at bug 96966 too. Close it or make the
same kind of 
margin collapsing for TH as there is now for TD.
Comment 11 Mats Palmgren (:mats) 2001-08-31 15:25:42 PDT
You should probably also have a rule for all the other  elements with a default
 top- and
bottom-margin (blockquote, dl ...) as is currently done in quirk.css for
:first-node and :last-node.
Comment 12 Marc Attinasi 2001-08-31 15:42:41 PDT
Mats, I am not comfortable doubling the number of rules related to this margin
collapsing. Granted, these will not impact the chrome, I still worry about the
performance impact. When this is closed, I will open another bug to address the
rest of those rules - we will need a performance impact analysis I think.
regarding bug 96966: I have looked at it, but I cannot work on it right now as I
am restricted to 'topembed' and 0.9.4 bugs (I have a lot of them). I will get to
it when I can (other contributors can, of course, take it in the meantime - I
won't mind!) - sorry.
Comment 13 David Baron :dbaron: ⌚️UTC-7 2001-08-31 16:44:29 PDT
The performance effect of those rules is far less than a single td >
*:first-node since they're hashed by tag so we don't have to do a first-node
check on every single element in the document.  That's why I changed it to be
that way.
Comment 14 Marc Attinasi 2001-09-04 12:14:06 PDT
OK David, I understand that, but what about the impact of doubling the number of
the rules that you replaced the old one with? I understand why you made the
change that you made, but I'd liek to make sure we don't regress the performance
benefits you provided with your original change, that's my concern. If you are
extremely confident, then I'll do it, otherwse we need to measure I think.

Also, do you know if we are _suppossed_ to be doing this margin collapsing in
CSS rules or in code? For some reason, I always thought this was suppossed to be
done in code...
Comment 15 David Baron :dbaron: ⌚️UTC-7 2001-09-04 12:36:19 PDT
We do the margin-collapsing in code -- except our code is CSS-conformant (well,
mostly, but the deviations are bugs).  These rules are deviations from the
standard -- we're hiding margins that are supposed to show up because older
browsers did the same.  That's why they're in quirk.css.

It doesn't really matter to me, though, as long as you use child combinators
rather than descendant combinators.
Comment 16 Marc Attinasi 2001-09-04 14:31:21 PDT
Thanks for the clarification, David. Since there should be no performance
penalty due to the way the rules are hashed, I just took a few seconds to check
the size penaly of adding all of the rules. We go from 140,281 bytes to 147,209
bytes used by style rules. 7K is not much wo worry about, so I think we should
include the rules.

BTW: here are the style size dumps from before / after the new rules were put in
Quirks.css (loaded about:blank in viewer):

[BEFORE]
Frame Type           Count TotalSize MinSize MaxSize AvgSize
----------           ----- --------- ------- ------- -------
CSSDeclarationImpl     313     58724      92     556     187
CSSImportRuleImpl        3       292      94     103      97
CSSMediaRuleImpl         1        58      58      58      58
CSSNameSpaceRuleImp      3       273      80     113      91
CSSRuleProcessor         1        36      36      36      36
CSSStyleRuleImpl       636     27984      44      44      44
CSSStyleSheet            5       480      96      96      96
CSSStyleSheetInner       5       540     108     108     108
DocumentColorRule        1        32      32      32      32
HTMLCSSStyleSheet        1        32      32      32      32
HTMLStyleSheet           1        88      88      88      88
MappedAttrTable          1        44      44      44      44
RuleCascade              1       212     212     212     212
StyleSet                 1        84      84      84      84
TableTHRule              1        16      16      16      16
nsAttrSelector         172      7788      40      96      45
nsCSSSelector          911     42188      36     140      46
nsStyleContext          24      1410      52     102      58
*** Total ***         2081    140281

[AFTER]
Frame Type           Count TotalSize MinSize MaxSize AvgSize
----------           ----- --------- ------- ------- -------
CSSDeclarationImpl     315     59012      92     556     187
CSSImportRuleImpl        3       292      94     103      97
CSSMediaRuleImpl         1        58      58      58      58
CSSNameSpaceRuleImp      3       273      80     113      91
CSSRuleProcessor         1        36      36      36      36
CSSStyleRuleImpl       693     30492      44      44      44
CSSStyleSheet            5       480      96      96      96
CSSStyleSheetInner       5       540     108     108     108
DocumentColorRule        1        32      32      32      32
HTMLCSSStyleSheet        1        32      32      32      32
HTMLStyleSheet           1        88      88      88      88
MappedAttrTable          1        44      44      44      44
RuleCascade              1       212     212     212     212
StyleSet                 1        84      84      84      84
TableTHRule              1        16      16      16      16
nsAttrSelector         172      7788      40      96      45
nsCSSSelector         1025     46320      36     140      45
nsStyleContext          24      1410      52     102      58
*** Total ***         2254    147209
Comment 17 Marc Attinasi 2001-09-04 14:34:05 PDT
Created attachment 48217 [details] [diff] [review]
PATCH: includes the :empty rules for the other margin-collapsing cases we already handled (also includes the changes for bug 97351)
Comment 18 Marc Attinasi 2001-09-04 14:44:31 PDT
CC'ing Pierre: I'd like to get reviews for the changes I just attached please -
thanks.
Comment 19 Pierre Saslawsky 2001-09-04 14:58:58 PDT
Comment on attachment 48217 [details] [diff] [review]
PATCH: includes the :empty rules for the other margin-collapsing cases we already handled (also includes the changes for bug 97351)

r=pierre
Comment 20 Kevin McCluskey (gone) 2001-09-04 15:05:52 PDT
Updated status
Comment 21 David Hyatt 2001-09-04 17:00:10 PDT
Comment on attachment 48217 [details] [diff] [review]
PATCH: includes the :empty rules for the other margin-collapsing cases we already handled (also includes the changes for bug 97351)

sr=hyatt
Comment 22 David Baron :dbaron: ⌚️UTC-7 2001-09-05 08:11:11 PDT
Don't you need to swap 'margin-top' and 'margin-bottom' in the new rulesets that
you added?  (Otherwise they won't add anything that's not already there, right?)
Comment 23 Marc Attinasi 2001-09-05 11:25:04 PDT
uh, yes, David is right. I don't know why that patch was posted here, it doe not
even fix the URL or testcase... The one in my tree is the correct version, and
that is the one that will be checked in. Thanks David!
Comment 24 Marc Attinasi 2001-09-05 18:28:24 PDT
checked into trunk and 0.9.2 branch
Comment 25 Asa Dotzler [:asa] 2001-09-06 14:13:17 PDT
Comment on attachment 48217 [details] [diff] [review]
PATCH: includes the :empty rules for the other margin-collapsing cases we already handled (also includes the changes for bug 97351)

a=asa (on behalf of drivers) for checkin to MOZILLA_0_9_4_BRANCH
Comment 26 Marc Attinasi 2001-09-06 21:17:11 PDT
Checked in to 0.9.4 branch now too. Marking FIXED

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