The default bug view has changed. See this FAQ.

[MARGIN-C][FIX]WRMB:frames in the given url page could not be displayed correctly

RESOLVED FIXED in mozilla0.9.5

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Amarendra Hanumanula, Assigned: Marc Attinasi)

Tracking

({testcase, topembed})

Trunk
mozilla0.9.5
x86
All
testcase, topembed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: checked into trunk,0.9.2 branch, 0.9.4 branch, URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

16 years ago
 Adding WRMB to summary and topmebed keyword
Keywords: topembed
Summary: frames in the given url page could not be displayed correctly → WRMB:frames in the given url page could not be displayed correctly

Comment 2

16 years ago
Created attachment 47419 [details]
Testcase

Comment 3

16 years ago
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.
Assignee: pollmann → attinasi
Component: HTMLFrames → Layout
Keywords: testcase
Summary: WRMB:frames in the given url page could not be displayed correctly → [MARGIN-C]WRMB:frames in the given url page could not be displayed correctly

Updated

16 years ago
Keywords: patch
(Assignee)

Comment 4

16 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 5

16 years ago
Updating various fields: patch tested and looks fine. Thanks so much Mats!
Priority: -- → P2
Summary: [MARGIN-C]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 correctly
Target Milestone: --- → mozilla0.9.4
(Assignee)

Comment 6

16 years ago
Created attachment 47704 [details] [diff] [review]
PATCH: quirk.css rule to handle margin collapsing of empty P in body and TD
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

16 years ago
What David said...  r=pierre with:
  body > p:empty:first-node,
  td   > p:empty:first-node,
  td   > p:empty:last-node
(Assignee)

Updated

16 years ago
Keywords: mozilla0.9.4
(Assignee)

Comment 9

16 years ago
OK, ready for 0.9.2 branch, nominating for 0.9.4 as well
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.
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.
(Assignee)

Comment 12

16 years ago
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.
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.
(Assignee)

Comment 14

16 years ago
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...
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.
(Assignee)

Comment 16

16 years ago
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
(Assignee)

Comment 17

16 years ago
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)
(Assignee)

Updated

16 years ago
Attachment #47704 - Attachment is obsolete: true
(Assignee)

Comment 18

16 years ago
CC'ing Pierre: I'd like to get reviews for the changes I just attached please -
thanks.

Comment 19

16 years ago
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
Attachment #48217 - Flags: review+
Updated status
Whiteboard: Waiting for review/super-review

Comment 21

16 years ago
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
Attachment #48217 - Flags: superreview+

Updated

16 years ago
Keywords: nsbranch
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Updated

16 years ago
Whiteboard: Waiting for review/super-review → have r,sr - will check into trunk (9-4-2001)
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?)
(Assignee)

Comment 23

16 years ago
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!
(Assignee)

Comment 24

16 years ago
checked into trunk and 0.9.2 branch
Whiteboard: have r,sr - will check into trunk (9-4-2001) → checked into trunk and 0.9.2 branch

Comment 25

16 years ago
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
Attachment #48217 - Flags: approval+
(Assignee)

Comment 26

16 years ago
Checked in to 0.9.4 branch now too. Marking FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: checked into trunk and 0.9.2 branch → checked into trunk,0.9.2 branch, 0.9.4 branch

Updated

16 years ago
Blocks: 99225
You need to log in before you can comment on or make changes to this bug.