Closed
Bug 313496
Opened 19 years ago
Closed 19 years ago
[FIX]Remove some QIs to nsIDOMHTMLSelectElement from nsCSSFrameConstructor
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
14.70 KB,
patch
|
Details | Diff | Splinter Review | |
13.78 KB,
patch
|
dbaron
:
review-
dbaron
:
superreview-
|
Details | Diff | Splinter Review |
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....
![]() |
Assignee | |
Comment 1•19 years ago
|
||
![]() |
Assignee | |
Comment 2•19 years ago
|
||
Attachment #200527 -
Flags: superreview?(dbaron)
Attachment #200527 -
Flags: review?(dbaron)
![]() |
Assignee | |
Updated•19 years ago
|
Summary: Remove some QIs to nsIDOMHTMLSelectElement from nsCSSFrameConstructor → [FIX]Remove some QIs to nsIDOMHTMLSelectElement from nsCSSFrameConstructor
Target Milestone: --- → mozilla1.9alpha
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.
![]() |
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?
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.
Attachment #200527 -
Flags: superreview?(dbaron)
Attachment #200527 -
Flags: superreview-
Attachment #200527 -
Flags: review?(dbaron)
Attachment #200527 -
Flags: review-
(And is there a bug somewhere on replacing at least some of the uses of nsIFrame::Type with something more like nsIContent::IsContentOfType?)
(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.
![]() |
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...
![]() |
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.
![]() |
Assignee | |
Comment 10•19 years ago
|
||
This got fixed when its dependencies got fixed.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•