Closed Bug 549094 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Editor, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9.3a4

People

(Reporter: sgautherie, Assigned: ehsan)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files)

Test     : "aaa"
Reference: "aaa bbb"
Blocks: SmTestFail
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.
blocking2.0: --- → ?
(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.
Depends on: 549475
This is the one test which keeps SeaMonkey reftest suite orange:
any solution, even temporary, would be very welcomed...
So is the solution to add editor.singleLine.pasteNewLines to runreftest.py?
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.
Please could you comment on how you would like pref-dependent reftests handled.
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?
(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.)
(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?
(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.
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.
(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...
Attached patch Patch (v1)Splinter Review
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.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #436522 - Flags: review?(bzbarsky)
Attachment #436522 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/rev/89e2b92f0b2a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Thus ensuring that at least something gets tested whatever your pref is.
Attachment #436569 - Flags: review?(bzbarsky)
Comment on attachment 436569 [details] [diff] [review]
Alternatively, add tests for each supported pref

Ehsan's a better reviewer for this.
Attachment #436569 - Flags: review?(bzbarsky) → review?(ehsan)
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.
Attachment #436569 - Flags: review?(ehsan) → review-
V.Fixed per Firefox and SeaMonkey tinderboxes.
Status: RESOLVED → VERIFIED
blocking2.0: ? → ---
Flags: in-testsuite?
(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.
(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.  :-)
No longer depends on: 549475
Blocks: 533285
You need to log in before you can comment on or make changes to this bug.