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

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 284390 [details]
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.)
(Assignee)

Comment 1

10 years ago
Created attachment 284393 [details]
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.
(Assignee)

Comment 2

10 years ago
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.
Assignee: nobody → dholbert
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

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

Updated

10 years ago
Duplicate of this bug: 23604
(Assignee)

Comment 5

10 years ago
Created attachment 284576 [details] [diff] [review]
fix

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

Comment 6

10 years ago
Comment on attachment 284576 [details] [diff] [review]
fix

Requesting approval1.9, because this blocks (and fixes) Bug 397844, which is blocking1.9
Attachment #284576 - Flags: superreview?(dbaron)
Attachment #284576 - Flags: review?(bzbarsky)
Attachment #284576 - Flags: approval1.9?
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.
Attachment #284576 - Flags: review?(bzbarsky) → review+
Oh, and check in reftests for those two testcases?
(Assignee)

Comment 9

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

Updated

10 years ago
Duplicate of this bug: 398380
(Assignee)

Comment 11

10 years ago
Comment on attachment 284576 [details] [diff] [review]
fix

(Cancelling sr and approval requests, 'cause I'm making a few changes and reposting.)
Attachment #284576 - Flags: superreview?(dbaron)
Attachment #284576 - Flags: approval1.9?
(Assignee)

Comment 12

10 years ago
Created attachment 285006 [details] [diff] [review]
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.
Attachment #284576 - Attachment is obsolete: true
Attachment #285006 - Flags: superreview?(dbaron)
Attachment #285006 - Flags: review?(bzbarsky)
Attachment #285006 - Flags: approval1.9?
bzbarsky can r+sr if he wants
(Assignee)

Updated

10 years ago
Attachment #285006 - Flags: superreview?(dbaron) → superreview?(bzbarsky)
(Assignee)

Comment 14

10 years ago
Requesting blocking1.9, because this bug is the source of the crash in the blocker bug 397844.
Flags: blocking1.9?
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.
Attachment #285006 - Flags: superreview?(roc)
Attachment #285006 - Flags: superreview?(bzbarsky)
Attachment #285006 - Flags: review?(bzbarsky)
Attachment #285006 - Flags: review+
Comment on attachment 285006 [details] [diff] [review]
fix v2 (with reftests)

Boris, I'm no match for you in a sanity contest...
Attachment #285006 - Flags: superreview?(roc)
Attachment #285006 - Flags: superreview+
Attachment #285006 - Flags: approval1.9?
Attachment #285006 - Flags: approval1.9+
(Assignee)

Comment 17

10 years ago
(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+. :)
Flags: blocking1.9?
(Assignee)

Comment 18

10 years ago
Created attachment 285483 [details] [diff] [review]
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
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Comment 19

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

Comment 20

10 years ago
Created attachment 286914 [details] [diff] [review]
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.
You need to log in before you can comment on or make changes to this bug.