629978 - [SeaMonkey] mochitest-plain-3: 2 permanent "test_bug629172.html | Textarea should appear correctly after switching the direction ..."

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Serge Gautherie (:sgautherie)]

I don't know whether the cause is in Core/test or SeaMonkey...

Comment 1

[[e-mail]]

So, the reason the test works on Firefox is that the cmd_switchTextDirection key is always enabled, even when the UI is hidden, so one way of fixing this would be to make the command always enabled in SeaMonkey too.

I don't really think synthesizing an Accel+Shift+X event is the ideal way of switching the text direction of a textarea, but I guess you can't call goDoCommand('cmd_switchTextDirection'); from a plain mochitest.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-MC) test_bug629172.html: s/synthesizeKey()/goDoCommand()/g, Add some blank lines

Per comment 1 suggestion: works in both FF40 and SM21.

I believe this is correct, unless one would want to specifically check the keyboard binding.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 508315 [details] [diff] [review]
(Av1-MC) test_bug629172.html: s/synthesizeKey()/goDoCommand()/g, Add some blank lines

PS: I ported this code from
http://mxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/html/tests/test_bug520189.html?force=1

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> So, the reason the test works on Firefox is that the cmd_switchTextDirection
> key is always enabled, even when the UI is hidden, so one way of fixing this
> would be to make the command always enabled in SeaMonkey too.

http://mxr.mozilla.org/comm-central/search?string=cmd_switchTextDirection&case=on
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#3587
http://mxr.mozilla.org/comm-central/search?string=updateEditUIVisibility&case=on
Then we would need to port
Bug 404232 - goUpdateGlobalEditMenuItems caller optimization
http://hg.mozilla.org/mozilla-central/rev/534e26451e0d
and its (in)direct dependencies,
right?

Comment 5

[[e-mail]]

(In reply to comment #4)
> Then we would need to port
> Bug 404232 - goUpdateGlobalEditMenuItems caller optimization
No, we just need to always or never update that particular command. (Never updating it works because we never show disabled UI for it.)

Comment 6

[(no longer active)]

(In reply to comment #1)
> I don't really think synthesizing an Accel+Shift+X event is the ideal way of
> switching the text direction of a textarea, but I guess you can't call
> goDoCommand('cmd_switchTextDirection'); from a plain mochitest.

It is, since it makes sure that the user-accessible way of doing this will continue to work.  Simulating user input using internal APIs such as goDoCommand should only be used as a last resort when writing a unit test.

Comment 7

[(no longer active)]

Comment on attachment 508315 [details] [diff] [review]
(Av1-MC) test_bug629172.html: s/synthesizeKey()/goDoCommand()/g, Add some blank lines

r- on the general approach (see comment 6).

Here is the right fix.  Add a javascript file called something like bug629172.js, defining a function named switchTextDirection, and provide two versions of that file, one for Firefox, and one for SeaMonkey.  The Firefox version of the function should continue to use synthesizeKey.  The SeaMonkey version can use your code in this patch.  Use Makefile.in magics to determine which javascript file to pick up at build time.

In the main test file, all you have to do is to include that file using a <script> tag, and replace the synthesizeKey call with a call to switchTextDirection.

And, by the way, do not add newlines like this, please.  The test is fine as far as whitespace is concerned the way it is.  :-)

Comment 8

[[e-mail]]

(In reply to comment #6)
> goDoCommand should only be used as a last resort when writing a unit test.
I'll file a bug on the test for bug 520189 then ;-)

Comment 9

[[e-mail]]

(In reply to comment #1)
> So, the reason the test works on Firefox is that the cmd_switchTextDirection
> key is always enabled, even when the UI is hidden, so one way of fixing this
> would be to make the command always enabled in SeaMonkey too.
And the reason that's true is that way back in bug 368918 the cmd_switchTextDirection command got moved from browser to toolkit where it was unable to access the gBidiUI variable.

Comment 10

[[e-mail]]

(In reply to comment #7)
> Here is the right fix.
<snip>
Or we could just go with comment #5, much simpler all round, and we'd have to do this if we ever switched to editMenuOverlay.xul anyway.

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to comment #7)
> > Here is the right fix.
> <snip>
> Or we could just go with comment #5, much simpler all round, and we'd have to
> do this if we ever switched to editMenuOverlay.xul anyway.

If you mean enanbling cmd_switchTextDirection by default, I'm all for it.  :-)

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Stop updating cmd_switchTextDirection status [Checked in: Comment 13]

Per your comment 5.
Untested.

Fwiw,
http://mxr.mozilla.org/mozilla-central/search?string=nsSwitchTextDirectionCommand&case=1&find=%2Feditor%2Flibeditor%2Fbase%2F
{
/editor/libeditor/base/nsEditorCommands.cpp
    * line 502 -- nsSwitchTextDirectionCommand::IsCommandEnabled(const char *aCommandName,
}

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 510271 [details] [diff] [review]
(Bv1) Stop updating cmd_switchTextDirection status
[Checked in: Comment 13]

http://hg.mozilla.org/comm-central/rev/058149fc3930

Comment 14

[(no longer active)]

OK, so can we call this fixed now, or is there anything else to be done here?

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> (In reply to comment #6)
> > goDoCommand should only be used as a last resort when writing a unit test.
> I'll file a bug on the test for bug 520189 then ;-)

I filed bug 632326.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> (In reply to comment #4)
> > Then we would need to port
> > Bug 404232 - goUpdateGlobalEditMenuItems caller optimization
> No

Should I file a separate bug to port that bug nonetheless?
Or does SeaMonkey not want that at all?

***

(In reply to comment #9)
> And the reason that's true is that way back in bug 368918 the
> cmd_switchTextDirection command got moved from browser to toolkit where it was
> unable to access the gBidiUI variable.

(In reply to comment #10)
> if we ever switched to editMenuOverlay.xul anyway.

Should I file a separate bug to port that other bug?

Comment 17

[[e-mail]]

(In reply to comment #16)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Then we would need to port
> > > Bug 404232 - goUpdateGlobalEditMenuItems caller optimization
> > No
> Should I file a separate bug to port that bug nonetheless?
I don't think it's sufficiently worthwhile.

> (In reply to comment #10)
> > if we ever switched to editMenuOverlay.xul anyway.
> Should I file a separate bug to port that other bug?
Might as well.

(In reply to comment #5)
> (In reply to comment #4)
> > Then we would need to port
> > Bug 404232 - goUpdateGlobalEditMenuItems caller optimization
> No, we just need to always or never update that particular command. (Never
> updating it works because we never show disabled UI for it.)
While doing the research for this comment, I discovered where our UI for this command lives. So we need to back out the change to utilityOverlay.js so that the UI works when gShowBiDi is set, but not the change to utilityOverlay.xul, so that the test still passes. Sorry about that.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)

> Might as well.

I filed bug 632334.

> we need to back out the change to utilityOverlay.js

http://hg.mozilla.org/comm-central/rev/f4fb75b34428
(Cv1) Revert utilityOverlay.js part of patch Bv1

Comment 19

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1297114634.1297115276.14979.gz
Linux comm-central-trunk debug test mochitests-3/5 on 2011/02/07 13:37:14

V.Fixed, by patch Bv1.

***

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1297170757.1297171673.11949.gz
Linux comm-central-trunk debug test mochitests-3/5 on 2011/02/08 05:12:37

Patch Cv1 didn't regress this bug.