399384 - First-letter style ignored after span's parent's padding is changed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

See testcase.

Expected behavior:
  - The 'a' should stay red when padding is adjusted.

Actual behavior:
  - The 'a' loses its color when padding is adjusted. (1 second after page load)

Note that the padding doesn't actually change at all -- we're setting it to 0px, which is the default value.

It's also not specific to just padding.  I tried setting the borderWidth and top/right/bottom/left css attributes, and they all have the same effect.  (And probably lots of others do as well.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (using rtl)

This is based on a testcase from bug 397844.

When we tweak the padding on this testcase, the 'a' not only loses its color, but it also gets separated into a different line.

I'm guessing that the color change and line-breaking are caused by the same problem, but I'm not sure.

Comment 2

[Daniel Holbert [:dholbert]]

Something's going wrong when we build the new styleContext for the nsFirstLetterFrame.

In processing the style change, we encounter this section of nsFrameManager.cpp (with aFrame being the nsFirstLetterFrame containing 'a'):

1154       if (newContext != oldContext) {
1155         aMinChange = CaptureChange(oldContext, newContext, aFrame,
1156                                    content, aChangeList, aMinChange,
1157                                    assumeDifferenceHint);
1158         if (!(aMinChange & nsChangeHint_ReconstructFrame)) {
1159           // if frame gets regenerated, let it keep old context
1160           aFrame->SetStyleContext(newContext);
1161         }

Right after calling CaptureChange, I found this:
 color(oldContext) = 0xff0000ff  (red)
 color(newContext) = 0xff000000  (black)

where color($arg0) is defined as
  p/x *(nsStyleColor*)($arg0)->PeekStyleData(eStyleStruct_Color)

So, newContext isn't getting the correct color.

Comment 3

[Daniel Holbert [:dholbert]]

Looks like the problem is back where we first set the value for newContext, here:

http://mxr.mozilla.org/seamonkey/source/layout/base/nsFrameManager.cpp#1129

1110     else if (pseudoTag) {
1111       nsIContent* pseudoContent =
1112           aParentContent ? aParentContent : localContent;
1113       if (pseudoTag == nsCSSPseudoElements::before ||
1114           pseudoTag == nsCSSPseudoElements::after) {
1115         // XXX what other pseudos do we need to treat like this?
1116         newContext = styleSet->ProbePseudoStyleFor(pseudoContent,
1117                                                    pseudoTag,
1118                                                    parentContext).get();
1119         if (!newContext) {
1120           // This pseudo should no longer exist; gotta reframe
1121           NS_UpdateHint(aMinChange, nsChangeHint_ReconstructFrame);
1122           aChangeList->AppendChange(aFrame, pseudoContent,
1123                                     nsChangeHint_ReconstructFrame);
1124           // We're reframing anyway; just keep the same context
1125           newContext = oldContext;
1126           newContext->AddRef();
1127         }
1128       } else {
1129         newContext = styleSet->ResolvePseudoStyleFor(pseudoContent,
1130                                                      pseudoTag,
1131                                                      parentContext).get();
1132       }

Variables are as follows:
  - aFrame is the nsFirstLetterFrame
  - pseudoContent = aParentContent = localContent nsHTMLSpanElement
  - pseudoTag is nsCSSElements::firstLetter
  - parentContext is the nsStyleContext for the span's nsInlineFrame

In this case, we jump to line 1129 to initialize newContext.  And that line returns the style context for the **span**, not for its first letter.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix

I think this fixes it. Still need to try reftests on it, though.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 284576 [details] [diff] [review]
fix

Requesting approval1.9, because this blocks (and fixes) Bug 397844, which is blocking1.9

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 284576 [details] [diff] [review]
fix

>Index: nsFrameManager.cpp
>+              break;
>+            } else {

no need for the else-after-break.  Just remove it, and outdent the body of the else.

>+          if (candidate) {

I think we can safely assert that candidate is not null here. If it is, someone _really_ screwed up the frame tree and we're in trouble anyway.  Better to crash on a null deref.

>+            // Note: if block were null, then we could only have exited the

I think this code is close enough to the loop the note isn't needed.  Either way, though.

r=bzbarsky with the nits.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, and check in reftests for those two testcases?

Comment 9

[Daniel Holbert [:dholbert]]

Thanks bz.

Also, I forgot to mention -- tested this patch on a bunch of stuff, with these results:
 - passes reftests
 - fixes this bug's testcases
 - fixes Bug 23604's testcases
 - fixes Bug 398380's testcase
 - fixes Bug 397844's testcases (including crashes)

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 284576 [details] [diff] [review]
fix

(Cancelling sr and approval requests, 'cause I'm making a few changes and reposting.)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (with reftests)

Second pass at fix.

Changes:
 - Added reftests
 - Addressed bz's suggestions from comment 7
 - Separated out the "look for lowest ancestor block" functionality into a standalone static function in nsBlockFrame, because it seems like it could be useful elsewhere.
 - Unlike the original fix, in nsFrameManager, *don't* replace parentContext -- just replace pseudoContent.  (The original fix triggered assertion failures on testcases involving both first-line and first-letter)
 - Made some comment-only typo fixes (2 occasions of flipped u/e in "pseudo", and one other place with missing letters)

This patch passes reftests, and the new (included) reftests fail pre-patch and pass post-patch.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bzbarsky can r+sr if he wants

Comment 14

[Daniel Holbert [:dholbert]]

Requesting blocking1.9, because this bug is the source of the crash in the blocker bug 397844.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 285006 [details] [diff] [review]
fix v2 (with reftests)

This looks good to me, but I feel that I had enough to do with this approach that it needs a sanity-check from _someone_ else... trying roc, since dbaron is swamped.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 285006 [details] [diff] [review]
fix v2 (with reftests)

Boris, I'm no match for you in a sanity contest...

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to comment #14)
> Requesting blocking1.9, because this bug is the source of the crash in the
> blocker bug 397844.
> 

Canceling 'blocking1.9?' flag, because the patch already got approval1.9+. :)

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: fix, as landed

Fix + reftests checked in.  From the previously posted patch, I just changed 2 instances of 
  nsBlockFrame *block
to
  nsBlockFrame* block
to match the convention in the surrounding code.


Checking in base/nsDisplayList.h;
/cvsroot/mozilla/layout/base/nsDisplayList.h,v  <--  nsDisplayList.h
new revision: 3.23; previous revision: 3.22
done
Checking in base/nsFrameManager.cpp;
/cvsroot/mozilla/layout/base/nsFrameManager.cpp,v  <--  nsFrameManager.cpp
new revision: 1.255; previous revision: 1.254
done
Checking in generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.881; previous revision: 3.880
done
Checking in generic/nsBlockFrame.h;
/cvsroot/mozilla/layout/generic/nsBlockFrame.h,v  <--  nsBlockFrame.h
new revision: 3.255; previous revision: 3.254
done
Checking in generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.762; previous revision: 3.761
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/23604-1-ref.html,v
done
Checking in reftests/bugs/23604-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/23604-1-ref.html,v  <--  23604-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/23604-1.html,v
done
Checking in reftests/bugs/23604-1.html;
/cvsroot/mozilla/layout/reftests/bugs/23604-1.html,v  <--  23604-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/23604-2-ref.html,v
done
Checking in reftests/bugs/23604-2-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/23604-2-ref.html,v  <--  23604-2-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/23604-2.html,v
done
Checking in reftests/bugs/23604-2.html;
/cvsroot/mozilla/layout/reftests/bugs/23604-2.html,v  <--  23604-2.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/399384-1-ref.html,v
done
Checking in reftests/bugs/399384-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/399384-1-ref.html,v  <--  399384-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/399384-1.html,v
done
Checking in reftests/bugs/399384-1.html;
/cvsroot/mozilla/layout/reftests/bugs/399384-1.html,v  <--  399384-1.html
initial revision: 1.1
done
Checking in reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.174; previous revision: 1.173
done

Comment 19

[Daniel Holbert [:dholbert]]

Oops -- I just stumbled across this function:
  nsBlockFrame*
nsLayoutUtils::FindNearestBlockAncestor

which seems to basically do the same thing as 
  nsBlockFrame::GetNearestAncestorBlock
(which was added by the patch for this bug)

Todo soon:  Remove GetNearestAncestorBlock and replace its uses with calls to nsLayoutUtils::FindNearestBlockAncestor.

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: update (use FindNearestBlockAncestor)

Pretty sure this is sufficient for switching to FindNearestBlockAncestor.

I initially thought I'd be able to remove the "#include nsBlockFrame.h", too, since I've removed all mentions of the nsBlockFrame type and its methods, but it turns out that would cause compiler errors.  (see below)  So I left it in.

The compiler errors happen because nsLayoutUtils::FindNearestBlockAncestor returns a nsBlockFrame*, but its .h file forward-declares the nsBlockFrame type without actually #including the nsBlockFrame.h header.  

So in order to convert the nsBlockFrame* returned by nsLayoutUtils::FindNearestBlockAncestor into a nsIFrame, we need to include nsBlockFrame.h, so that we know nsBlockFrame extends nsIFrame and can be treated as one.