Closed
Bug 629978
Opened 14 years ago
Closed 14 years ago
[SeaMonkey] mochitest-plain-3: 2 permanent "test_bug629172.html | Textarea should appear correctly after switching the direction ..."
Categories
(SeaMonkey :: UI Design, defect)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
seamonkey2.1b3
People
(Reporter: sgautherie, Assigned: sgautherie)
References
(Blocks 1 open bug, )
Details
(Keywords: rtl, Whiteboard: [perma-orange])
Attachments
(1 file, 1 obsolete file)
1.87 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
I don't know whether the cause is in Core/test or SeaMonkey...
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Updated•14 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•14 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•14 years ago
|
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Target Milestone: --- → seamonkey2.1b2
Assignee | ||
Comment 2•14 years ago
|
||
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•14 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•14 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•14 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.)
Comment 6•14 years ago
|
||
(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•14 years ago
|
||
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•14 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•14 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.
Comment 10•14 years ago
|
||
(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•14 years ago
|
||
(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. :-)
Updated•14 years ago
|
Target Milestone: seamonkey2.1b2 → ---
Assignee | ||
Comment 12•14 years ago
|
||
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•14 years ago
|
Attachment #510271 -
Flags: review?(neil) → review+
Assignee | ||
Comment 13•14 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]
Comment 14•14 years ago
|
||
OK, so can we call this fixed now, or is there anything else to be done here?
Assignee | ||
Comment 15•14 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•14 years ago
|
Attachment #508315 -
Attachment is obsolete: true
Assignee | ||
Comment 16•14 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?
Comment 17•14 years ago
|
||
(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•14 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•14 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
You need to log in
before you can comment on or make changes to this bug.
Description
•