Bug 608756. Cache display structs in the ruletree even for floated and positioned elements. r=dbaron

The basic idea is that mOriginalDisplay and mOriginalFloats are kept synchronized with mDisplay and mFloats unless the latter are changed due to position:absolute/fixed (for both) or float:left/right (for display). When initializing an nsStyleDisplay from a start struct, we restore the values from mOriginalDisplay/Floats to get correct behavior.
2011-10-29 03:04:20 -04:00 · 2011-10-29 03:04:20 -04:00 · 04589480b0
--- a/layout/reftests/bugs/608756-1-ref.html
+++ b/layout/reftests/bugs/608756-1-ref.html
@ -0,0 +1,4 @@
+<!DOCTYPE html>
+<body>
+<span style="border: 1px solid green;">Test<br>Test</span>
+</body>
--- a/layout/reftests/bugs/608756-1a.html
+++ b/layout/reftests/bugs/608756-1a.html
@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<head>
+  <style>
+    span { position: absolute; }
+  </style>
+</head>
+<body>
+<span></span>
+<span style="position: static; border: 1px solid green">Test<br>Test</span>
+</body>
--- a/layout/reftests/bugs/608756-1b.html
+++ b/layout/reftests/bugs/608756-1b.html
@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<head>
+  <style>
+    span { float: left; }
+  </style>
+</head>
+<body>
+<span></span>
+<span style="float: none; border: 1px solid green;">Test<br>Test</span>
+</body>
--- a/layout/reftests/bugs/608756-2-ref.html
+++ b/layout/reftests/bugs/608756-2-ref.html
@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<head>
+  <style>
+    span { float: left; border: 1px solid green }
+  </style>
+</head>
+<body>
+<span style="position: static; border: 1px solid green">Test<br>Test</span>
+</body>
--- a/layout/reftests/bugs/608756-2.html
+++ b/layout/reftests/bugs/608756-2.html
@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<head>
+  <style>
+    span { position: absolute; float: left; }
+  </style>
+</head>
+<body>
+<span></span>
+<span style="position: static; border: 1px solid green">Test<br>Test</span>
+</body>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@ -1599,6 +1599,9 @@ fails-if(!haveTestPlugin) == 599476.html 599476-ref.html
 == 604737.html 604737-ref.html
 == 605138-1.html 605138-1-ref.html
 == 605157-1.xhtml 605157-1-ref.xhtml
+== 608756-1a.html 608756-1-ref.html
+== 608756-1b.html 608756-1-ref.html
+== 608756-2.html 608756-2-ref.html
 == 609272-1.html 609272-1-ref.html
 == 608636-1.html 608636-1-ref.html
 needs-focus == 613433-1.html 613433-1-ref.html
--- a/layout/style/nsRuleNode.cpp
+++ b/layout/style/nsRuleNode.cpp
@ -3875,6 +3875,14 @@ nsRuleNode::ComputeDisplayData(void* aStartStruct,
 {
  COMPUTE_START_RESET(Display, (), display, parentDisplay)

+  // We may have ended up with aStartStruct's values of mDisplay and
+  // mFloats, but those may not be correct if our style data overrides
+  // its position or float properties.  Reset to mOriginalDisplay and
+  // mOriginalFloats; it if turns out we still need the display/floats
+  // adjustments we'll do them below.
+  display->mDisplay = display->mOriginalDisplay;
+  display->mFloats = display->mOriginalFloats;
+
  // Each property's index in this array must match its index in the
  // const array |transitionPropInfo| above.
  TransitionPropData transitionPropData[4];
@ -4281,6 +4289,10 @@ nsRuleNode::ComputeDisplayData(void* aStartStruct,
  SetDiscrete(*aRuleData->ValueForDisplay(), display->mDisplay, canStoreInRuleTree,
              SETDSC_ENUMERATED, parentDisplay->mDisplay,
              NS_STYLE_DISPLAY_INLINE, 0, 0, 0, 0);
+  // Backup original display value for calculation of a hypothetical
+  // box (CSS2 10.6.4/10.6.5), in addition to getting our style data right later.
+  // See nsHTMLReflowState::CalculateHypotheticalBox
+  display->mOriginalDisplay = display->mDisplay;

  // appearance: enum, inherit, initial
  SetDiscrete(*aRuleData->ValueForAppearance(),
@ -4358,6 +4370,8 @@ nsRuleNode::ComputeDisplayData(void* aStartStruct,
              display->mFloats, canStoreInRuleTree,
              SETDSC_ENUMERATED, parentDisplay->mFloats,
              NS_STYLE_FLOAT_NONE, 0, 0, 0, 0);
+  // Save mFloats in mOriginalFloats in case we need it later
+  display->mOriginalFloats = display->mFloats;

  // overflow-x: enum, inherit, initial
  SetDiscrete(*aRuleData->ValueForOverflowX(),
@ -4483,7 +4497,8 @@ nsRuleNode::ComputeDisplayData(void* aStartStruct,
    if (nsCSSPseudoElements::firstLetter == aContext->GetPseudo()) {
      // a non-floating first-letter must be inline
      // XXX this fix can go away once bug 103189 is fixed correctly
-      display->mDisplay = NS_STYLE_DISPLAY_INLINE;
+      // Note that we reset mOriginalDisplay to enforce the invariant that it equals mDisplay if we're not positioned or floating.
+      display->mOriginalDisplay = display->mDisplay = NS_STYLE_DISPLAY_INLINE;

      // We can't cache the data in the rule tree since if a more specific
      // rule has 'float: left' we'll end up with the wrong 'display'
@ -4494,28 +4509,25 @@ nsRuleNode::ComputeDisplayData(void* aStartStruct,
    if (display->IsAbsolutelyPositioned()) {
      // 1) if position is 'absolute' or 'fixed' then display must be
      // block-level and float must be 'none'
-
-      // Backup original display value for calculation of a hypothetical
-      // box (CSS2 10.6.4/10.6.5).
-      // See nsHTMLReflowState::CalculateHypotheticalBox
-      display->mOriginalDisplay = display->mDisplay;
      EnsureBlockDisplay(display->mDisplay);
      display->mFloats = NS_STYLE_FLOAT_NONE;

-      // We can't cache the data in the rule tree since if a more specific
-      // rule has 'position: static' we'll end up with problems with the
-      // 'display' and 'float' properties.
-      canStoreInRuleTree = false;
+      // Note that it's OK to cache this struct in the ruletree
+      // because it's fine as-is for any style context that points to
+      // it directly, and any use of it as aStartStruct (e.g. if a
+      // more specific rule sets "position: static") will use
+      // mOriginalDisplay and mOriginalFloats, which we have carefully
+      // not changed.
    } else if (display->mFloats != NS_STYLE_FLOAT_NONE) {
      // 2) if float is not none, and display is not none, then we must
      // set a block-level 'display' type per CSS2.1 section 9.7.
-
      EnsureBlockDisplay(display->mDisplay);

-      // We can't cache the data in the rule tree since if a more specific
-      // rule has 'float: none' we'll end up with the wrong 'display'
-      // property.
-      canStoreInRuleTree = false;
+      // Note that it's OK to cache this struct in the ruletree
+      // because it's fine as-is for any style context that points to
+      // it directly, and any use of it as aStartStruct (e.g. if a
+      // more specific rule sets "float: none") will use
+      // mOriginalDisplay, which we have carefully not changed.
    }

  }
--- a/layout/style/nsStyleContext.cpp
+++ b/layout/style/nsStyleContext.cpp
@ -374,10 +374,17 @@ nsStyleContext::ApplyStyleFixups(nsPresContext* aPresContext)
        disp->mDisplay != NS_STYLE_DISPLAY_TABLE) {
      nsStyleDisplay *mutable_display = static_cast<nsStyleDisplay*>
                                                   (GetUniqueStyleData(eStyleStruct_Display));
+      // If we're in this code, then mOriginalDisplay doesn't matter
+      // for purposes of the cascade (because this nsStyleDisplay
+      // isn't living in the ruletree anyway), and for determining
+      // hypothetical boxes it's better to have mOriginalDisplay
+      // matching mDisplay here.
      if (mutable_display->mDisplay == NS_STYLE_DISPLAY_INLINE_TABLE)
-        mutable_display->mDisplay = NS_STYLE_DISPLAY_TABLE;
+        mutable_display->mOriginalDisplay = mutable_display->mDisplay =
+          NS_STYLE_DISPLAY_TABLE;
      else
-        mutable_display->mDisplay = NS_STYLE_DISPLAY_BLOCK;
+        mutable_display->mOriginalDisplay = mutable_display->mDisplay =
+          NS_STYLE_DISPLAY_BLOCK;
    }
  }

--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@ -2080,9 +2080,10 @@ nsStyleDisplay::nsStyleDisplay()
  MOZ_COUNT_CTOR(nsStyleDisplay);
  mAppearance = NS_THEME_NONE;
  mDisplay = NS_STYLE_DISPLAY_INLINE;
-  mOriginalDisplay = NS_STYLE_DISPLAY_NONE;
+  mOriginalDisplay = mDisplay;
  mPosition = NS_STYLE_POSITION_STATIC;
  mFloats = NS_STYLE_FLOAT_NONE;
+  mOriginalFloats = mFloats;
  mBreakType = NS_STYLE_CLEAR_NONE;
  mBreakBefore = false;
  mBreakAfter = false;
@ -2146,6 +2147,7 @@ nsStyleDisplay::nsStyleDisplay(const nsStyleDisplay& aSource)
  mAppearance = aSource.mAppearance;
  mDisplay = aSource.mDisplay;
  mOriginalDisplay = aSource.mOriginalDisplay;
+  mOriginalFloats = aSource.mOriginalFloats;
  mBinding = aSource.mBinding;
  mPosition = aSource.mPosition;
  mFloats = aSource.mFloats;
--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@ -1539,9 +1539,13 @@ struct nsStyleDisplay {
  float   mOpacity;             // [reset]
  PRUint8 mDisplay;             // [reset] see nsStyleConsts.h NS_STYLE_DISPLAY_*
  PRUint8 mOriginalDisplay;     // [reset] saved mDisplay for position:absolute/fixed
+                                //         and float:left/right; otherwise equal
+                                //         to mDisplay
  PRUint8 mAppearance;          // [reset]
  PRUint8 mPosition;            // [reset] see nsStyleConsts.h
  PRUint8 mFloats;              // [reset] see nsStyleConsts.h NS_STYLE_FLOAT_*
+  PRUint8 mOriginalFloats;      // [reset] saved mFloats for position:absolute/fixed;
+                                //         otherwise equal to mFloats
  PRUint8 mBreakType;           // [reset] see nsStyleConsts.h NS_STYLE_CLEAR_*
  bool mBreakBefore;    // [reset]
  bool mBreakAfter;     // [reset]