380364 - when I right click on tinderbox, I get "Search Google for <tr></tr>"

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[(not reading, please use [e-mail] instead)]

when I right click on tinderbox, I get "Search Google for <tr></tr>"

steps to reproduce:

1) go to http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox
2) right click

here comes a screen shot.

Comment 1

[(not reading, please use [e-mail] instead)]

actually, you may need to try to right click on a name in "Guilty" column first.

but once in this state, right clicking anywhere on tinderbox seems to cause the problem.

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of the bug

Comment 3

[(not reading, please use [e-mail] instead)]

note, I'm using my own trunk, debug build:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070510 Minefield/3.0a5pre

Comment 4

[:Gavin Sharp [email: [e-mail]]]

This seems like a regression with window.getSelection(). The browser uses it to get the selected text and generate the context menu. With a trunk build, after clicking on an empty space in the "guilty" column, running "javascript:alert(window.getSelection())" gives me "<tr></tr>", while on a branch build it gives me a blank string.

Comment 5

[Elmar Ludwig]

I also see this on Linux (20070510 nightly trunk build), not just on tinderbox but on other pages as well. Seems to be triggered by "selecting" an empty table cell, as Gavin already noted. Selecting non-empty cells leads to: Search Google for "<tr></tr>..." (replace the dots with the beginning of the cell contents).

Comment 6

[Elmar Ludwig]

Attachment: simple testcase

Trivial testcase to reproduce the bug: left click on the space between "A" and "B", then right click to get context menu

Comment 7

[Elmar Ludwig]

Regression range: 2007-03-27-04-trunk (ok) - 2007-03-28-04-trunk (bug)

Looking at the checkins in this range points to the fix for bug 236546 as a likely regression cause, so marking as blocking and updating component accordingly.

Comment 8

[Jeff Schiller]

Yep, this is a regression of bug 236546.

Question:  Why is the branch behavior such that left-clicking an empty table cell causes the string selection to be an empty string?  Is this desired behavior?

Comment 9

[u88484]

This is also happening on CNN.com's latest news headline links

Comment 10

[Jeff Schiller]

Attachment: Another test case (HTML)

Comment 11

[Jeff Schiller]

Attachment: Another test case (XHTML)

Comment 12

[Jeff Schiller]

Attachment: First attempt at patch

This patch ensures that the only time we inject <tr></tr> tags is in an HTML serialization.  This works as expected for HTML, but for the XHTML test case.  In XHTML table cell selection, it appears that mMimeType in nsDocumentEncoder is set to "text/plain".  Does anyone know why?

Comment 13

[Jeff Schiller]

Clarification (due to typo):  This patch works for HTML, but not for XHTML.

Shouldn't the MIME type of an XHTML be set to "application/xhtml+xml" or at least "text/html" instead of "text/plain" ?

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 265634 [details] [diff] [review]
First attempt at patch

>Index: nsDocumentEncoder.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsDocumentEncoder.cpp,v
>retrieving revision 1.120
>diff -u -8 -p -r1.120 nsDocumentEncoder.cpp
>--- nsDocumentEncoder.cpp	27 Mar 2007 15:10:23 -0000	1.120
>+++ nsDocumentEncoder.cpp	22 May 2007 05:42:22 -0000
>@@ -900,46 +900,49 @@ nsDocumentEncoder::EncodeToString(nsAStr
>     PRInt32 i, count = 0;
> 
>     rv = mSelection->GetRangeCount(&count);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     nsCOMPtr<nsIDOMNode> node, prevNode;
>     nsCOMPtr<nsIContent> content;
>     PRBool bTableCellsFound = PR_FALSE;
>+    PRBool bIsHTML = mMimeType.Equals(NS_LITERAL_STRING("text/html")) || 
>+                     mMimeType.Equals(NS_LITERAL_STRING("application/xhtml+xml"));

I don't think our encoder code knows anything about XHTML yet, so it should be ok to just deal with text/html here. Note that this mimetype isn't representing the mimetype of the data we're encoding, it represents the mimetype of the output we're creating, and we don't have an XHTML encoder yet that I know of.

r+sr=jst with that changed.

Comment 15

[Jeff Schiller]

jst, i checked through this a little, it seems that for XHTML documents, the Mime Type of the nsSelection is being set to text/plain because of the following line in nsHTMLCopyEncoder::SetSelection():

  if (!htmlDoc || mDocument->IsCaseSensitive())
    mIsTextWidget = PR_TRUE;

In this case, for XHTML documents, mDocument->IsCaseSensitive() is true.  Looks like bzbarsky made this change a few years ago:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/base/src&command=DIFF_FRAMESET&file=nsDocumentEncoder.cpp&rev2=1.102&rev1=1.101

I'm tempted to remove this condition, but I have no idea why it was made (can't seem to figure out how to get a Bug # from it).

Btw, I also wondered if overriding nsDocumentEncoder::EncodeToString() in nsHTMLCopyEncoder would be a better idea instead of checking the Mime Type like that.

Comment 16

[Jeff Schiller]

> Note that this mimetype isn't representing
> the mimetype of the data we're encoding, it 
> represents the mimetype of the output we're 
> creating, and we don't have an XHTML encoder 
> yet that I know of

Can't we just create HTML output from XHTML selections?  

Comment 17

[Jeff Schiller]

Ok, here's bzbarsky's change: http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/content/base/src/nsDocumentEncoder.cpp&rev=1.102 and it deals with Bug 270145

Comment 18

[Jeff Schiller]

Attachment: Patch with jst's comments

Updated with jst's comments

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Can't we just create HTML output from XHTML selections?  

No.  Some XHTML documents can create DOMs that cannot be serialized as HTML.  So this patch is probably the way to go.

Is there a reason the patch doesn't use EqualsLiteral()?

Comment 20

[Jeff Schiller]

> Is there a reason the patch doesn't use EqualsLiteral()?

Yes, the reason is ignorance on my part ;)

I notice that other sections of the code use LowerCaseEqualsLiteral(), that probably should be the real fix.  I'm confused by the process - if the Bug is marekd "RESOLVED FIXED", can I still submit a patch for it?

Comment 21

[Jeff Schiller]

Attachment: Uses LowerCaseEqualsLiteral()

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265759 [details] [diff] [review]
Uses LowerCaseEqualsLiteral()

You probably want a diff against the current trunk (with the previous patch checked in), right?  And yes, just putting this followup on this bug is fine.  Thanks for doing it!

Comment 23

[Jeff Schiller]

Boris - the patch never was checked in, I erroneously marked it as "FIXED" earlier.  This patch still needs to be applied, and it is against the current trunk.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  That last patch should be good to go then -- it looks good to me, and the r+sr=jst probably stands.

Comment 25

[Jeff Schiller]

Ok, but I need someone else to check it in, I don't have that power...

Comment 26

[:Gavin Sharp [email: [e-mail]]]

That's what the [checkin needed] marker in the status whiteboard is for - some people regularly search for that and check in patches that need checking in. Alternatively, you can find someone on IRC to land this for you.

Comment 27

[[e-mail]]

Attachment: My take on the fix

This code is more generic so is less likely to go wrong if someone changes the way the encoders work or even implements a new encoder.

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 265860 [details] [diff] [review]
My take on the fix

This should work too, r+sr=jst for this one as well.

Comment 29

[Jeff Schiller]

Ok, it looks like the last patch by Neil was checked in apparently.  Do I just resolve the bug and mark it Fixed?

Comment 30

[Elmar Ludwig]

Yes (I'm doing that now), this was probably just an oversight.

Fixed with Neil's patch in:

mozilla/content/base/src/nsDocumentEncoder.cpp 	1.121   (2007-05-25 13:47)