Closed
Bug 669011
Opened 13 years ago
Closed 11 years ago
Scratchpad doesn't allow ctrl-shift-x to switch text direction
Categories
(DevTools :: Source Editor, defect, P3)
DevTools
Source Editor
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: tomer, Unassigned)
References
Details
(Keywords: rtl)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
When Scratchpad starts, it is default to rtl (bug 668960), and it is impossible to switch it back to LTR using ctrl-shift-x key combination.
Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110702 Firefox/6.0a2
Comment 1•13 years ago
|
||
Rob, can we get this fixed, please? The fix should be really simple, this is how this works for Firefox.
<http://mxr.mozilla.org/mozilla-central/search?string=cmd_switchTextDirection>
(We basically need a command and key element -- the rest should work without any glitches...)
Comment 2•13 years ago
|
||
yeah, that looks pretty easy to do. The command's already in the editMenuOverlay.js file.
I'll try to get to this this week. If anyone else is feeling eager, I will accept a patch! :)
Whiteboard: [good-first-bugs]
Comment 3•13 years ago
|
||
I think this'll work, but I can't seem to test it here. I need an RTL string I guess?
anyway, let me know if you need a build to try this out and I can produce one for you on whichever platform you prefer.
Comment 4•13 years ago
|
||
Comment on attachment 547128 [details] [diff] [review]
rtl-switch
No, this won't work, I was talking about a <xul:command> element. :-)
You can test by pressing ctrl+shift+x. That should change the direction of the textbox and everything inside it, even without RTL content in it (try doing this on a textarea on a webpage for example to get an idea of what would happen.)
Attachment #547128 -
Flags: review?(ehsan) → review-
Comment 5•13 years ago
|
||
(In reply to comment #4)
> Comment on attachment 547128 [details] [diff] [review] [review]
> rtl-switch
>
> No, this won't work, I was talking about a <xul:command> element. :-)
Yes, I know. If you look at the scratchpad.xul file, there's an included commandset already. We just didn't have a key binding registered for it.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/scratchpad.xul#60
> You can test by pressing ctrl+shift+x. That should change the direction of
> the textbox and everything inside it, even without RTL content in it (try
> doing this on a textarea on a webpage for example to get an idea of what
> would happen.)
oh yes, I see what you mean right here in this very textarea.
Comment 6•13 years ago
|
||
(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 547128 [details] [diff] [review] [review] [review]
> > rtl-switch
> >
> > No, this won't work, I was talking about a <xul:command> element. :-)
>
> Yes, I know. If you look at the scratchpad.xul file, there's an included
> commandset already. We just didn't have a key binding registered for it.
>
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> scratchpad.xul#60
Ah, right.
OK, so this patch uses bidiSwitchTextDirectionItem.commandKey, which is defined in browser.dtd, which is not included in scratchpad.xul AFAICT (in fact, I get a yellow screen with this patch applied)...
Updated•13 years ago
|
Assignee: rcampbell → nobody
Status: ASSIGNED → NEW
Comment 7•13 years ago
|
||
We're doing developer tool prioritization, filter on 'brontozaur' to ignore the spam.
Priority: -- → P3
Updated•13 years ago
|
Whiteboard: [good-first-bugs] → [good first bug]
Updated•13 years ago
|
Whiteboard: [good first bug] → [good first bug][mentor=robcee]
Comment 8•13 years ago
|
||
I updated the patch, it works, however the line numbers bar is not well aligned.
Attachment #547128 -
Attachment is obsolete: true
Comment 9•13 years ago
|
||
Comment on attachment 618755 [details] [diff] [review]
Updated patch
Thanks for the patch, Andres, and thank you for including a properly formatted commit message and author :)
Attachment #618755 -
Flags: review?(rcampbell)
Comment 10•13 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #9)
> Comment on attachment 618755 [details] [diff] [review]
> Updated patch
>
> Thanks for the patch, Andres, and thank you for including a properly
> formatted commit message and author :)
this works, though it breaks the styling on the source editor. The text overflows into the line gutter.
Mihai, any thoughts on this? Is it an easy fix for the source editor or a lot of work?
Comment 11•13 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> (In reply to Josh Matthews [:jdm] from comment #9)
> > Comment on attachment 618755 [details] [diff] [review]
> > Updated patch
> >
> > Thanks for the patch, Andres, and thank you for including a properly
> > formatted commit message and author :)
>
> this works, though it breaks the styling on the source editor. The text
> overflows into the line gutter.
>
> Mihai, any thoughts on this? Is it an easy fix for the source editor or a
> lot of work?
This is expected. We should not switch the text direction for the entire document that holds Orion because it breaks important assumptions in the code.
This is not an easy fix. We need to open an upstream bug report about switching text direction for the entire document being edited.
Until then, if I am not mistaken, based on their demos, users of RTL languages should be able to input bidi text into Orion as-is.
Comment 12•13 years ago
|
||
Comment on attachment 618755 [details] [diff] [review]
Updated patch
ok, with mihai's comments in mind, I'm going to have to cancel the review on this patch until Orion is capable of being RTLified properly.
The bits of XUL and the command to invoke RTL mode should work when the underpinnings are in place. We'll just have to call whatever API is available in the Source Editor to make that happen.
Attachment #618755 -
Flags: review?(rcampbell)
Comment 13•12 years ago
|
||
I am going to look into this bug, maybe a fix will come with the next Orion update in bug 759351. No guarantees - it might depend on a lot of upstream work.
Depends on: 759351
OS: Linux → All
Hardware: x86 → All
Whiteboard: [good first bug][mentor=robcee] → [sourceeditor][orion]
Comment 14•12 years ago
|
||
Reported upstream:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=381742
Comment 15•12 years ago
|
||
Moving to Source Editor component.
Filter on CHELICERAE.
Component: Developer Tools → Developer Tools: Source Editor
Comment 16•12 years ago
|
||
Tomer / Ehsan: this is a patch which makes Ctrl-Shift-X work in the source editor. This shortcut toggles the text direction between ltr and rtl. Default is always ltr.
Orion seems problematic with rtl, selection doesn't always work as I'd expect from the little experience I have with Arabic.
I'd like someone more experienced to tell me if we want this fix or not? Is this something rtl users would want? In spite of Orion's problems (which are separate issues that we need to fix).
If this is an acceptable fix for the bug at hand, then I will propose the patch to upstream as well.
Thank you!
Assignee: nobody → mihai.sucan
Attachment #618755 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #642924 -
Flags: feedback?(tomer.moz.bugs)
Attachment #642924 -
Flags: feedback?(ehsan)
Comment 17•12 years ago
|
||
Comment on attachment 642924 [details] [diff] [review]
proposed patch
I tried applying this patch (which did not apply cleanly) but neither Cmd+Shift+x nor Ctrl+Shift+x worked for me.
Note that it's probably not a good idea to upstream this as this shortcut is specific to Firefox.
Attachment #642924 -
Flags: feedback?(ehsan)
Comment 18•12 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> Comment on attachment 642924 [details] [diff] [review]
> proposed patch
>
> I tried applying this patch (which did not apply cleanly) but neither
> Cmd+Shift+x nor Ctrl+Shift+x worked for me.
This patch depends on bug 759351.
> Note that it's probably not a good idea to upstream this as this shortcut is
> specific to Firefox.
Good point, but Orion has some Firefox-specific code already. I may as well just keep it in our codebase.
Comment 19•12 years ago
|
||
Could you please create a try server build which we can use to test this?
Comment 20•12 years ago
|
||
Yes. Builds finished.
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-329848768a2a/
Ehsan and Tomer: please download the builds that suit your OS of choice and test ctrl-shift-x in scratchpad. Thank you!
Comment 21•12 years ago
|
||
(In reply to comment #20)
> Yes. Builds finished.
>
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-329848768a2a/
>
> Ehsan and Tomer: please download the builds that suit your OS of choice and
> test ctrl-shift-x in scratchpad. Thank you!
Doesn't work for me on Mac.
Comment 22•12 years ago
|
||
Oops sorry I was using the wrong build.
So the toggle definitely works, but now I doubt how useful this is in practice, given how awkward it is to edit js code in RTL mode. Tomer, what do you think?
Comment 23•12 years ago
|
||
Reported bug upstream about selection problems in RTL text:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=386167
If we get the selection and cursor behavior fixed in Orion, I'd like us to land the ctrl-shift-x shortcut implementation in our codebase.
Comment 25•11 years ago
|
||
Yes. You cannot switch the text direction with ctrl-shift-x in scratchpad with codemirror either.
Assignee: mihai.sucan → nobody
Flags: needinfo?(mihai.sucan)
Updated•11 years ago
|
Attachment #642924 -
Flags: feedback?(tomer.moz.bugs)
Comment 26•11 years ago
|
||
I will look into this if nobody picks it up by next week.
Updated•11 years ago
|
Assignee: nobody → gabriel.luong
Comment 27•11 years ago
|
||
I am not entirely sure if this is still an issue. Currently, the bidi algorithm in CM will detect the language and switch the text input direction automatically. For instance, if I was typing in arabic, the original cursor position will be remembered and the characters will be inserted right to left, and a secondary rtl cursor will appear at the very right of the inserted text. Afterwards, if I switch back to english, it reverts back to left to right automatically. However, this switch from rtl to ltr will insert text from the original cursor position. Ideally, reverting back to ltr should insert text from the position of the secondary cursor.
Codemirror only provides a option for rtlMoveVisually, which switches the cursor movement for the arrow keys. In particular, when enabled for rtl text, pressing the left arrow will visually move the cursor right for rtl text.
Given the fact the text direction switches automatically, do we need a ctrl-shift-x command?
Flags: needinfo?(mihai.sucan)
Comment 28•11 years ago
|
||
Gabriel, as I understand the bug report, ctrl-shift-x should allow the direction switch for the whole input. See how it works in a <textarea>. Let's ask Tomer for retesting with codemirror and he can tell us if this bug is still valid.
Tomer, can you please retest Scratchpad in a Firefox nightly? We would like to know if you think we should add ctrl-shift-x for switching the text direction. Thank you!
Flags: needinfo?(mihai.sucan) → needinfo?(tomer.moz.bugs)
Whiteboard: [sourceeditor][orion]
Updated•11 years ago
|
Summary: [RTL] Scratchpad doesn't allow ctrl-shift-x to switch text direction → Scratchpad doesn't allow ctrl-shift-x to switch text direction
Updated•11 years ago
|
Assignee: gabriel.luong → nobody
Comment 29•11 years ago
|
||
No reply from Tomer. Marking as WONTFIX. Please reopen if needed. Thank you!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(tomer.moz.bugs)
Resolution: --- → WONTFIX
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•