134298 - SaveAs leaves private [-|_]moz attributes in the output

Status: VERIFIED FIXED

(Core :: XML, defect, P3)

Description

[rbs]

Spin off of bug 126669

Comment 1

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

Hmm... I can easily strip off attributes that start with '-', because I know
those are invalid. However, how do I know what other attributes were generated
by Mozilla? We cannot trust the attribute name, so it is not safe to strip _moz.

Comment 2

[Akkana Peck]

We should standardize on using either moz- or moz_ for our own attributes that
should be removed.  When I was working on the serializers, I made a pass through
at one point and made sure they were all the same (can't remember now whether it
was - or _ that we agreed was more correct). But those had moz at the beginning
of the attribute name, not at the end, so it sounds like the -moz or _moz
attributes are ones that have been added more recently for other purposes (rbs
mentions mathml in bug 126669).  Perhaps those should be renamed to be compliant
with the other special attributes we already use and know how to strip out?

Comment 3

[rbs]

Yep, I have been using '-moz*' for mathml purposes. The motivation of using that
form (while not knowing that the was a proposal for standardization) was that
these are guaranteed to be invalid, i.e, they can never collide with
user-defined attributes. The XML parser prompts a fatal error if someone tries
to use such attributes externally.

Comment 4

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

Akkana: but but but.... what if the input file contained _mozfoo attribute? We'd
just generate another dataloss bug. Is this really how we are doing it for HTML???

Comment 5

[Akkana Peck]

The cases I'm aware of involve the editor adding tags that either shouldn't be
part of the serializer's output, or should be treated slightly differently by
the serializer, so a _moz* attribute is added to the tag as a signal to the html
or plaintext serializer that they need to do something special with it.  And
yes, this means that if someone tried to serialize a document that actually had
_mozfoo attributes, the attributes would be lost or would cause unexpected
behavior in the output, which we don't worry about because they're not legal
html attributes.

Comment 6

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

Attachment: Proposed fix

Fix for XML serializer. Is the same needed for HTML?

Comment 7

[rbs]

Comment on attachment 77701 [details] [diff] [review]
Proposed fix

r=rbs -- patch is okay for the XML side (standardization between XML/HTML might
help later).

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 77701 [details] [diff] [review]
Proposed fix

sr=jst

Comment 9

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

Saving a MathML file from Mozilla makes it non-wellformed (and we cannot open it
again in Mozilla). Trivial, safe fix. Limited to serializing XML.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 77701 [details] [diff] [review]
Proposed fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 11

[Jaime Rodriguez, Jr.]

adt1.0.0- because we don't believe this will affect many users

Comment 12

[rbs]

What does adt means?

Comment 13

[rbs]

-> re-assigning to myself for checkin as a non-netscaper since adt doesn't apply 
to the time and expertise that non-netscapers are putting in beautifying Mozilla 
even more.

Comment 14

[rbs]

Checked in.

Comment 15

[rbs]

Attachment: additional patch for the HTML content serializer

I am trying to fix bug 49721 - "View Partial Source" - The aim of that bug is
to allow someone to select a piece of text, and to show the underlying markup
of the selection in the typical syntax highlighted view-source way. (This could
for example allow debugging/capturing live things generated via JavaScript.) To
get the fix requires creating a range and extracting the markup with innerHTML,
i.e., the HTML content serializer is on the way... This causes the private -moz
attributes to show up and it is a mess. So seeking r/sr to finish off the HTML
side.

Comment 16

[Akkana Peck]

Two comments on the patch:
(1) Those NS_ConvertASCIItoUCS2 calls are grody and shouldn't be there.   I
understand you were just copying the existing style, but that style undoubtedly
came from someone's "mass global substitute in 300 files and don't review the
result) and we shouldn't make it worse.  Can you change the #defines to
#define kMozStr NS_LITERAL_STRING("_moz")
#define kDashMozStr NS_LITERAL_STRING("-moz")
and then you can remove the NS_ConvertASCIItoUCS2().get() part and it'll work
more efficiently.
(2) Maybe it's just how it came out in patch, but the indentation seems weird --
usually I'd expect the || to be indented to the same level as the first
nsCRT::strncmp (then the next two lines indented accordingly).

Fix those, and r=akkana.

I have a comment about "view partial source" but I'll go read bug 49721 and
comment there if it still applies.

Comment 17

[rbs]

Attachment: iteration on the additional patch - speedup

OK, I have tightened the whole lot to reduce the overhead (e.g., function
calls) for the common cases where the attributes aren't [_|-]moz*.

Comment 18

[rbs]

Comment on attachment 78817 [details] [diff] [review]
iteration on the additional patch - speedup

Adding flag: r=akkana

> I have a comment about "view partial source" but I'll go read bug 49721 and
> comment there if it still applies.

yes, comments are welcome.

Comment 19

[rbs]

jst, care to sr attachment 78817 [details] [diff] [review]? thanks.

Comment 20

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

If sharedName == 0, you'll get a crash at *sharedName.

Comment 21

[rbs]

Since it is an atom (an attribute atom), it can't be zero -- otherwise it will
be identicative that something is broken elsewhere (i.e., an attrName=value pair
where 1) the attrName is nil and 2) nil atom creation is being allowed).
However, if these are possible scenari, I could add a null check then.

Comment 22

[Chris Petersen]

Making verified on Mozilla trunk (2002-04-12-03).

Comment 23

[rbs]

-> re-opening to finish off the HTML side which is detrimental to my other view
partial source work.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 78817 [details] [diff] [review]
iteration on the additional patch - speedup

+#define kMozStr NS_LITERAL_STRING("moz")
[...]
+    if ((('_' == *sharedName) || ('-' == *sharedName)) &&
+	 !nsCRT::strncmp(sharedName+1, kMozStr.get(), kMozStr.Length())) {

Eek, both of the above kMozStr's will end up expanding to two nsAutoString's on
Linux (and nsDependentString's on other platforms, which is cheaper, but
still...), just to get the trivial string "moz" and the length of that string.
Maybe not something that will ever show up on profile runs, but it just hurts
my eyes to see this. Same things further down in the patch.

I can't sr this, find a cheaper way...

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 78817 [details] [diff] [review]
iteration on the additional patch - speedup

+#define kMozStr NS_LITERAL_STRING("moz")
[...]
+    if ((('_' == *sharedName) || ('-' == *sharedName)) &&
+	 !nsCRT::strncmp(sharedName+1, kMozStr.get(), kMozStr.Length())) {

Eek, both of the above kMozStr's will end up expanding to nsAutoString's on
Linux (and nsDependentString's on other platforms, which is cheaper, but
still...), just to get the trivial string "moz" and the length of that string.
Maybe not something that will ever show up on profile runs, but it just hurts
my eyes to see this. Same things further down in the patch.

I can't sr this, find a cheaper way...

Comment 26

[Johnny Stenback (:jst)]

Argh, I hate it when bugzilla does that to me...

Comment 27

[rbs]

Attachment: iteration to make jst happy

Comment 28

[rbs]

Comment on attachment 78959 [details] [diff] [review]
iteration to make jst happy

passing on r=akkana

Comment 29

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

If valueStr is not quaranteed to be flat, then going .get()+1 might not be safe(?)

Comment 30

[rbs]

valueStr is a nsAutoString -- can't nsAutoString be bumpy, er.., non-flat?

Comment 31

[rbs]

s/can't/can/

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 78959 [details] [diff] [review]
iteration to make jst happy

AFAIK, any string API that has a .get() method on it is guaranteed to be flat.

sr=jst

Comment 33

[rbs]

Checked in the trunk. Have asked drivers for a= on the branch. Leaving open to 
see what drivers think.

Comment 34

[Asa Dotzler [:asa]]

Comment on attachment 78959 [details] [diff] [review]
iteration to make jst happy

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 35

[rbs]

Checked in the 1.0 branch.

Comment 36

[rbs]

In bug 126669, akkana wrote:
> Kathy thinks that this has regressed composer -- on empty documents, composer
> is now outputting a single br tag, which comes from composer's bogus node,
> which should look like: <br _moz_editor_bogus_node>.  I don't understand why
> this would have caused that, but I'm cc'ing Kathy and Joe for discussion.

I am starring at attachment 78959 [details] [diff] [review] but I have no luck seing something suspicious.
Was something overlooked in the patch?

Comment 37

[rbs]

Stepped in the debugger and all looked well. Then, it occured to me that the
bug/regression has been there for some time. Rather than just skipping the _moz
attribute, what you want is to skip the whole <br> tag once it is determined
that it has the '_moz_editor_bogus_node' attribute. That's another story.

Comment 38

[Anatoliy Averbukh]

This caused the following problem:

And looks like this function eliminates the attribute.

Simple test shows that:

a. Open Composer with <HTML> source view
b. Type in:
   <span -moz-smiley="s1"><span>:-)</span></span>

c. swith to "Normal" view and switch back to <HTML> view

   Now, you can see:
      <span><span>:-)</span></span>

The attribute -moz-smiley="s1" disappeared.

The same test on my build from 04/11 worked fine.

It is important for Smiley and broke them.

Comment 39

[rbs]

That is understandable.

The intention is to deliberate remove the various [_|-]moz attributes so that
they are kept private/internal when passing things around because it is hard to
emulate what these attributes do by other means at this stage (witness the
recent unsuccessful crusade of bryner to get rid of -moz-checked). But as the
infrastructure improves, these -moz attributes may go away at some stage. (jst
even noted that the DOM may deny to create invalid attributes :-)

Maybe the smiley doesn't need to complicate its life with a -moz attribute and
can just use a normal one? e.g., class="..."? Care to fill me with some
background on why the smiley is doing what it is doing?

Comment 40

[Anatoliy Averbukh]

Please restore old behavior by letting -moz* attribute. We cannot change that 
in the near future (for incomming release). And now it breaks existing 
functionality (for at least 3 applications).

Thank you.

Comment 41

[Johnny Stenback (:jst)]

-moz-* attributes should *never* be typed in anywhere, they're for internal use,
not for external use.

Comment 42

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Johnny : a -moz-* attribute is the way we use to generate smileys for Instant
Messenging though the editor. As Anatoliy stated it, I doubt it can easily be
changed for the coming release....

Comment 43

[rbs]

Advocating to reverse the fix upon the Mozilla tree seems very poor and looks
like a no-no to me since the bug was also fixed for a reason -- not out of
fantasy. There are still several months before the next Nestcape release, giving
enough time to:

s/-moz-smiley="s1"/class="smiley1"/g

which will be a much cleaner and long lasting investment.

Comment 44

[Chris Petersen]

Changing QA contact to rakesh.

Comment 45

[Rakesh Mishra]

As per my correspondence with Roger B. Sidje, [e-mail], regarding
steps to reproduce this bug before the patch fixed it up, 

Roger wrote:

You cannot see the problem directly from "Save As". What the patch
fixed was another codepath that was involved in "View Selection Source"
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=79770

Before the patch: if you tried "View Selection Source", the
view-source output would have come up with the private attributes
(if they were there).

After the patch: these attributes don't show up. The
problem is that "View Selection Source" was checked in later
on... and so there is no build that has "View Selection Source"
without having the first patch... It was during the
development of "View Selection Source" in bug 122524 that I saw
the remaining -moz problem and fixed it.

Hope this helps...

RBS

On the basis of above comments and with the testcase under the URL
http://www.mozilla.org/projects/mathml/demo/basics.xhtml, verified on the trunk
build 2002-05-29-08-trunk on windows 2000 and on the branch build
2002-05-29_08-1.0.0 on Windows 2000 .
The private [-|_]moz attributes are no more in the output.

Marking verified 
Adding "verified1.0.0" to the keyword