549094 - [SeaMonkey] reftest: new 'editor/newline-1.html' is perma-orange

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Serge Gautherie (:sgautherie)]

Test     : "aaa"
Reference: "aaa bbb"

Comment 1

[Boris Zbarsky [:bzbarsky]]

So...  It looks like the default gecko value (in all.js) of editor newline handling is eNewlinesPasteToFirst.  Firefox overrides this with eNewlinesReplaceWithSpaces in firefox.js.

Which of course makes this test fail in any other Gecko app...

Note that the name of this pref is "editor.singleLine.pasteNewlines".  The fact that it affects DOM access in any way is ... unfortunate.  And doesn't seem right.  Certainly having such DOM access differences between Gecko consumers is wrong.

Comment 2

[[e-mail]]

(In reply to comment #1)
> So...  It looks like the default gecko value (in all.js) of editor newline
> handling is eNewlinesPasteToFirst.
Worse than that, it's platform-specific; although it's eNewlinesPasteToFirst on Windows and Mac, it's eNewlinesPasteIntact on Linux.

Comment 3

[Serge Gautherie (:sgautherie)]

This is the one test which keeps SeaMonkey reftest suite orange:
any solution, even temporary, would be very welcomed...

Comment 4

[[e-mail]]

So is the solution to add editor.singleLine.pasteNewLines to runreftest.py?

Comment 5

[(not currently active) Ted Mielczarek]

I'd vote for just changing the global default to match what Firefox does. Apps can still override it, and it can be set on a per-textbox basis nowadays, so it's not such a big deal.

Comment 6

[[e-mail]]

Please could you comment on how you would like pref-dependent reftests handled.

Comment 7

[Robert Kaiser]

I'm for changing the global pref to match Firefox here, at least, no matter what SeaMonkey wants. Firefox not matching global default is prone to make others runs into similar errors.

As for the SeaMonkey-specific case, do we even want to have a different default than Firefox here? Neil?

Comment 8

[(no longer active)]

(In reply to comment #3)
> This is the one test which keeps SeaMonkey reftest suite orange:
> any solution, even temporary, would be very welcomed...

The actual solution would be using the pref only for pasting, and handling normal newlines according to the HTML5 spec (bug 549475).  That bug depends on a number of other patches in my queue (which unfortunately aren't ready for landing yet.)

In the mean time, we could either change the global value of the pref to match Firefox, or add some kind of a condition to reftest manifests to only run the reftest when the value of the pref equals to the Firefox default value (if the global value of the pref shouldn't change.)

I very much prefer the latter solution.  Dbaron, what do you think?  (I can create the patch for either solution.)

Comment 9

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

(In reply to comment #6)
> Please could you comment on how you would like pref-dependent reftests handled.

crowder (I think) recently wrote a patch that adds getIntPref() to the reftest sandbox used in fails-if(), random-if(), etc.  Other pref functions could be added similarly.

In this case, however, it seems like setting the pref in firefox.js is probably a bug; I don't see why the behavior should differ between Gecko applications.  Why was the preference not put in all.js?

Comment 10

[(no longer active)]

(In reply to comment #9)
> (In reply to comment #6)
> > Please could you comment on how you would like pref-dependent reftests handled.
> 
> crowder (I think) recently wrote a patch that adds getIntPref() to the reftest
> sandbox used in fails-if(), random-if(), etc.  Other pref functions could be
> added similarly.

Very nice!  Of course, getIntPref is enough here.

> In this case, however, it seems like setting the pref in firefox.js is probably
> a bug; I don't see why the behavior should differ between Gecko applications. 
> Why was the preference not put in all.js?

The Firefox specific value for the pref is coming from bug 406062.  That bug doesn't give a rationale on why this change was Firefox-specific, though.

Comment 11

[(not currently active) Ted Mielczarek]

I think mconnor just wanted that behavior and didn't want to get into a pissing contest with people, so he changed it for Firefox where he had the authority to do so.

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> I think mconnor just wanted that behavior and didn't want to get into a pissing
> contest with people, so he changed it for Firefox where he had the authority to
> do so.

Which made us all land in a pissing contest because Firefox is not alone in the world. Interesting, isn't it?

Don't want to end up pointing fingers, just explain how once again a decision seems to have had the opposite effect of what was intended...

Comment 13

[(no longer active)]

Attachment: Patch (v1)

This patch changes the Gecko value of the pref to match what Firefox has right now.  The consensus seems to be that this is what we want.  SeaMonkey and other applications are still free to override it as they need.

I'm not sure who the best person to review this is, so I'm just throwing the request at Boris.  Boris, feel free to change it to someone else if needed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 436522 [details] [diff] [review]
Patch (v1)

Sure.

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/89e2b92f0b2a

Comment 16

[[e-mail]]

Attachment: Alternatively, add tests for each supported pref

Thus ensuring that at least something gets tested whatever your pref is.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 436569 [details] [diff] [review]
Alternatively, add tests for each supported pref

Ehsan's a better reviewer for this.

Comment 18

[(no longer active)]

Comment on attachment 436569 [details] [diff] [review]
Alternatively, add tests for each supported pref

This is a good idea, but it has three problems.

Firstly you shouldn't really be using newlines inside the reference file, because these are tests about what happens to newlines against the reference.  You may need multiple reference files, though.

Secondly, only one of these tests will ever get executed.  You should convert this to a mochitest which changes the pref and tests the effect of it.

Thirdly, this bizarre behavior will be going away in bug 549475, and will only be used for paste operations after that bug is fixed, so it might make sense to actually paste text in your mochitest instead of setting the value property/attribute.

Comment 19

[Serge Gautherie (:sgautherie)]

V.Fixed per Firefox and SeaMonkey tinderboxes.

Comment 20

[[e-mail]]

(In reply to comment #18)
> Firstly you shouldn't really be using newlines inside the reference file,
> because these are tests about what happens to newlines against the reference. 
> You may need multiple reference files, though.
That's just a naming convention. The files could easily be renamed newline.html (with the newlines) and newline-ref-?.html (individual references).

> Secondly, only one of these tests will ever get executed.  You should convert
> this to a mochitest which changes the pref and tests the effect of it.
This bug was originally filed because three of these tests would in fact have been executed, but attachment 436522 [details] [diff] [review] makes that moot.

> Thirdly, this bizarre behavior will be going away in bug 549475, and will only
> be used for paste operations after that bug is fixed, so it might make sense to
> actually paste text in your mochitest instead of setting the value
> property/attribute.
On the other hand I could just let the assignee of bug 549475 deal with it.

Comment 21

[(no longer active)]

(In reply to comment #20)
> (In reply to comment #18)
> > Firstly you shouldn't really be using newlines inside the reference file,
> > because these are tests about what happens to newlines against the reference. 
> > You may need multiple reference files, though.
> That's just a naming convention. The files could easily be renamed newline.html
> (with the newlines) and newline-ref-?.html (individual references).

Right.  But with the convention that you'd use in your patch, the tests were confusing.

> > Secondly, only one of these tests will ever get executed.  You should convert
> > this to a mochitest which changes the pref and tests the effect of it.
> This bug was originally filed because three of these tests would in fact have
> been executed, but attachment 436522 [details] [diff] [review] makes that moot.

True.

> > Thirdly, this bizarre behavior will be going away in bug 549475, and will only
> > be used for paste operations after that bug is fixed, so it might make sense to
> > actually paste text in your mochitest instead of setting the value
> > property/attribute.
> On the other hand I could just let the assignee of bug 549475 deal with it.

Definitely.  :-)