71743 - Advanced Edit dialog's "Name" and "Value" textfields should be editable menulists with prefilled choices

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Charles Manske]

In each panel of Advanced Edit dialog, the "Name" and "Value" textfields should
contain dropdown lists of appropriate attributes, CSS attributes, JS events,
etc. This would make the dialog *much* more useable.
It is also the best way we have in the short term to make Composer support
accessibility guidelines, since HTML attributes such as "language" and "longdesc'
would be easily available.
These widgets must be "editable" menulists, so users can also type names and
values not in the prefilled lists.

Comment 1

[Charles Manske]

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

Comment 2

[Charles Manske]

Adding status to track Advanced Edit dialog bugs that will be fixed with new
Advanced Edit work for this bug.

Comment 3

[Charles Manske]

Oops! More time than I thought. I do intend to finish this in about a week.
Moving back to 0.9.1

Comment 4

[Charles Manske]

moving to 0.9.2

Comment 5

[Charles Manske]

Attachment: Changes required to editable menulist so "value" is similar to use in textbox (also fixes buggy behavior in previous version)

Comment 6

[Joe Hewitt (gone)]

Could we even get rid of the "label" property now?  Either way, sr=hewitt

Comment 7

[Charles Manske]

Good question. I'd feel safer keeping it at this pointing, even though it is
rather redundant.

Comment 8

[Charles Manske]

Attachment: Much better XBL that autoselects as user types in inputField

Comment 9

[Charles Manske]

Attachment: Even better XBL to not update selected item on every input, but only when popup is opened. And fixed ".selecteItem" so it is always accurate as well.

Comment 10

[Joe Hewitt (gone)]

sr=hewitt

Comment 11

[Charles Manske]

Attachment: Patch for editor changes

Comment 12

[Charles Manske]

Attachment: New file for HTML and JS events database for Advanced Property editing (place in editor/ui/dialogs/content)

Comment 13

[Charles Manske]

Attachment: Small XBL change to assure we have a <menupopu> for a <menulist edtiable="true>

Comment 14

[Charles Manske]

Attachment: Additional rules for color styling of text. Apply patch in 'themes' directory

Comment 15

[Charles Manske]

Attachment: Updated patch for changes in editor directory

Comment 16

[Kathleen :Brade]

* Why can I click on Attribute and Value headers?
* Why does double-clicking of Attribute or Value header select the value edit 
field?
* We shouldn't allow users to add "align" with "char" since we don't want to 
support that (since layout doesn't)
* Unable to choose a value from the menu (it comes up empty) for already set 
attributes (can't change them)
* For <td> bgcolor, I don't get a popup menu/list.  I expect either the 
colorpicker or a dropdown list with the 16 colors.
* I wish the drop down list icon would disappear when no list is going to appear 
(or disable it?)
* can't add "nowrap" without giving it a value
* I'd like to see more of a highlight when I select something in the attribute/
value list (so I know what I am removing if I click "remove attribute") (I'm 
using Classic Mac theme if that matters)

more later...

Comment 17

[Charles Manske]

Attachment: Updated patch for editor directory work

Comment 18

[Charles Manske]

Attachment: Update to attribute name/value database file EdAEAttributes.js

Comment 19

[Charles Manske]

Ok. Ready for more testing: Use latest versions of EdAEAttributes.js and the
patch attached 5/29 09:29. Also apply the 5/29 patch to bug 82652.
Concerning Kathy's observations:
* Why can I click on Attribute and Value headers?
* Why does double-clicking of Attribute or Value header select the value edit
field?
Just the way xul tables work, I guess.
* We shouldn't allow users to add "align" with "char" since we don't want to
support that (since layout doesn't)
That's layout's problem. This is 'Advanced Edit' and should support every
attribute in the DTD. That's the point of the dialog.
* Unable to choose a value from the menu (it comes up empty) for already set
attributes (can't change them)
* For <td> bgcolor, I don't get a popup menu/list.  I expect either the
colorpicker or a dropdown list with the 16 colors.
These problem should be fixed with latest patches.

* I wish the drop down list icon would disappear when no list is going to appear
I'll experiment with removing the "editable" attribute to see if we can do that,
but I'd be surprised if we could!

* can't add "nowrap" without giving it a value
Should be fixed now.
* I'd like to see more of a highlight when I select something in the attribute/
value list (so I know what I am removing if I click "remove attribute") (I'm
using Classic Mac theme if that matters)
That issue belongs to the Themes design team. I agree -- highlight is not
strong enough and doesn't look different when focused or not.

mo

Comment 20

[Charles Manske]

Attachment: Updated patch to editor and themes directories

Comment 21

[Charles Manske]

Patch on 5/29/01 18:24 fixes more of the things brade noticed:
The HTML "value" input field is a <textbox> when there's 1 or less items in the
value menulist.
The CSS for the selection highlighting is the default for the theme; unnecessary
"ae-selection" and other advanced editor specific styling was removed.
The highlighting for the "required" attributes are shown in bold in Classic, and
bold+italics in Modern theme (all menuitem text is bold in Modern.)

To test all of this: Apply the 18:24 patch, add the EdAEAttributes.js file to
editor/ui/dialog/contents, and apply the latest patch to bug 82652.

Comment 22

[Kathleen :Brade]

In your latest patch, I don't see any changes to a jar.mn file.  Should there be?

Comment 23

[Charles Manske]

Yes! Same change as before -- I wonder how that happened!

Comment 24

[Charles Manske]

Attachment: Same as last patch, but includes jar.mn change

Comment 25

[Kathleen :Brade]

more comments...
* tabbing is a bit odd; I can't get used to it not going to the next "control"
* on the <body> I can't seem to get the dropdown list for "bgcolor" value; I seem 
to see this problem on other tags with other attribute value lists (alignment)
* we need a prompt (???) when someone sets a name/value but then doesn't click 
"add"?  Example:  choose "id", tab, type the value, choose "class", tab, type the 
value...
* I don't understand why some menu items are italic (modern theme)

Comment 26

[Charles Manske]

Attachment: Updated patch: Simplify more -- eliminate need for Add/Set and Clear buttons.

Comment 27

[Charles Manske]

Attachment: Update on new file: Fixed wrong value list for table element align attribute.

Comment 28

[Charles Manske]

"To test" instructions:
Apply latest patch to bug 82652.
Apply patch from 5/31/01 21:26.
Add new file from patch 05/31/01 21:38 to editor/ui/dialogs/content.

Comment 29

[Charles Manske]

Attachment: Patch from 5/31 for editor directory was wrong file! This one is correct

Comment 30

[Charles Manske]

Attachment: Updated patch for editor directory

Comment 31

[Charles Manske]

Attachment: Updated patch (removed color picker stuff)

Comment 32

[Charles Manske]

Attachment: Updated editor patch (removed more irrelevant changes)

Comment 33

[Charles Manske]

Attachment: Updated editor patch

Comment 34

[Kathleen :Brade]

* in td, I can add align=Char, but there isn't an option for "char" or "charoff" 
in the dropdown list.  This seems inconsistent to me.
* it's still not intuitive to me why some things (in Modern) are "bold" and 
others aren't
* I still think it's bizarre that a double-click on the header of the list 
selects the contents of the "Value" editfield.  It doesn't matter which header I 
click on (Attribute or Value).  Ideally, double-click would do the same thing as 
click or better yet, nothing at all.  We must have some control over this stuff 
or the double-click wouldn't be selecting our control for the value.  Double-
clicking in the blank area below the attribute/value
* do we really want to have things like table height in the drop down list 
instead of encouraging users to use inline sytle?
* no dropdown list of options for "dir" attribute (I'm in <table> now); expect 
"rtl" and "ltr"

more another time...

Comment 35

[Charles Manske]

Any changes in actual contents of the dropdown lists is trivial to make: just
edit the very readable "EdAEAttributes.js". Thus we will investigate your
suggestions based on the DTD.
The "bold" items in the attribute list are the "required" attributes. Should we
add a message under the menulist to explain that?
I'll look into supressing the double-click on heading behavior.

Comment 36

[Kathleen :Brade]

Rather than having "bold" (or italic) items in the menu, how about we have a 
button to "prefill all required attributes"?

Comment 37

[Charles Manske]

Attachment: Update for new file (EdAEAttributes.js): added "char" and "charoff"

Comment 38

[Charles Manske]

Attachment: Updated patch for editor and themes directories

Comment 39

[Charles Manske]

To test this:
Apply latest patch to bug 82652.
Apply patch from 6/5/01 12:33 in "mozilla" directory.
Add new file from patch 6/5/01 12:31 to editor/ui/dialogs/content.

Comment 40

[Kathleen :Brade]

Here's a few comments based on the first part of the most recent patch file 
(sorry I can't do more; my head is too congested):

* in onChangeCSSAttribute(), you should check for (!name) before trimming the 
value string.
* would prefer "NewCSSAttribute" begin with a verb such as "AddNewCSSAttribute"
* in onSelectCSSTreeItem(), "return" should be on its own line to match up with 
other places in file
* in RemoveCSSAttribute(), "return" should be on its own line
* in SelectCSSTree(), the first line is blank (intentional?)
* in BuildHTMLAttributeNameList(), forceInteger and forceOneChar don't need to be 
initialized until we get inside the "if"
* in BuildHTMLAttributeNameList(), there is this typo in a comment: fitering
* in onChangeHTMLAttribute(), you should check for (!name) before trimming the 
value string.
* in onSelectHTMLTreeItem(), "return" should be on its own line
* in onInputHTMLAttributeName(), you have "var firstItem = null;" but never use 
it; remove line
* in onInputHTMLAttributeName(), you have declared two variables named 
"newValue"; rename one or both
* in onInputHTMLAttributeValue(), "return" should be on its own line
* in RemoveHTMLAttribute(), "return" should be on its own line
* in BuildJSEAttributeTable(), "if(nodeMap.length > 0)" should have a space after 
the "if"
* in EdAdvancedEdit.xul, is "tooltiptext" localizable?

Some things to test (if you haven't already):
 * how do we handle the use of single and double quotes in the values? (note: 
this may not be any more broken than before but it'd be good to know)
 * why do some attributes (with no values) come in with value == attribute?

Other random thoughts:
 * I like the idea of adding a 3rd column to the tree and showing an icon whether 
that attribute is required, standard (in spec), or non-standard (possibly 
others?)
 * I like the idea of having a button to "add required attributes" and giving 
them default values (if not already present in list)

Comment 41

[Charles Manske]

Attachment: Separated patch for themes directory (no changes from previous patches)

Comment 42

[Charles Manske]

Attachment: Updated patch for just editor directory: Addresses all concerns of reviewer's comments

Comment 43

[Charles Manske]

All of the syntax and format changes suggested by brade have been applied.
On the issue of "required" attributes, I removed the setting of any styles;
I'd prefer to not add more columns, buttons, etc for this version (let's keep it
simple!). We should explore those enhancements later.
Cause of the assertions are fixed, but there is still the annoying JS error:
 line 0: uncaught exception: [Exception... "Component returned failure code: 0x8
0004005 (NS_ERROR_FAILURE) [nsIMenuBoxObject.activeChild]"  nsresult: "0x8000400
5 (NS_ERROR_FAILURE)"  location: "JS frame :: <unknown filename> :: onxblcreate
:: line 5"  data: no]

that occurs when you open the Attribute menulist and there's no selected item
in that popup yet. It doesn't seem to cause any real
 failures, lockups, crashes, etc. Continuing to investigate.

The new file (EdAEAttributes.js) attached on 6/05/01 12:33 is still valid.

Comment 44

[Charles Manske]

Adding dependency on bug 35847, which has caused the input field in editable
menulists from using the correct style, and thus cutting off half the text.

Comment 45

[Charles Manske]

Attachment: Updated patch for just editor directory: Remove unused code, other small behavior fixes.

Comment 46

[Charles Manske]

Attachment: Updated patch for editor directory. Fixes "Cancel" button not working.

Comment 47

[Charles Manske]

Attachment: Updated patch to editor directory (removed diffs for irrelevant files)

Comment 48

[Charles Manske]

Attachment: Update to EdAEAttributes.js -- attribute/value database

Comment 49

[Charles Manske]

Attachment: Updated patch to editor directory -- fixes for suggestions from review

Comment 50

[kinmoz]

r=[e-mail] and sr=[e-mail] on the 06/12/01 12:25 and 06/12/01 
12:29 patches!!

Check this in quick before the diffs change again! :-)

Comment 51

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 52

[Charles Manske]

checked in. Yea!

Comment 53

[sujay]

Charley is gonna help QA by verifying this one and marking VERIFIED-FIXED.

thanks Charley!

Comment 54

[Charles Manske]

Sujay: Not a big deal, but you can verify it, can't you? It works!

Comment 55

[sujay]

Thanks Charley...I also verified it...just wanted to have another
pair of eyes to double check because this bug had lots
of attachments and dependencies..

thanks.