Bernd Reporter
	Description • 19 years ago

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.

	Bernd Reporter
	Comment 2 • 19 years ago

The button redness is a regression from bug 51767

	Simon Montagu :smontagu
	Comment 3 • 19 years ago

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.

	Bernd Reporter
	Comment 4 • 18 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 18 years ago

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.

	Bernd Reporter
	Comment 6 • 18 years ago

>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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 18 years ago

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

	Bernd Reporter
	Comment 8 • 18 years ago

>I've been waiting on TableProcessChild to die

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 18 years ago

I agree with Boris' comment #5 (of course!)

	Bernd Reporter
	Comment 10 • 18 years ago

>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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 18 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 18 years ago

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.

	David Baron :dbaron:
	Comment 13 • 18 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 18 years ago

> 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.  :(

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 18 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 18 years ago

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.

	David Baron :dbaron:
	Comment 17 • 18 years ago

Comment on attachment 261617 [details] [diff] [review]
Updated to comments

Would be good to use DIRS += rather than DIRS = in layout/generic/Makefile.in.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 18 years ago

I'll change that part to:

DIRS		= $(NULL)
ifdef MOZ_MOCHITEST
DIRS		+= test
endif

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 18 years ago

Just used DIRS += test, per irc comments.

Checked in.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 18 years ago

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.

	Bernd Reporter
	Comment 21 • 18 years ago

Thank you Boris

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 22 • 18 years ago

Actually, this depends on the TableProcessChild change, doesn't it?  So this is really not branch material...

	Daniel Veditz [:dveditz]
	Comment 23 • 18 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 18 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 25 • 18 years ago

Dan, see comment 24.

	Jesse Ruderman
	Comment 26 • 18 years ago

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.)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 27 • 18 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 18 years ago

Get this on the radar...

	Daniel Veditz [:dveditz]
	Comment 29 • 18 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 30 • 17 years ago

This patch depends on the patch for bug 377824...  I'll land it on branch once that lands.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 31 • 17 years ago

Checked in on the branches.

	Martijn Wargers (dead)
	Comment 32 • 17 years ago

Verified on trunk branches, the squares are now all green with the testcase.

	Mats Palmgren (inactive)
	Comment 33 • 17 years ago

Looks like this caused a branch regression for empty SELECTs (bug 382600).