88761 - html code for selected and noshade (and other minimizable attributes) not w3c conformant

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect, P1)

Description

[infinity]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.2) Gecko/20010628
BuildID:    2001062815

Mozilla composer sets "<option selected
value="http://hannover.gay-web.de/">Homepages...</option>" to "<option
selected="" value="http://hannover.gay-web.de/">Homepages...</option>".
selected="" isn't w3c html 4.01 conform. Same problem with "<hr noshade
width="100%" size="1">". Mozilla sets noshade="".





Reproducible: Always
Steps to Reproduce:
1. insert horizontal rule without 3d effect
2. insert selected option
3.

Actual Results:  after saving the code mozilla composer sets noshade to
noshade="" and selected to selected=""

Expected Results:  html code should be w3c conform. checked with
http://validator.w3c.org

would be great to support for simply editing option fields by menu...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Over to the editor ("compositor" is not the same as "composer")

we want to be doing 'selected="selected"' and 'noshade="noshade"'...

Comment 2

[rubydoo123]

correct the attribute value for selected must be selected and not ""

for the hr element, the noshade attriubte is deprecated, so we should not be
inserting that by default.

Comment 3

[Charles Manske]

changing milestone to 0.9.4

Comment 4

[Charles Manske]

Beppe: So now that "noshade" is deprecated, I think we should simply rip it out
of the dialog and the prefs we store via the "Use as default" 
button in the H.Line Properties dialog. If we edit a page with HR that has 
"noshade", the attribute is editable in the Advanced Edit dialog. Note that in 
that dialog, we correctly use:
noshade="noshade"
so if an advanced user adds that attribute, it will be written as w3c compliant.
How does this sound?

Comment 5

[rubydoo123]

totally agree charley

Comment 6

[Charles Manske]

I don't see how Composer is at fault for the <option selected... issue.
We don't explicitly support <option> tags. How did you insert that tag? 
Raw HTML in the HTML source window?

Comment 7

[Charles Manske]

Attachment: Fix: Remove UI for "3D Shading" (noshade attribute) for horizontal line

Comment 8

[Charles Manske]

Sorry, the patch also contains the fix for bug 76206 in EdHLineProps.js.
I'm trying to fix all the issues with Horizontal Line code at the same time 
and it was difficult to separate the issues. Thought it best the leave in the
extra lines. So in these changes:
@@ -124,6 +113,8 @@
       var percentIndex = width.search(/%/);
       var percent;
       var widthInt;
+      var heightInt;
+
       if (width)
       {
         if (percentIndex > 0) {
@@ -139,13 +130,12 @@
         percent = true;
         widthInt = Number(100);
       }
-      prefs.SetIntPref("editor.hrule.width", widthInt);
-      prefs.SetBoolPref("editor.hrule.width_percent", percent);
 
-      // Convert string to number
-      prefs.SetIntPref("editor.hrule.height", Number(height));
+      heightInt = Number(height ? height : 2);
 
-      prefs.SetBoolPref("editor.hrule.shading", shading);
+      prefs.SetIntPref("editor.hrule.width", widthInt);
+      prefs.SetBoolPref("editor.hrule.width_percent", percent);
+      prefs.SetIntPref("editor.hrule.height", heightInt);
 
       // Write the prefs out NOW!
       prefs.savePrefFile(null);

the only line pertaining to this bug is to remove saving the "shading" pref:
-      prefs.SetBoolPref("editor.hrule.shading", shading);

Comment 9

[Simon Fraser [no longer active]]

So this does not address the issue of adding attributes with empty values, which 
is what the bug is about. I think we need to know more about how those attributes 
get in.

Comment 10

[Charles Manske]

I agree. Maybe it would be better to file a new bug to remove "noshade" so it
doesn't muddle the original bug.
I'll do that.

Comment 11

[kinmoz]

Bah, mid-air collission:

The bug is really in nsHTMLContentSerializer::SerializeAttributes(). We need 
to add code there that checks for known attributes that have no values defined 
for them and leave off the ="" if the value has a length of zero.

Cc'ing HTML Serializer folks. This should probably go to [e-mail].

Why remove the noshade UI? I vote to keep it, since I use it all the time. Let's 
file a different bug regarding removal of noshade and leave this bug for the 
original reported issue.

Comment 12

[kinmoz]

Cc'ing HTML Serializer folks. :-) So they see my last comment.

Comment 13

[Charles Manske]

Removing "noshade" issue is now bug 93468.
Please ignore the 8/02/01 attachment -- it has been moved to the new bug.

Comment 14

[Akkana Peck]

Output used to handle attributes with null values correctly (not add the =""). 
But it looks like the code has been changed to inherit from
nsXMLContentSerializer and to call that class' method SerializeAttr, which
always adds the ="".  This probably happened as part of the noxif landing, so
the solution might involve resurrecting or at least consulting the code from
nsHTMLContentSinkStream which was removed at that time (unless the right thing
to do is simple, e.g. never add ="").

Comment 15

[Charles Manske]

According to W3C, the correct format is the use the attribute name as the value,
e.g., noshade="noshade", not to use just "noshade".

Comment 16

[Charles Manske]

Sounds like Akkana knows how to pursue this.

Comment 17

[Akkana Peck]

Anthony owns the serializers.

The content sink stream originally used the attribute name as the value, and we
got complaints and had to change it, e.g. for <table border> which people didn't
want to see turned into <table border="border">.  See bug 6083.

Comment 18

[Charles Manske]

Attachment: Fixes syntax of "noshade" attribute created by new HLine command

Comment 19

[Charles Manske]

We still need to make the serializer changes, but now that we are keeping 
"noshade" attribute, the small fix for inserting a new hline needs to be made.

Comment 20

[Charles Manske]

Attachment: Updated patch to fix 'noshade' syntax in Composer in both Inser HLine command and in HLine properties dialog

Comment 21

[Simon Fraser [no longer active]]

sr=sfraser

Comment 22

[rubydoo123]

on 8/6 we had a lengthly discussion about this via email, during that
discussion, Kin provided the sr, from his posting on 8/6:

r/sr=[e-mail] on the noshade patch

Comment 23

[Charles Manske]

8/07/01 patch for ComposerCommand.js has been checked in.

Comment 24

[rubydoo123]

--> cmanske

Comment 25

[Syd Logan]

Please clear my head on this. There is a patch that has r= and sr=, why is it
not checked in?

Is there some other part of this bug not addressed by the patch? Do we need to
reassign this bug on serializer, or open another (dependency) bug?

Comment 26

[Charles Manske]

The r=, sr= was for a samll part of the fix in Composer.
The basic problem is in serializer. It should be reassigned there.
I'm not sure what component owns that.

Comment 27

[Akkana Peck]

Charley: it is a component, Dom to Text Conversion.  Reassigning.

Comment 28

[Peter Van der Beken [:peterv]]

All these missed the bus/train/plane/boat/whatever. Sad.

Comment 29

[Kathleen :Brade]

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

Comment 30

[Adam Lock]

Doesn't this issue indicate a problem during parsing if the DOM is not being
built correctly for valueless attributes?

Comment 31

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

Actually it is not a valueless attribute: it is an attribute that has a value
equal to its name. The shorthand is allowed in SGML (and HTML), but not in XML.
We loose this information in serialization somehow. Adding dataloss keyword.

This bug _might_ be caused by something like this (haven't investigated!): we
parse everything correctly, but store the 'selected' value in the frame code,
which we do not consult when we serialize the content tree. If this really is
the case, then we need to move the logic from frame to content. I thought about
this because a lot of logic/state has already been moved from frames to content.

This should be re-triaged, I believe this is important enought to fix before 1.0.

Comment 32

[scottputterman]

ADT2 per ADT triage.

Comment 33

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

Peter says he has a preliminary fix, need to verify etc. In any case this looks
serious.

Comment 34

[Peter Van der Beken [:peterv]]

Attachment: v1

This should fix it.

Comment 35

[Peter Van der Beken [:peterv]]

I fixed this by outputting shorthand attributes as shorthand if their value is
empty or their value is equal to their name.

<hr noshade=""> becomes <hr noshade>
<hr noshade="noshade"> becomes <hr noshade>
<hr noshade="foo"> becomes <hr noshade="foo">

Comment 36

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

That is somehow counterintuitive to me, but it seems to coinside to what we
actually do in layout. All of the following result in the same noshade layout. I
would have expected that the second and the last would not have done that.
Apparently we only check for the presence of the attribute. IE6 does the same thing.

<hr noshade>
<hr noshade="">
<hr noshade="noshade">
<hr noshade="foo">

However, regarding output, I would really prefer: 

<hr noshade> becomes <hr noshade="noshade">
<hr noshade=""> becomes <hr noshade="noshade">
<hr noshade="noshade"> becomes <hr noshade="noshade">
<hr noshade="foo"> becomes <hr noshade="foo">

if we can't retain the original information. This way we would actually make the
page _better_, not _worse_ (like the patch now does, making XHTML documents
instantly non-wellfomed, for example).

Comment 37

[Peter Van der Beken [:peterv]]

Attachment: v2

Fixes HTML and XHTML. Output shorthand attribute foo with empty value as
foo="foo".

Comment 38

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

Don't put that stuff into the XML content sink. A validating XML parser would
flag this as an error:

<hr noshade="">

because the DTD defines that

<!ATTLIST hr
          noshade (noshade) #IMPLIED>

i.e. noshade attribute is optional but the only allowed value is "noshade". For
more details see the XML Recommendation Section 3.3.1, [59] Enumeration and
validity constrain enumeration at
http://www.w3.org/TR/2000/REC-xml-20001006#sec-attribute-types

Otherwise it looks good so if you don't do fixups in the XML serializer and move
the other stuff back to HTML serializer I can give r/sr.

Comment 39

[Peter Van der Beken [:peterv]]

I don't understand what you're saying. Right now we don't store the shorthand
attribute's value, so in HTML and in XHTML <hr noshade="noshade" /> will end up
in the content model as an hr element with a noshade attribute with an empty
value. You want me to serialize that to the (invalid) <hr noshade="" /> ?
Moreover, even if we were to store the value and someone set the value to an
empty string somehow you want to output invalid XHTML?

Comment 40

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

Comment on attachment 84910 [details] [diff] [review]
v2

Doh, I stand corrected, I somehow assumed that in XHTML (served with an XML
mime type) the attribute value would be present if I explicitly had it in the
source. But since the implementation is really HTML that does not save the
attribute value it disappears. Hence your fix is correct, and r=heikki

However, please file a new bug on retaining the values of these attributes. If
we fix that (later) then we could move the fix to HTML and we wouldn't need to
fixup XHTML.

Comment 41

[Johnny Stenback (:jst)]

Comment on attachment 84910 [details] [diff] [review]
v2

+    if (namespaceID == kNameSpaceID_None) {
+      PRInt32 elementNsID;
+      nsCOMPtr<nsIAtom> elementName;
+      content->GetNameSpaceID(elementNsID);
+      content->GetTag(*getter_AddRefs(elementName));
+      if (elementNsID == kNameSpaceID_XHTML &&
+	   IsShorthandAttr(attrName, elementName) &&
+	   valueStr.IsEmpty()) {
+	 valueStr = nameStr;
+      }
+    }

If you'd separate out the if (elementNsID == kNameSpaceID_XHTML) you could
avoid creating the nsCOMPtr<nsIAtom> and also avoid the call to GetTag() if the
element is not XHTML.

- In nsXMLContentSerializer::IsShorthandAttr(const nsIAtom* aAttrName, const
nsIAtom* aParentName):

Rename aParentName to aElementName, and split that mile-long return statement
in to separate if (...) return PR_TRUE;, what you have is not very readable,
and separate if statements will generate the same code, or at least close
enough.

sr=jst if you fix all that.

Comment 42

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 43

[Peter Van der Beken [:peterv]]

Checked in to the trunk.

Comment 44

[Peter Van der Beken [:peterv]]

Comment on attachment 85237 [details] [diff] [review]
v2.1

Carrying forward r=heikki, sr=jst.

Comment 45

[sujay]

infinity, please verify in next build...and mark verified fixed..

Comment 46

[Jaime Rodriguez, Jr.]

Mass removing adt1.0.0, and adding adt1.0.1 because, we are now on 1.0.1.

Comment 47

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

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

Comment 48

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

Please email adt and drivers for approval for the 1.0.1 branch, and add note
here when you have done that.

Comment 49

[sujay]

verified.

Comment 50

[Peter Van der Beken [:peterv]]

I did ask drivers and adt for approval a few days ago. adt replied with a
question about the seriousness and I responded. No further response from adt and
no response at all from drivers.

Comment 51

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

Without this patch we will effectively forget the following attributes when
saving a document:

checked
compact
declare
defer
disabled
ismap
multiple
noresize
noshade
nowrap
readonly
selected

Some of those are pretty common, so it is a rather serious dataloss in the
serializer (seemingly in the editor).

Additionally, if someone tries to save an XHTML file and then read it with a
validating parser, they would get a parser error.

Comment 52

[Paul Wyskoczka]

Adding adt1.0.1+ on behalf of the adt for checkin to the 1.0 branch.  Please get
drivers approval before checking in. When you check this into the branch, please
change the mozilla1.0.1+ keyword to fixed1.0.1

Comment 53

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 54

[Peter Van der Beken [:peterv]]

Checked in on the branch.

Comment 55

[sujay]

verified in 7/24 branch