Closed Bug 323656 Opened 14 years ago Closed 13 years ago

[FIX]Frames with anonymous box inside cause broken inheritance

Categories

(Core :: Layout, defect)

x86
All
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: bernd_mozilla, Assigned: bzbarsky)

References

Details

(Keywords: testcase, verified1.8.0.12, verified1.8.1.4)

Attachments

(4 files)

Currently three frames that create anonymous boxes ( fieldsets, table cells and buttons) call ProcessChildren with the inner anonymous box and  the inner frame provides the style context and as consequence the inheritance is broken. See attached testcase.
Attached file testcase
Depends on: 85872
The button redness is a regression from bug 51767
Blocks: 317876
This could be solved similarly to bug 267459, by adding rules to forms.css and/or ua.css to make the anonymous boxes inherit properties not inherited by default.
Boris, do you have a preference what shall happen here? I think using the inheritance approach will not work here as it would require to inherit display which is bad for the inner table cell frame which is a block. One can not exclude the display from the properties where the inheritance needs to work cross the anonymous block.
The right fix, imo, is to fix the frame constructor to pass in not only the parent frame but also the parent stylecontext (which may come from some other frame) around to child processing and then probably to fix ReParentStyleContext and ReResolveStyleContext to handle this correctly (most likely, by fixing GetCorrectedParent to skip all pseudos in all cases for non-pseudo frames; see bug 289517 for a case where we actually had to hack out a "correct" part of that code due to the frame constructor bustage).

Then we could probably remove most of the "inherit" hacks in ua.css.
>pass in not only the parent frame but also the parent stylecontext (which may >come from some other frame) around to child processing.

I guess having only one ProcessChildren function (and not the TableProcessChildren desaster) might be a plus for this.

Absolutely.  I've been waiting on TableProcessChild to die before working on that stuff.  (Well, also waiting on having more time on my hands.)
>I've been waiting on TableProcessChild to die

The patch there is slowly but moving to your review request list ;-)



Depends on: 243159
>I've been waiting on TableProcessChild to die before working on
>that stuff.  (Well, also waiting on having more time on my hands.)

TableProcessChild  is finally dead on trunk, and nobody whines :-). I can't efficeiently help with the time on your hands however I tried to prevent that table crash bugs end in your bug queue.

I could handle the part of comment 5
"frame constructor to pass in not only the parent frame but also the parent stylecontext" but the reminder is too much for me.

Blocks: 280610
Attached patch Possible fixSplinter Review
I've been thinking, and I think we should go ahead and do something like this for now on trunk and branch.  Or possibly make less drastic changes (white-list things to skip?) to the nsFrame code on branch?  In any case, I still need to test the heck out of it, but I think it's right in general.

Then if we decide to refactor the CSSFrameConstructor code in some other way, we can.  But I rather like having the parent style context frame determination only living in one place.

This fixes this bug, bug 280610, and bug 85872.

It makes the asserts in bug 317876 go away (even without this patch I couldn't get it to crash).

It also makes the asserts in bug 372376 go away (again, I couldn't get it to actually crash, but it's supposed to be random).

Something like:

data:text/html,<style>span:hover { font-style: normal} div:first-letter { color: red } div:first-line { font-style: italic} </style><div><span>aaaa</span>bbbb

still has issues (loses first-letter styling on hover), but it does that without the patch too.

David, what do you think?
Attachment #257065 - Flags: superreview?(dbaron)
Attachment #257065 - Flags: review?(dbaron)
I guess we could do the anon-box check in GetCorrectedParent() and not have to pass all those nulls in...  This way does give us more flexibility in terms of changing the exact things we skip for different kids, though.
Comment on attachment 257065 [details] [diff] [review]
Possible fix

Seems like you might want to move the null-check into GetStyleParentFrame (from GetCorrectedParent) and then maybe even get rid of GetCorrectedParent entirely.

Should you have an NS_NOTREACHED in the case where GetIBSpecialSibling fails?

Worth testing some of the bugs that show up in CVS blame for this stuff -- maybe even adding testcases if it's easy.

With that this seems fine.  r+sr=dbaron
Attachment #257065 - Flags: superreview?(dbaron)
Attachment #257065 - Flags: superreview+
Attachment #257065 - Flags: review?(dbaron)
Attachment #257065 - Flags: review+
> Seems like you might want to move the null-check into GetStyleParentFrame

I could, but the only caller that _might_ have a null parent is in fact GetCorrectedParent.  Didn't seem worth the extra null-check in the other codepaths, though maybe that's silly.

> Should you have an NS_NOTREACHED in the case where GetIBSpecialSibling fails?

Probably a good dea.

Yeah, I need to do a bunch of testing.  Need to find time.  :(
Just to make it clear, I've done zero testing except against the testcases in the bugs cited in comment 11.  In particular, general correctness testing still needs to happen, in addition to what dbaron asked for in comment 13.  Which means a bunch of tests need to get written....

I'm very unlikely to get to this in the next week (especially since I'm gone more than half of that time); after that it won't happen until the 27th.
Assignee: nobody → bzbarsky
Summary: Frames with anonymous box inside cause broken inheritance → [FIX]Frames with anonymous box inside cause broken inheritance
Whiteboard: Needs some very thorough testing before landing
Blocks: 374297
Blocks: 85872, 372376
No longer depends on: 85872
This includes tests and such.

Changes from previous patch:
1)  GetIBSpecialSibling used to leave the out param uninitialized if it got an error from the property table (e.g. if there were no property there).  Changed it to set it to null if the property is missing (and return NS_OK), and propagate any other errors.

2)  Changed callers of GetIBSpecialSibling to assert and bail on an error from it.

3)  Changed

  +  // Anon boxes are parented to their actual parent already.
  +  if (aChildPseudo && nsCSSAnonBoxes::IsAnonBox(aChildPseudo)) {

to

  +  // Anon boxes are parented to their actual parent already, except
  +  // for non-elements.  Those should not be treated as an anon box.
  +  if (aChildPseudo && aChildPseudo != nsCSSAnonBoxes::mozNonElement &&
  +      nsCSSAnonBoxes::IsAnonBox(aChildPseudo)) {

because otherwise text kids of an anon box (e.g. text inside a scrollframe) inherited from the wrong thing.  Not quite sure how to write tests for this, unfortunately; I was just seeing wrong style context warnings hovering over the url bar.

4)  Removed the unused :-moz-select-scrolled-content pseudo-element.
Attachment #261617 - Flags: review?(dbaron)
Comment on attachment 261617 [details] [diff] [review]
Updated to comments

Would be good to use DIRS += rather than DIRS = in layout/generic/Makefile.in.
Attachment #261617 - Flags: review?(dbaron) → review+
I'll change that part to:

DIRS		= $(NULL)
ifdef MOZ_MOCHITEST
DIRS		+= test
endif
Just used DIRS += test, per irc comments.

Checked in.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: Needs some very thorough testing before landing
Attached patch Branch versionSplinter Review
We may want to fix this on the branch, because of some of the dependent bugs.

Risk is moderate; this is not the most straightforward code in the world.  I'm a little happier landing this on branch now that we've got those tests I wrote.  The branch builds with this patch do pass those tests.
Attachment #261619 - Flags: approval1.8.1.4?
Attachment #261619 - Flags: approval1.8.0.12?
Flags: in-testsuite+
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Thank you Boris
Actually, this depends on the TableProcessChild change, doesn't it?  So this is really not branch material...
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Depends on: 377603
Comment on attachment 261619 [details] [diff] [review]
Branch version

Clearing (not denying) branch approval requests. This bug is marked "Depends on" a couple other trunk-only bugs. Please let us know how this all hangs together for the branch before re-requesting the branch approval.
Attachment #261619 - Flags: approval1.8.1.4?
Attachment #261619 - Flags: approval1.8.0.12?
I'm not quite sure, to be honest.  This patch _might_ work correctly without the TableProcessChild thing.  The tests seem to work.  I could check for sure, if desired.

That said, I'd say we hold off on it unless one of the deps is something we absolutely need to fix.  If so, I'll look into making absolutely sure this plays nice on branch.
Dan, see comment 24.
It would be nice for bug 372376 to be fixed on branch, given that it's a security hole.  (I haven't tested whether it exists on branch.)
Comment on attachment 261619 [details] [diff] [review]
Branch version

Well, I checked and this does play nice on branch.  The TableProcessChild code just calls the functions I've fixed, so it picks up the right behavior automatically.

The dependency was really there because for a while I was going to take a very different approach to fixing this bug...

So I do think we should try to take this on branch, since Jesse feels it would be useful.
Attachment #261619 - Flags: approval1.8.1.4?
Attachment #261619 - Flags: approval1.8.0.12?
Get this on the radar...
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Depends on: 377824
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Comment on attachment 261619 [details] [diff] [review]
Branch version

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Attachment #261619 - Flags: approval1.8.1.4?
Attachment #261619 - Flags: approval1.8.1.4+
Attachment #261619 - Flags: approval1.8.0.12?
Attachment #261619 - Flags: approval1.8.0.12+
This patch depends on the patch for bug 377824...  I'll land it on branch once that lands.
Blocks: 378240
Checked in on the branches.
Verified on trunk branches, the squares are now all green with the testcase.
Status: RESOLVED → VERIFIED
Depends on: 380842
Depends on: 382600
Looks like this caused a branch regression for empty SELECTs (bug 382600).
Depends on: 384649
You need to log in before you can comment on or make changes to this bug.