Closed
Bug 126072
Opened 23 years ago
Closed 22 years ago
[FIXr][RR]:before and :after pseudo elements are not rendered when using alternate style sheets
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla1.3beta
People
(Reporter: paul, Assigned: bzbarsky)
References
()
Details
(Keywords: css2, regression, testcase)
Attachments
(5 files, 4 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/css
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.8) Gecko/20020204
BuildID: 2002020406
When an alternate style sheet is selected from the menu the :before and :after
psuedo classes aren't rendered. The problem also persists if the sheet is
switched back to the prefered style sheet.
Reproducible: Always
Steps to Reproduce:
1. load up page
2. select alternate style sheet from Mozilla's menu
Actual Results: No psuedo classes were rendered
Expected Results: Text should be rendered before and after the test paragraph.
Assignee | ||
Comment 1•23 years ago
|
||
seeing this on Linux 2002-02-17-06 too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 98 → All
Hardware: PC → All
Assignee | ||
Comment 2•23 years ago
|
||
Interestingly enough, this works at http://web/bzbarsky/www/index.shtml (switch
to the "Debug Look" stylesheet). Is this an issue when the first sheet has no
generated content?
Comment 3•23 years ago
|
||
ReResolveStyleContext presumably needs to ProbePseudoStyleContextFor and cause a
framechange hint if it gets one where there wasn't one before.
Summary: :before and :after psuedo classes are not rendered when using alternate style sheets → [RR]:before and :after psuedo classes are not rendered when using alternate style sheets
Assignee | ||
Comment 4•23 years ago
|
||
Oh, this is a regression, btw. It works in a build from Dec 13, 2001. It fails
with a build from Jan 7, 2002 (the next one I have after the Dec 13).
I have to correct comment #2, I was testing that with 0.9.6 or 0.9.7. With a
current build on my page the :after pseudo-elements on the <a> tags fail to
render. Making the <a> elts display:block fixes that; not sure whether that's
related to the problem at hand.
Keywords: regression
Comment 5•23 years ago
|
||
hyatt changed things on 12-17 so switching stylesheets just causes a reresolve
instead of a frame reconstruct. That's probably what did it.
Assignee | ||
Comment 6•23 years ago
|
||
Hmm... ProbePseudoStyleContextFor seems to want a pseudo.. so would we want to
separately probe for nsCSSAtoms::beforePseudo and nsCSSAtoms::afterPseudo, I guess?
Comment 7•23 years ago
|
||
yep.
Assignee | ||
Comment 8•23 years ago
|
||
Ok.. I can "fix" this by just causing a framechange anytime there are pseudos on
the new style context. This seems like it would be very broken.
The problem I run into is that I can't tell whether the old style context had
pseudos involved or not... I added the following code to ReResolveStyleContext:
nsCOMPtr<nsIStyleContext> pseudoContext;
aPresContext->ProbePseudoStyleContextFor(content, nsCSSAtoms::beforePseudo,
oldContext, PR_FALSE,
getter_AddRefs(pseudoContext));
if (pseudoContext) {
fprintf(stderr, "Old style context is: %p\n", oldContext);
pseudoContext->List(stderr, 0);
}
I get the following output on sheet switch (going from sheet one to sheet two):
frame: Inline(p)(1) (0x8780e88) style: 0x8780e00 :before {}
Wrong parent style context: style: 0x8780ccc {}
should be using: style: 0x8781610 {}
frame: Inline(p)(1) (0x8781068) style: 0x8780fe0 :after {}
Wrong parent style context: style: 0x8780ccc {}
should be using: style: 0x8781610 {}
Old style context is: 0x8781610
0x87815dc(1) parent=0x8781610 :before {
p:before weight: 1 { content: ""[before two] "" }
}
So it looks like there are some issues with style context parenting here to
start with... The key problem, however is that the "old" style context already
seems to have the new pseudo context as a descendant.... the "new" context also
has it as a descendant (that's hardly surprising). The upshot of all this is
that there is no way to tell that there used to be no pseudo, as far as I can
see....
Comment 9•23 years ago
|
||
I'm getting this in Mozilla 0.9.8 too (specifically the package compiled
for Debian GNU/Linux's 'unstable' branch, version 2:0.9.8-2), and things
look even more screwy here. At http://menagerie.tf/~jrhunter/mozoddity/
(it's a copy of the front page of my personal website, which I'm currently
working on), if you switch to "nocode.css", there should be little colored
'degree' characters after every link. As per this bug, they don't show up
in most of the document body, but they DO show up in the Table Of Contents.
I'm not sure how/if this helps, but it strikes me as being one of those
"things that make you go 'hmmm'".
Comment 10•23 years ago
|
||
bz -- I think you may need to reframe in any case.
There are three scenarios:
Persistent stylesheet has content - Switching stylesheets doesn't change
content.
Preferred stylesheet has content - Switching stylesheets causes content to
disappear.
Only alternate stylesheet has content - Switching stylesheets doesn't create
content.
Also, Mozilla crashes sometimes if there are images involved.
Comment 11•23 years ago
|
||
Comment 12•23 years ago
|
||
Assignee | ||
Comment 13•23 years ago
|
||
Well... do we use ReResolveStyleContext for anything other than stylesheet
switching?
Comment 14•23 years ago
|
||
We use ReResolveStyleContext for similar things to sheet changes such as rule
removal / insertion and selector modification (althougb before hyatt's
re-enabling of theme switching at least some of these caused full reconstructs),
and also for things like attribute changes and content state changes (:hover,
:active) that affect which selectors match a content node.
Assignee | ||
Comment 15•23 years ago
|
||
I suspected something like that... It's :hover that bothers me... If I have:
p:before { content: "three" }
it seems like reframing the <p> on every :hover just because it has a pseudo
would be a bad idea....
Comment 16•23 years ago
|
||
Only the pseudos need to be reframed--the rest of the content only needs a
reflow, and that only if size-related properties have changed.
Comment 17•23 years ago
|
||
We don't need to reframe unnecessarily (you know what I mean, anyway). See
RemoveGeneratedContentFrameSiblings for how to tell if they exist -- and we can
then do a ProbePseudoStyleContextFor during ReResolve to see if they should
continue to exist (if they don't, in which case we should get a ReResolve).
Making them disappear might be a tiny bit tricky, but I think
ResolvePseudoStyleContextFor has a useful return value in that case...
Comment 18•23 years ago
|
||
If you need another testcase, I believe this is one (from the debugs):
http://news.bbc.co.uk/hi/english/sci/tech/newsid_1880000/1880566.stm
Comment 19•23 years ago
|
||
Check out my humor page which uses :before and :after to insert images various
places in the document. It loads with all the :before :after content showing.
After switching the stylesheet and then going back to "Added Extras", the
images don't show up anymore. Have to reload the page to get them back.
http://www.visi.com/~hoju/humor.html
Jake
Comment 20•23 years ago
|
||
I think my bug 133465 may be a manifestation of this bug as well - although
in this case it is print media styles/stylesheet with generated content
activated on print or print preview.
Updated•23 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha
Comment 21•23 years ago
|
||
See also bug 57226, Changing attributes does not reframe :before/:after
generated content [GC]. Is this a dup?
Comment 22•23 years ago
|
||
I marked bug 57226 as depending on this one. See bug 57226 comment 8 for why.
(The testcases here are simpler, and I'm more convinced it's only one issue.)
Comment 23•23 years ago
|
||
cc'ing myself
Updated•23 years ago
|
Target Milestone: mozilla1.1alpha → mozilla1.1beta
Updated•23 years ago
|
Target Milestone: mozilla1.1beta → mozilla1.2alpha
Comment 24•22 years ago
|
||
Note that this is somewhat related to bug 145419, which is about :first-letter
and :first-line, although that bug probably also depends on bug 23604.
Severity: normal → major
Target Milestone: mozilla1.2alpha → Future
Comment 25•22 years ago
|
||
The :before pseudo-element called from an external style sheeted linked via
<link> tag does not work. But if the style is included internally within the
<style> tag it works fine.
see attached test case
Comment 26•22 years ago
|
||
Comment 27•22 years ago
|
||
Comment 28•22 years ago
|
||
my Bad...... please ignore comment 25, comment 26, and comment 27.
They were supposed to be included in bug 171324 :-)
Assignee | ||
Comment 29•22 years ago
|
||
I have a not-too-bad solution for the case when :before and :after need to be
added.... Still working on something that's not horribly slow for when they
need to be removed.
Assignee | ||
Comment 30•22 years ago
|
||
This should work... two issues:
1) The usage of nsIInspectorCSSUtils is complete crap... I just can't get
nsRuleNode::IsRoot callable from layout otherwise... we need some sort of
glue
to do that. :(
2) This still triggers warnings from VerifyStyleTree when it should be tearing
down the pseudo frames... perhaps they are not even being torn down.
Assignee | ||
Comment 31•22 years ago
|
||
Comment on attachment 108686 [details] [diff] [review]
proof-of-concept
This is not ready for prime-time, of course; just putting it up in case people
have any bright ideas....
Attachment #108686 -
Attachment is obsolete: true
Comment 32•22 years ago
|
||
Comment on attachment 108686 [details] [diff] [review]
proof-of-concept
>Index: layout/html/base/src/nsFrameManager.cpp
> aPresContext->ResolvePseudoStyleContextFor(pseudoContent, pseudoTag,
> parentContext,
> &newContext);
>+ if (newContext) {
>+ // Check whether this is sitting at the root rulenode; if it
>+ // is, we don't actually have style for this pseudo anymore
>+ // and should kill this frame
>+ nsRuleNode* ruleNode = nsnull;
>+ newContext->GetRuleNode(&ruleNode);
>+ if (ruleNode) {
>+ nsCOMPtr<nsIInspectorCSSUtils> cssUtils =
>+ do_GetService(kInspectorCSSUtilsCID);
>+ PRBool root = PR_FALSE;
>+ cssUtils->IsRuleNodeRoot(ruleNode, &root);
>+ if (root) {
>+ NS_UpdateHint(aMinChange, nsChangeHint_ReconstructFrame);
>+ aChangeList.AppendChange(aFrame, pseudoContent,
>+ nsChangeHint_ReconstructFrame);
>+ }
>+ }
>+ }
Can't you just ProbePseudoStyleContextFor instead, and append a framechange if
you get null?
I think there's another bug somewhere on the same topic where I suggested
having bits on the style context if they have child style contexts for :before
or :after. Any thoughts on that idea, or can this approach fix the whole
problem?
Assignee | ||
Comment 33•22 years ago
|
||
Hmm... I guess I could just probe and that gives me exactly the same style
context _except_ when I need to reframe anyway... Lemme try that.
Yeah, having bits on the parent would allow at least avoiding the two probes for
new pseudos (we'd still need to resolve the context for the pseudo frame when it
already exists, since we could just want to keep it exactly as-is).
Assignee | ||
Comment 34•22 years ago
|
||
I still get the warnings from VerifyFrameTree in
FrameManager::ComputeStyleChangeFor, but I get them for _all_ frames that are
being reframed (<head>, the <div>s whose "position" changed, etc on
http://web/mit.edu/bzbarsky/www/index.shtml). The problem is that
ReResolveStyleContext leaves the old context when a frame will be reframed.
This means that the context has the wrong parent, of course, if its parent
frame got a new context.
We should try to make that warning system a little smarter, somehow... :(
Assignee | ||
Comment 35•22 years ago
|
||
Attachment #108690 -
Attachment is obsolete: true
Assignee | ||
Comment 36•22 years ago
|
||
I should just take this...
Assignee: dbaron → bzbarsky
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla1.3beta
Assignee | ||
Comment 37•22 years ago
|
||
Summary of changes:
1) Move some functions from being statics in nsPresShell.cpp to living in a
new
utility class -- nsLayoutUtils.
2) Detect when we need to create new :before or :after kids and reframe the
parent frame
3) Detect when a :before or :after frame needs to go away and reframe it
4) Fix ComputeStyleChangeFor not to call VerifyStyleTree since the style
context and frame trees are still in an inconsistent state there (pending
reframing of some stuff)
5) Make callers of ComputeStyleChangeFor call DebugVerifyStyleTree as needed
6) Build system changes, blah blah
Changes #4 and #5 make the bogus warnings I was seeing go away. I _do_ still
see some warnings when switching back to the "Screen Look" on my site, but
those look correct to me (some table pseudo-frame issues that I think we may be
messing up).
Attachment #108691 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Summary: [RR]:before and :after psuedo classes are not rendered when using alternate style sheets → [FIX][RR]:before and :after psuedo classes are not rendered when using alternate style sheets
Assignee | ||
Comment 38•22 years ago
|
||
Comment on attachment 108828 [details] [diff] [review]
Ok, this one I'm happy with
David? kin? Would you review? This only fixes the ::before and ::after
pseudos; I need to read up a bit more on how we handle things like ::first-line
and ::first-letter before trying to do anything useful with them...
Attachment #108828 -
Flags: superreview?(kin)
Attachment #108828 -
Flags: review?(dbaron)
Assignee | ||
Updated•22 years ago
|
Summary: [FIX][RR]:before and :after psuedo classes are not rendered when using alternate style sheets → [FIX][RR]:before and :after pseudo elements are not rendered when using alternate style sheets
I think nsLayoutUtils::IsGeneratedContentFrame and nsLayoutUtils::IsPseudoFrame
should be methods in nsIFrame (concrete inline methods, not virtual ones).
Assignee | ||
Comment 40•22 years ago
|
||
Attachment #108828 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #108828 -
Flags: superreview?(kin)
Attachment #108828 -
Flags: review?(dbaron)
Assignee | ||
Updated•22 years ago
|
Attachment #108918 -
Flags: superreview?(roc+moz)
Attachment #108918 -
Flags: review?(dbaron)
Comment on attachment 108918 [details] [diff] [review]
Ooh. Good idea.
sr=roc+moz
Attachment #108918 -
Flags: superreview?(roc+moz) → superreview+
Comment 42•22 years ago
|
||
any chance this can get commited someone else in Boris' absence?
Jake
Assignee | ||
Comment 43•22 years ago
|
||
I can commit it myself, but _after_ it has review.
Comment 44•22 years ago
|
||
Comment on attachment 108918 [details] [diff] [review]
Ooh. Good idea.
r=dbaron
Attachment #108918 -
Flags: review?(dbaron) → review+
Assignee | ||
Updated•22 years ago
|
Summary: [FIX][RR]:before and :after pseudo elements are not rendered when using alternate style sheets → [FIXr][RR]:before and :after pseudo elements are not rendered when using alternate style sheets
Assignee | ||
Comment 45•22 years ago
|
||
fix checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•