Closed
Bug 380364
Opened 18 years ago
Closed 18 years ago
when I right click on tinderbox, I get "Search Google for <tr></tr>"
Categories
(Core :: DOM: Serializers, defect)
Core
DOM: Serializers
Tracking
()
RESOLVED
FIXED
People
(Reporter: moco, Assigned: codedread)
References
()
Details
(Keywords: regression, testcase)
Attachments
(6 files, 2 obsolete files)
5.74 KB,
image/png
|
Details | |
331 bytes,
text/html
|
Details | |
988 bytes,
text/html
|
Details | |
1.05 KB,
application/xhtml+xml
|
Details | |
2.21 KB,
patch
|
Details | Diff | Splinter Review | |
2.50 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•18 years ago
|
||
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.
Reporter | ||
Comment 2•18 years ago
|
||
Reporter | ||
Comment 3•18 years ago
|
||
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•18 years ago
|
||
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.
Updated•18 years ago
|
Keywords: regression
Comment 5•18 years ago
|
||
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).
OS: Windows XP → All
Hardware: PC → All
Comment 6•18 years ago
|
||
Trivial testcase to reproduce the bug: left click on the space between "A" and "B", then right click to get context menu
Comment 7•18 years ago
|
||
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.
Assignee | ||
Comment 8•18 years ago
|
||
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?
Status: NEW → ASSIGNED
Assignee | ||
Updated•18 years ago
|
Assignee: dom-to-text → codedread
Status: ASSIGNED → NEW
Assignee | ||
Comment 10•18 years ago
|
||
Assignee | ||
Comment 11•18 years ago
|
||
Assignee | ||
Comment 12•18 years ago
|
||
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?
Assignee | ||
Comment 13•18 years ago
|
||
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•18 years ago
|
||
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.
Attachment #265634 -
Flags: superreview+
Attachment #265634 -
Flags: review+
Assignee | ||
Comment 15•18 years ago
|
||
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.
Assignee | ||
Comment 16•18 years ago
|
||
> 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?
Assignee | ||
Comment 17•18 years ago
|
||
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
Assignee | ||
Comment 18•18 years ago
|
||
Updated with jst's comments
Attachment #265634 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
![]() |
||
Comment 19•18 years ago
|
||
> 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()?
Assignee | ||
Comment 20•18 years ago
|
||
> 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?
Assignee | ||
Updated•18 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•18 years ago
|
Status: REOPENED → ASSIGNED
![]() |
||
Comment 22•18 years ago
|
||
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!
Assignee | ||
Comment 23•18 years ago
|
||
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•18 years ago
|
||
Ah, ok. That last patch should be good to go then -- it looks good to me, and the r+sr=jst probably stands.
Whiteboard: [checkin needed]
Assignee | ||
Comment 25•18 years ago
|
||
Ok, but I need someone else to check it in, I don't have that power...
Comment 26•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
Comment on attachment 265860 [details] [diff] [review] My take on the fix This should work too, r+sr=jst for this one as well.
Attachment #265860 -
Flags: superreview+
Attachment #265860 -
Flags: review+
Assignee | ||
Comment 29•18 years ago
|
||
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•18 years ago
|
||
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)
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in
before you can comment on or make changes to this bug.
Description
•