Boris Zbarsky [:bzbarsky] Assignee
	Description • 19 years ago

There are two sorta-silly reasons we do these QIs:

1)  To avoid calling WipeContainingBlock
2)  To check whether we should call RemoveDummyFrameForSelect

For the former, the frame we're looking at (the parent frame) is an nsSelectsAreaFrame.  Problem is, it has display:inline.  What we should probably do is that WipeContainingBlock should immediately bail out (no wiping) if the parent is an area or block frame, since in that case there is no reason to wipe.  I prefer this to adding a frame type to nsSelectsAreaFrame, since we'd need that type added all over where we check for area/block frames.

The latter is a little harder, but I'm thinking we just want to replace the QI with a IsContentOfType+tagname check or something.  Since our parent is not the actual listbox or combobox frame (but the nsSelectsAreaFrame), we can't check that.  Or I guess we could do a GetPrimaryFrameFor() here, actually.  It should be fast enough, since we already did it at the start of the method, so it's in the hashtable....

	David Baron :dbaron:
	Comment 3 • 19 years ago

Did you go through all the locations that test for nsLayoutAtoms::areaFrame and see if they need to test for comboboxControlFrame?  I didn't see changes of that type, so I'm a little suspicious.

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

Ugh.  No, I haven't done that... And I really would rather not -- it would be pretty error-prone to do that, I think.

Would you be OK with checking the content to see whether it's an html:select instead of checking the parent frame?  It won't fix stuff that uses xbl:extends to extend html:select, but do we really care that much about the dummy option stuff for that case?

	David Baron :dbaron:
	Comment 5 • 19 years ago

So, the WipeContainingBlock stuff needs to be fixed for inline-block as well, so we should really find a different approach for that.  Maybe now's the time.

As for RemoveDummyFrameFromSelect, I don't really know what that code does.  What's a dummy frame and why do we need it?  Is it for when a combobox has no options?  And it seems to be combobox-specific, so we probably shouldn't call it a select (since that's combobox or listbox).  I'm a little hesitant to break extending select since other things may actually want to reuse combobox, although it would be great if we rewrote the whole thing at some point soon.

	David Baron :dbaron:
	Comment 6 • 19 years ago

(And is there a bug somewhere on replacing at least some of the uses of nsIFrame::Type with something more like nsIContent::IsContentOfType?)

	David Baron :dbaron:
	Comment 7 • 19 years ago

(In reply to comment #5)
> So, the WipeContainingBlock stuff needs to be fixed for inline-block as well,
> so we should really find a different approach for that.  Maybe now's the time.

Well, I think it does, but I'm actually not sure.

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

> So, the WipeContainingBlock stuff needs to be fixed for inline-block as well,
> so we should really find a different approach for that.  Maybe now's the time.

I think it should already work fine for inline-block...  The only reason there was a problem for <html:select> is that that creates an nsAreaFrame with display:inline.  Since options are display:block, this means that every option insertion would reframe the whole select, which is not really needed.  That's all this patch is trying to avoid with the areaFrame/blockFrame check.  For inline-block, we'd bail out of WipeContainingBlock at the |NS_STYLE_DISPLAY_INLINE != aFrame->GetStyleDisplay()->mDisplay| check, would we not?

The other option would be to check whether aFrame is a "non-replaced inline frame"; the problem is that I'm not sure we have a good check for this.  Perhaps compare the type to inline and positioned inline?  I believe those are the only cases that lead to {ib} splits, right?  At least those are the only callers of ConstructInline, and that's the only place where we do {ib} splits.

> What's a dummy frame and why do we need it?  Is it for when a combobox has no
> options?

It's for when a <select> has no options.  We synthesize an option and stick it in the listbox (if the <select> is a combobox we stick it into the dropdown).  It's not great, since if all the options are display:none we won't do the dummy frame and also won't put anything in the dropdown.

Quite frankly, it looks like the dummy option was added just to deal with the sizing of empty selects being "wrong".  The checkin comment (in nsCSSFrameConstructor.cpp) is:

  1.188 <rods@netscape.com> 1999-08-06 07:11
  Added code to create and remove the generated content and frame when the
  select has no options. This necessary for correct sizing of the select when
  empty.

(no bug number, or testcases, or anything).  I think we can get pretty much the same effect with:

  select:empty { min-width: 1em; }

or so (might need to twiddle the width a tad, maybe use ex instead of em?) in html.css.  And then rip out all this code...  How does that sound?

> (And is there a bug somewhere on replacing at least some of the uses of
> nsIFrame::Type with something more like nsIContent::IsContentOfType?)

No, but we should do it.  Want to file, or should I?  That would allow us to not worry about adding new frame types like we have to right now...

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

OK, I think I should split the two distinct parts of this patch into separate bugs, since they're pretty independent and since I'm going to sorta take a violent approach to the dummy frame and don't really want to WipeContainingBlock changes to wait on it.  Let's turn this into a tracker and I'll file new bugs for the two parts.

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

This got fixed when its dependencies got fixed.