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

VERIFIED FIXED in seamonkey2.1b3

Status

SeaMonkey
UI Design
--
major
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Blocks: 1 bug, {rtl})

Trunk
seamonkey2.1b3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [perma-orange], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
I don't know whether the cause is in Core/test or SeaMonkey...
(Assignee)

Updated

7 years ago
blocking-seamonkey2.1: --- → ?
status2.0: --- → ?
(Assignee)

Updated

7 years ago
Summary: [SeaMonkey] mochitest-plain-3: 2 "test_bug629172.html | Textarea should appear correctly after switching the direction ..." → [SeaMonkey] mochitest-plain-3: 2 permanent "test_bug629172.html | Textarea should appear correctly after switching the direction ..."

Comment 1

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Target Milestone: --- → seamonkey2.1b2
(Assignee)

Comment 2

7 years ago
Created attachment 508315 [details] [diff] [review]
(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.
Attachment #508315 - Flags: review?(ehsan)
(Assignee)

Comment 3

7 years ago
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
(Assignee)

Comment 4

7 years ago
(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

7 years ago
(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.)
(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 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.  :-)
Attachment #508315 - Flags: review?(ehsan) → review-

Comment 8

7 years ago
(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

7 years ago
(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.
(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.
(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.  :-)
Target Milestone: seamonkey2.1b2 → ---
(Assignee)

Comment 12

7 years ago
Created attachment 510271 [details] [diff] [review]
(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,
}
Attachment #510271 - Flags: review?(neil)

Updated

7 years ago
Attachment #510271 - Flags: review?(neil) → review+
(Assignee)

Comment 13

7 years ago
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
Attachment #510271 - Attachment description: (Bv1) Stop updating cmd_switchTextDirection status → (Bv1) Stop updating cmd_switchTextDirection status [Checked in: Comment 13]
OK, so can we call this fixed now, or is there anything else to be done here?
(Assignee)

Comment 15

7 years ago
(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.
(Assignee)

Updated

7 years ago
Attachment #508315 - Attachment is obsolete: true
(Assignee)

Comment 16

7 years ago
(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?
Status: ASSIGNED → RESOLVED
blocking-seamonkey2.1: ? → ---
Last Resolved: 7 years ago
status2.0: ? → ---
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
(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.
(Assignee)

Comment 18

7 years ago
(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
(Assignee)

Comment 19

7 years ago
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.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

7 years ago
Keywords: rtl
You need to log in before you can comment on or make changes to this bug.