112713 - Implement HTML Select in XBL

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Brian Ryner (not reading)]

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

Comment 1

[Brian Ryner (not reading)]

Attachment: Patch, version 1

Comment 2

[Brian Ryner (not reading)]

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.

Comment 3

[Brian Ryner (not reading)]

Attachment: mac build changes to go along with this patch

Comment 4

[Jan Varga [:janv]]

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>

Comment 5

[Jan Varga [:janv]]

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.

Comment 6

[Brian Ryner (not reading)]

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

Comment 7

[John Keiser (jkeiser)]

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.

Comment 8

[Brian Ryner (not reading)]

Attachment: patch, version 2

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.

Comment 9

[David Hyatt]

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

Comment 10

[David Hyatt]

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

Comment 11

[John Keiser (jkeiser)]

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.

Comment 12

[David Hyatt]

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

Comment 13

[Brian Ryner (not reading)]

Attachment: patch, version 3

Comment 14

[jag (Peter Annema)]

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 15

[Brian Ryner (not reading)]

Attachment: fire a custom dom event when the selected item is changed

Comment 16

[Brian Ryner (not reading)]

Attachment: better version of previous patch

Comment 17

[John Keiser (jkeiser)]

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

r=jkeiser

Comment 18

[Johnny Stenback (:jst)]

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

sr=jst

Comment 19

[Brian Ryner (not reading)]

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

Comment 20

[Kevin McCluskey (gone)]

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

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 22

[Bob Clary [:bc] (inactive)]

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.

Comment 23

[Peter Trudelle]

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

Comment 24

[Brian Ryner (not reading)]

Attachment: implement optgroup

Comment 25

[Brian Ryner (not reading)]

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

Comment 26

[Brian Ryner (not reading)]

Attachment: better patch for optgroup support

Comment 27

[Brian Ryner (not reading)]

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

Comment 28

[John Keiser (jkeiser)]

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

Comment 29

[Brian Ryner (not reading)]

Attachment: new optgroup patch w/ jkeiser's suggestions

Comment 30

[Brian Ryner (not reading)]

Attachment: one more go at the optgroup patch

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

Comment 31

[Johnny Stenback (:jst)]

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

Comment 32

[Brian Ryner (not reading)]

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 33

[Johnny Stenback (:jst)]

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.

Comment 34

[Brian Ryner (not reading)]

I spun off the attribute setting problem into bug 128947.

Comment 35

[Brian Ryner (not reading)]

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.