The HTML Select control (both size=1 and listbox) needs to be implemented in XBL using outliner.

The patch I just attached gets listbox selects working, controlled via a pref. Optgroups are not yet implemented, nor is <select size=1>. I'd like to get this patch in so that people can start playing with this (and looking at reflow bugs). This should not affect the current form controls at all.

don't want bug you, but I've had some ideas - I think it's possible to do all SetOptionsSelectedByIndex() stuff in XBL I don't see any need to add all these new methods to outliner selection and content view. - Do we really want to support <option> in outliner content view ? Why not create a binding which will expand to <outlinerrow><outlinercell/></outlinerrow>

And I'm not sure if GetPrefSize() should be calculated for all outliners. There is a loop which goes through all rows and calculate max row width. Imagine a view with thousands of rows.

>------- Additional Comment #4 From Jan Varga 2001-12-29 18:53 ------- > >- I think it's possible to do all SetOptionsSelectedByIndex() stuff in XBL > I don't see any need to add all these new methods to outliner selection and > content view. Perhaps... I'll look at that closer. It would involve making nsISelectElement scriptable. >- Do we really want to support <option> in outliner content view ? > Why not create a binding which will expand to > <outlinerrow><outlinercell/></outlinerrow> That would certainly work, but we need to be conscious of performance and footprint here. Generating anonymous content isn't free. This way is cheaper, and I don't see the need to hide the HTML-ism from outliner. >------- Additional Comment #5 From Jan Varga 2001-12-29 19:00 ------- > >And I'm not sure if GetPrefSize() should be calculated for all outliners. >There is a loop which goes through all rows and calculate max row width. >Imagine a view with thousands of rows. Definitely a valid point. Any ideas on when we should and should not calculate intrinsic width?

Yes, if it's necessary to make nsISelectElement scriptable, that would be juust fine. The code in SetOptionsSelectedByIndex() is complicated enough that duplication of it is not desirable. As to GetPrefSize(): the real biggie problem here is when the size gets *smaller*. It's easy to check if the size has gotten bigger (you just calculate the new size of an option and change preferred size if the preferred size is bigger). But when the size of an option decreases, it's not obvious that the preferred size decreases either. If options 1 and 2 are both 50px wide, and you change option 2 to be 30px, the select should still be 50px wide, but how would you know about that without checking all options? My feeling on the matter is, you need to recalculate the size completely, but you only do it when: 1. An option is added 2. An option is removed 3. An option's text or label changes Much as I hate to say this, these things should be put on a timer (so that when you add 1000 options you don't recalc 1000 times, but maybe once or twice) or there should be a flag that gets set when the option is added/removed, and you recalculate the size in GetPreferredSize() only if the flag is set. I prefer the second method, but to each his own. Note that in my patch in bug 113866 (not going in yet) I added an nsISelectControlFrame::OnOptionTextChanged() that gets fired from the option. If XBL select goes in soon, that patch will become unnecessary, but I think that that particular method is a good idea.

This brings the patch up-to-date with the CVS tip, also fixes a couple of problems I spotted and some footprint savings in nsOutlinerBodyFrame by using PRPackedBool. No longer includes the nsIOutlinerView AString changes because that has already been checked in.

(1) By patching GetMappedAttributeImpact, you're now going to lose reflows when labels change on options. Maybe you just stop handling that in GetMappedAttributeImpact and rely instead on the outliner content model view's document observer to deal with those attribute changes by triggering a reflow only in the select case. (2) Don't do the intrinsic width computation in GetPrefSize for XUL trees. Only do it for HTML selects. If we find we need that in XUL, we'll work something out, but let's not slow down all outliners with this computation. You *should* do the height computation, though (checking the rows attribute for outliners and the size attribute for selects), for both XUL and HTML.

Also, I believe no timer is necessary, jkeiser, since XUL automatically coalesce reflows. If a script goes and adds/removes a bunch of options synchronously, the outliner will only call GetPrefSize once (when the single posted reflow finally gets processed).

So would it work if we just set a flag when stuff affecting the size changes and then only recalculate it in GetPrefSize()? If XUL doesn't need it then I assume it never calls it.

It does need it for height (just not for width).

Comment on attachment 63580 [details] [diff] [review] patch, version 3 >Index: layout/xul/base/src/outliner/src/nsOutlinerBodyFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/xul/base/src/outliner/src/nsOutlinerBodyFrame.cpp,v >retrieving revision 1.87 >diff -u -r1.87 nsOutlinerBodyFrame.cpp >--- layout/xul/base/src/outliner/src/nsOutlinerBodyFrame.cpp 3 Jan 2002 22:58:06 -0000 1.87 >+++ layout/xul/base/src/outliner/src/nsOutlinerBodyFrame.cpp 4 Jan 2002 21:24:24 -0000 >@@ -935,18 +1049,16 @@ >- if (!colID.EqualsWithConversion(aColID)) { >+ if (!currCol->GetID().EqualsWithConversion(aColID)) { This can be Equals instead of EqualsWithConversion (both sides are ucs-2) >Index: layout/xul/base/src/outliner/src/nsOutlinerBodyFrame.h >=================================================================== >RCS file: /cvsroot/mozilla/layout/xul/base/src/outliner/src/nsOutlinerBodyFrame.h,v >retrieving revision 1.41 >diff -u -r1.41 nsOutlinerBodyFrame.h >--- layout/xul/base/src/outliner/src/nsOutlinerBodyFrame.h 3 Jan 2002 22:58:07 -0000 1.41 >+++ layout/xul/base/src/outliner/src/nsOutlinerBodyFrame.h 4 Jan 2002 21:24:24 -0000 >@@ -175,8 +176,7 @@ > nsIContent* GetElement() { return mColElement; }; > > nscoord GetWidth(); >- const PRUnichar* GetID() { return mID.get(); }; >- void GetID(nsString& aID) { aID = mID; }; >+ const nsString& GetID() { return mID; }; You should be able to use const nsAFlatString& GetID() { return mID; }; >Index: layout/xul/base/src/outliner/src/nsOutlinerContentView.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/xul/base/src/outliner/src/nsOutlinerContentView.cpp,v >retrieving revision 1.4 >diff -u -r1.4 nsOutlinerContentView.cpp >--- layout/xul/base/src/outliner/src/nsOutlinerContentView.cpp 3 Jan 2002 22:58:09 -0000 1.4 >+++ layout/xul/base/src/outliner/src/nsOutlinerContentView.cpp 4 Jan 2002 21:24:24 -0000 >@@ -204,6 +215,19 @@ > nsOutlinerContentView::SetSelection(nsIOutlinerSelection* aSelection) > { > mSelection = aSelection; >+ if (mUpdateSelection) { >+ mUpdateSelection = PR_FALSE; >+ >+ mSelection->SetSelectEventsSuppressed(PR_TRUE); >+ for (PRInt32 i = 0; i < mRows.Count(); ++i) { >+ Row* row = (Row*)mRows[i]; >+ nsAutoString attrval; >+ if (row->mContent->GetAttr(kNameSpaceID_None, nsLayoutAtoms::optionSelectedPseudo, attrval) != >+ NS_CONTENT_ATTR_NOT_THERE) You should be able to use HasAttr now, right? >@@ -966,13 +1113,26 @@ > } > > void >+nsOutlinerContentView::SerializeOption(nsIContent* aContent, PRInt32 aParentIndex, >+ PRInt32* aIndex, nsVoidArray& aRows) >+{ >+ Row* row = Row::Create(mAllocator, aContent, aParentIndex); >+ aRows.AppendElement(row); >+ >+ // This will happen before the OutlinerSelection is hooked up. So, cache the selected >+ // state in the row properties and update the selection when it is attached. >+ >+ nsAutoString selected; >+ if (aContent->GetAttr(kNameSpaceID_None, nsLayoutAtoms::optionSelectedPseudo, selected) != NS_CONTENT_ATTR_NOT_THERE) >+ mUpdateSelection = PR_TRUE; HasAttr? >Index: layout/xul/base/src/outliner/src/nsOutlinerSelection.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/xul/base/src/outliner/src/nsOutlinerSelection.cpp,v >retrieving revision 1.20 >diff -u -r1.20 nsOutlinerSelection.cpp >--- layout/xul/base/src/outliner/src/nsOutlinerSelection.cpp 28 Sep 2001 20:06:02 -0000 1.20 >+++ layout/xul/base/src/outliner/src/nsOutlinerSelection.cpp 4 Jan 2002 21:24:24 -0000 >@@ -426,10 +436,12 @@ > if (aAugment && mFirstRange) { > // We need to remove all the items within our selected range from the selection, > // and then we insert our new range into the list. >- mFirstRange->RemoveRange(start, end); >+ ContentViewDeselectRange(start, end); >+ mFirstRange->RemoveRange(start, end); > } Nit: 1-space indent.

Comment on attachment 64441 [details] [diff] [review] better version of previous patch r=jkeiser

Comment on attachment 64441 [details] [diff] [review] better version of previous patch sr=jst

Work on XBL form controls is ongoing in the tree, but will not be completed for 0.9.8. -> 0.9.9.

*** Bug 18895 has been marked as a duplicate of this bug. ***

Major corporations depend on eapp bugs, and need them to be fixed before they can recommend Mozilla-based products to their customers. Adding nsbeta1+ keyword and making sure the bugs get re-evaluated if they are targeted beyond 1.0.

eapp was incorrectly used to change this to nsbeta1+. Resetting to nsbeta1 to nominate. This is an important issue and deserves to be nsbeta1+ by the ADT.

This is scheduled work, part of the plan of record, and not requiring nsbeta1 nomination or approval, but if it will make it clear to customers - nsbeta1+.

XBL form controls will not be turned on for 0.9.9. -> 1.0.

The XUL MenuFrame and MenuPopup changes are not part of this bug, they are for bug 127189.

Comment on attachment 71800 [details] [diff] [review] better patch for optgroup support r=jkeiser with these changes: nsHTMLSelectElement.cpp 1. Change anIndex to aIndex in GetOptionIndex. Not your fault, but you're changing the signature anyway, so ... 2. Make sure either GetOptionIndex() or its caller checks to ensure that the start index is within bounds. Especially from the select, it will actually be out of bounds sometimes. select.xml 3. Isn't this really all one if () ? It threw me off for a second why the twisty thing was in a different place from isContainer(). Similar situation with isContainer() in the next chunk. - if (row.value == -1) + if (row.value == -1 || box.view.isContainer(row.value)) return; - if (!selection.isSelected(row.value)) { + if (obj.value != "twisty" && !selection.isSelected(row.value)) { 4. If c could be -1 then i will end up as -2, won't that muck up everything? You might want to make getPrevOptionIndex safe from this too (it currently does i != -1 rather than i < 0). There are other places where you do this, just search on getPrevOptionIndex() var c = this.currentIndex; - if (c == -1 || c == 0) + var i = this.getPrevOptionIndex(c); + if (i == -1) 5. Need to remove ending } from updateLabel method 6. Looks like a bit much was removed from <handler event="command" phase="capturing"> themes/modern/forms/select-dropdown.css 7. If you're making the text color !important, shouldn't you make the background color !important? Or can both be left alone? THINGS FOR THE FUTURE (i.e. before we turn them on by default): - use atoms for tag matching instead of strings - shift+selection could use a little lookin'-at, could be modularized better

How about if I actually make changing the selected item in a select size=1 work correctly?

Uh, will we end up with "menuactive" attributes on option elements now? That's not cool, tell me I misunderstood something here.

Yeah, we will... I can rework the patch so that the option wraps the menuitem, instead of extending it, that would eliminate setting the attribute on the option.

Comment on attachment 72020 [details] [diff] [review] one more go at the optgroup patch Ok, that would be very much preferred over the current aporach.

I spun off the attribute setting problem into bug 128947.

Closing out "Implement <blah> in XBL" bugs where we have an implementation checked in that is enabled via the XBL form controls preference in the Debug pref panel. Please file any remaining issues as separate bugs.