Samuel Sidler (old account; do not CC) Reporter
	Description • 16 years ago

Bug 428096 landed without tests on the trunk (prior to the branching of 1.9.1) without them. So far there have been two major regressions as a result of its landing. We should have tests for this bug.

Jonathan (who was not at all responsible for the initial landing) has said he'd be willing to try and get some tests landed for bug 428096 given his work on one of its regressions (bug 466599).

I still stand by the bug 470384 comment 6. Bug 428096 should never have landed without tests.

Thanks for taking a look at creating tests for this Jonathan.

	Atul Varma [:atul]
	Comment 1 • 16 years ago

Thanks for looking into this, Jonathan!

I apologize for not submitting any tests for bug 428096. Josh Aas had asked me if I could get some in shortly after the code landed and I said yes, but I dropped the ball on it.  It was also silly of me to not at least *manually* try out my patch for bug 428096 on content with non-ascii characters... Sorry about that.

	Josh Aas
	Comment 2 • 16 years ago

We should definitely get these tests done but I don't see us blocking release on them. This bug does not reflect a real bug in Gecko, only a lack of tests.

	Jonathan Kew [:jfkthame] Assignee
	Comment 3 • 16 years ago

Here's an initial attempt at testing for rich-text support when we send data to the system pasteboard. It's not exhaustive (e.g., doesn't test drag-and-drop usage, only the Copy operation), but at least it's a start.

The approach I took is to use an onload handler to trigger Cmd-A (select all) and Cmd-C (copy) events in the test page, as I didn't know how to simulate these from the script otherwise. Then retrieve the data from the global clipboard, checking that it can provide both text/unicode (plain text) and text/html (rich text), and that they each have reasonable-looking content.

	Josh Aas
	Comment 4 • 16 years ago

Comment on attachment 356999 [details] [diff] [review]
chrome mochitest to check that "copy" puts rich text on clipboard

>+    pastetext = str.data.substring(0,strLength.value / 2);

Missing a space before the second argument to substring

Should we add a focus call to make sure we're copying what we think we're copying with the select-all command or does that not matter because of where you're sending the event?

	Jonathan Kew [:jfkthame] Assignee
	Comment 5 • 16 years ago

(In reply to comment #4)

> Should we add a focus call to make sure we're copying what we think we're
> copying with the select-all command or does that not matter because of where
> you're sending the event?

It's worked as-is for me in numerous test runs, both with and without the relevant code patches (to verify that it would have failed with older versions).

My guess would be that focus doesn't matter because the script uses document.dispatchEvent, but I'm completely new to both Javascript and the testing frameworks, so I'd appreciate advice from anyone who knows better!

I'll update the patch tomorrow, heading for bed now.

	Jonathan Kew [:jfkthame] Assignee
	Comment 6 • 16 years ago

I've added the missing space (and another similar instance!). Also added an explicit focus() call, although I doubt this is necessary - perhaps it'll help ensure robustness some day.

	Josh Aas
	Comment 7 • 16 years ago

Comment on attachment 357409 [details] [diff] [review]
updated patch to address joshmoz comments

It is unfortunate that we have to code against keyboard shortcuts that are not guaranteed, but those are safe enough bets that it'll do for now.

	Josh Aas
	Comment 8 • 16 years ago

Comment on attachment 357409 [details] [diff] [review]
updated patch to address joshmoz comments

Actually, will this work with all languages? I don't know if cmd-a and cmd-c are usable on all systems. Can we just put data on the clipboard the same way we pull it off, via transferable?

	Jonathan Kew [:jfkthame] Assignee
	Comment 9 • 16 years ago

It's quite possible that Cmd-A and Cmd-C are not correct for all localized systems, though I don't know for sure. If there's a way to simulate a user-level Copy operation without using this, that'd be great, but I don't know how to do that from a test (or from a separate testing tool, unless we require the Universal Access prefs to be set up to "enable access for assistive devices").

The test I wrote for bug 466599 does use transferable to put data on the clipboard, but that means the test script itself is determining which data format(s) to supply; here, that's no good because it would bypass the whole point of this bug: the original issue was that we were not putting all the appropriate formats onto the clipboard during a Copy operation.

There may be a more robust way to trigger the Copy operation from the test script, but I don't know enough about the frameworks yet. Suggestions welcome from anyone who knows where to look! Meanwhile, I think Cmd-A and Cmd-C should be OK for the vast majority of situations, and in particular I assume our normal automated tests are run on English systems.

	Al
	Comment 10 • 16 years ago

Is this blocking landing of bug # 428096 which has been marked as complete?

	Jonathan Kew [:jfkthame] Assignee
	Comment 11 • 16 years ago

No, 428096 is landed. It'd still be nice to have some kind of test in place, though; joshmoz, are you willing to r+ this, or do you have alternatives to suggest?

	Josh Aas
	Comment 12 • 16 years ago

Sorry if I'm misremembering here, but if this is supposed to test bug 428096, and bug 428096 added the ability to pull HTML out of the global clipboard transferable and put it on the native clipboard then you should be able to call nsIClipboard's SetData with a transferable you built to get the Mac OS X copy and paste code to export the HTML in your transferable to the native clipboard. No need for cmd-a cmd-c. We're not testing whether or not copying HTML in gecko puts HTML on the transferable itself.

Testing that native export is tricky though. You can't use GetData (which is what the current test does) because that is smart enough to just pull from the cached transferable and not the native clipboard and the whole point is to test whether or not the HTML flavor was exported to the native clipboard. I'm not sure what to do here, we'd probably need some mechanism to ensure that a pull skipped the cached transferable.

	Jonathan Kew [:jfkthame] Assignee
	Comment 13 • 16 years ago

Re-reading this, I note (see comment #5) that I definitely tried the test both with and without the patch for bug 428096, and verified that without the patch it fails, and with the patch it passes.

I don't really know much about the underlying code, but I thought the use of nsIClipboard's kGlobalClipboard meant that the readFromClipboard() function in the test was pulling data from the system clipboard (not a private one). This seems to be what https://developer.mozilla.org/en/Using_the_Clipboard is saying. It keeps talking about getting "the system clipboard object" with code like this.

Could you take another look and see what is going on here? AFAICT, the test should really be pulling data from the system ("global") clipboard, and so it does test whether we placed the expected flavors there. (Though whether it's testing the "push" or "pull" side of that operation is unclear, I guess!)