229526 - Using html markup in dialogs connects the 2 words surrounding the tag to one word

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jaap A. Haitsma]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20031206 Firebird/0.7+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20031206 Firebird/0.7+

If you have xul code in a dialog which looks like this
<description>
    This is normal text
    <html:i>this is italics text</html:i>
</description>

"text" and "this" will be connected into one word. There is no space between the
two word. Also a line will never break between "text" and "this". So I guess
mozilla somehow sees them as one word.

Reproducible: Always

Steps to Reproduce:
1. See details
2.
3.

Actual Results:  
words get connected into one word

Expected Results:  
keep treating words as seperate words

Comment 1

[Boris Zbarsky [:bzbarsky]]

Inter-tag whitespace is stripped out in XUL, which I bet is the problem here.

Comment 2

[Jaap A. Haitsma]

I already got the same answer on a mailing list. There's probably a good reason
to do so, but in this case IMHO xul should not do this

Comment 3

[[e-mail]]

Can anyone see an issue with ignoring whitespace nodes only, and not trimming
other text nodes?

Comment 4

[[e-mail]]

Attachment: Like this

Comment 5

[Boris Zbarsky [:bzbarsky]]

I dunno.  It's all fine with me as long as it doesn't come up and bite XUL
authors.  Would something like:

This is a la<label>
be
</label>
l

be expected to have whitespace in the display?

Comment 6

[[e-mail]]

I'm not sure what you mean there - the text before and after the <label> won't
display because there's nothing to display it; labels will trim their html
before displaying it (as if they were a <div>).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  In that case the patch looks fine to me; someone like hyatt, ben, or
jag probably needs to approve it...

Comment 8

[jag (Peter Annema)]

Comment on attachment 138131 [details] [diff] [review]
Like this

>Index: nsXULContentSink.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xul/document/src/nsXULContentSink.cpp,v
>retrieving revision 1.127
>diff -p -u -d -r1.127 nsXULContentSink.cpp
>--- nsXULContentSink.cpp	18 Nov 2003 18:19:59 -0000	1.127
>+++ nsXULContentSink.cpp	29 Dec 2003 22:18:06 -0000
>@@ -658,14 +658,12 @@ XULContentSinkImpl::FlushText(
>         if (mState != eInDocumentElement || mContextStack.Depth() == 0)
>             break;
> 
>-        // Trim the leading and trailing whitespace
>         nsXULPrototypeText* text = new nsXULPrototypeText();
>         if (! text)
>             return NS_ERROR_OUT_OF_MEMORY;
> 
>         text->mValue.SetCapacity(mTextLength + 1);

Not your code, but:

there's really no need for SetCapacity here. SetCapacity is only useful when
you're doing multiple concatenations to get your end result; for Assign and
Append we already do SetCapacity on the length of the end result if the current
capacity doesn't suffice. In the case that mValue isn't empty, the above
SetCapacity is really useless since you'd want |SetCapacity(mValue.Length() +
mTextLength)| (and no + 1, strings themselves take care of accounting for zero
terminators).

>         text->mValue.Append(mText, mTextLength);

That should probably become Assign.

>-        text->mValue.Trim(" \t\n\r");

So, I'm unsure about removing this. Is there some other mechanism that will
strip leading and trailing whitespace for this case (note two trailing spaces
after def):

<description>
  abc  def  
</description>

With your patch, do we store this in mValue as "\n  abc  def  \n", as " abc def
", or as "abc def"? (white space preserved, collapsed, and collapsed & trimmed,
respectively) I don't recall exactly what Expat feeds us, perhaps it's nice
enough to collapse white space for us and trim it from the first and last text
fragments, in which case we're all set, since "abc def" is what we want. I
think. It's certainly how it behaves right now (very html-like).

I can't give you r= or sr=, I just don't know enough about how this code works
right now, and don't really have the time to play with it. Based on how AddText
works (it simply appends what it gets from Expat), and what I currently see for
my test case, I'd say Expat at least collapses white space. I wouldn't be
surprised if it trims leading white space from the first text fragment and
trailing from the last (which can be one and the same).

Could you test the above <description> example and the one at the top in this
bug with your patch and make sure we get the desired results ("This is normal
text this is italics text")? If you do, and perhaps with some printfs could
confirm that Expat is indeed collapsing and trimming, I'd feel confident enough
to r or sr= this patch as the right thing to do.

Hmmm, so if it collapses and trims, and we ignore white space only text
fragments, what happens in this case:

<description> <html:i>italic</html:i> <html:b>bold</html:b> </description>

It looks like we'll get "italicbold". So you might have to remove the
IsDataInBuffer check from FlushText to make that test work.

Comment 9

[[e-mail]]

Ah, right, the italicbold case won't work, because the blank between the tags
will still be removed... the <description> seems to do the collapsing/trimming
as appropriate, as you can tell if you use script to write text nodes.

Exaggerated workaround for the italicbold case:
<description> <html:i> italic </html:i> <html:b> bold </html:b> </description>

Comment 10

[jag (Peter Annema)]

<description> <html:i> italic </html:i> <html:b> bold </html:b> </description>

Wouldn't this become

"italic  bold"? *confused* I guess there's some really smart white space
collapsing going on if not.

So does removing the IsDataInBuffer check fix the "italic bold" case?

Comment 11

[[e-mail]]

The <description> appears to obey HTML collapsing rules, so I assume you could
restore the white space by using style="white-space: pre;" - I didn't try
removing IsDataInBuffer because it will probably break the browser.

Comment 12

[jag (Peter Annema)]

I don't think it will break the browser, the only time that should be triggered
is for empty text sections, which I think you'll only find for things like the
"italic bold" example.

And hmm, yeah, there is some smart white space collapsing going on.

In HTML, the following:

<p>
some <i> italic </i> <b> bold </b> <u> underlined </u> text
</p>

becomes

some italic bold underlined_text

Where the space between "underlined" and "text" is also underlined (Safari
renders it that way too). I can only assume (but it would make sense) that the
space after "bold" is bold formatted, and the space after "italic" is italic.

What does the following XUL do with your patch?

<?xml version="1.0"?> 
<window id="main-window"
        xmlns:html="http://www.w3.org/1999/xhtml"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
<description> some <html:i> italic </html:i> <html:b> bold </html:b> <html:u>
underlined </html:u> text </description>
</window>

Comment 13

[[e-mail]]

It looks just like the HTML does...

BTW, I tried disabling the IsDataInBuffer check - parts of Mozilla worked ;-)

Comment 14

[jag (Peter Annema)]

Ugh, that's nasty. We should really not depend on whitespace between tags not
being stripped out. I guess I'll have to play with this to see what parts didn't
work and why not. For now fixing the example in comment 0 is a good start, but
we'll need to fix the "italic bold" thing too.

sr=jag on removing the Trim, would like to see SetCapacity removed and Append
changed to Assign.

Comment 15

[[e-mail]]

Attachment: As per jag's comments

Comment 16

[David Hyatt]

How about this.  Both XUL and XBL should strip whitespace based off the current *element*.  If the 
element is a XUL element, then whitespace should be stripped.  If the element is a <description> or 
<label>, then whitespace should not be stripped.  If the element is not a XUL element, then whitespace 
should not be stripped.

I think implementing this behavior for both XUL *and* XBL will retain backwards compatibility with 
current Mozilla chrome while ensuring correct behavior.

Comment 17

[[e-mail]]

Attachment: XBL vs XUL test case

Note that XBL does not trim text nodes, but does drop whitespace-only nodes.

Comment 18

[jag (Peter Annema)]

neil: interesting. 

http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLContentSink.cpp#172
 confirms that we strip white space only nodes, but don't Trim.

Your test case doesn't show the visual effect of white space only nodes getting
dropped though, this would:

    some <html:i>italic</html:i> <html:b>bold</html:b> <html:u> underlined
</html:u> text

Btw, I played with a simple html document and the DOM inspector yesterday and it
looks like in html we don't even strip the white space from the DOM, the
collapsing happens when the document gets rendered. So I guess the same's going
on for <description> and <label>. So hyatt's suggestion makes sense to me now,
when we're inside those two elements (in the XUL namespace), don't Trim and
don't strip white space only nodes, for the rest, do.

So for <label><html:i>italic</html:i></label>, does XULContentSink::AddText get
"italic" or does that go somewhere else? And if it does, can we just walk the
parent chain to find out if we're somewhere inside a <xul:label> or
<xul:description>?

Hmmm, actually, could we use xml:space="preserve" (by e.g. putting it on the
binding's <children>?) as a hint that in these two elements white space should
be preserved, and make XUL's default be the current mode? That seems cleaner
than special casing just these two XUL elements.

hyatt?

Note that we might have to do this in both XULContentSink and XBLContentSink.

Comment 19

[jag (Peter Annema)]

It just dawned on me where the problem lies in not stripping out white space
only nodes in XUL. While this is fine for content inside <description> and
<label>, the rest of XUL would get a bunch of text nodes added that JS amongst
others isn't expecting.

So what would be an example for XUL where we still want/need to Trim? As far as
I know in XUL non white space text nodes can only appear in <description> and
<label>, so we can safely remove Trim like neil's patch does.

Comment 20

[[e-mail]]

Attachment: As per hyatt's comments

OK, so this patch does not change the behaviour for XBL or XUL, except for
label and description, which no longer trim or strip whitespace.

It would have been shorter if I had completely stopped trimming XUL.

Comment 21

[jag (Peter Annema)]

So I talked to hyatt and he agrees on having support for xml:space, but we still
might want to special case xul:description and xul:label to avoid increased
memory usage from putting that attribute on all those elements. I wonder, if we
put it on the binding, could we only use it as a parsing directive without
putting it on the XUL element (thereby avoiding the memory issue)?

Comment 22

[[e-mail]]

In that case it's beyond my skills to implement.

Comment 23

[Brendan Eich [:brendan]]

Yeesh, way to drive Neil away! ;-)

Confirming and cc'ing some people who can probably help.

/be

Comment 24

[David Hyatt]

I think concerns over xml:space can be addressed separately in a later patch.  Even with support for 
xml:space ,we still have to special-case label and description, since anyone who makes their own 
binding for label/description should not have to think about having to put xml:space="preserve" 
on their binding in order to get behavior that really should be the default.

r/sr = me on neil's latest patch.

Comment 25

[Brendan Eich [:brendan]]

Neil, pls. see last two comments.

/be

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 27

[[e-mail]]

Fix checked in. Sorry for not searching for a dup...

Comment 28

[[e-mail]]

Ok, so we've got a regression already :-)

textbox.xml has the following anonymous content:

<html:textarea>
  <children/>
</html:textarea>

Is the correct thing here to close up the tags onto one line?

Comment 29

[David Hyatt]

Yes, patch the XBL file.

Comment 30

[Boris Zbarsky [:bzbarsky]]

This caused bug 232545 (the XBL content sink changes did; I assume there may be
similar issues in XUL).