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)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: tomer, Unassigned)

References

Details

(Keywords: rtl)

Attachments

(1 file, 2 obsolete files)

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
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...)
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]
Attached patch rtl-switch (obsolete) (deleted) — Splinter Review
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.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attachment #547128 - Flags: review?(ehsan)
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-
(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.
(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)...
Assignee: rcampbell → nobody
Status: ASSIGNED → NEW
We're doing developer tool prioritization, filter on 'brontozaur' to ignore the spam.
Priority: -- → P3
Whiteboard: [good-first-bugs] → [good first bug]
Whiteboard: [good first bug] → [good first bug][mentor=robcee]
Attached patch Updated patch (obsolete) (deleted) — Splinter Review
I updated the patch, it works, however the line numbers bar is not well aligned.
Attachment #547128 - Attachment is obsolete: true
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)
(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?
(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 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)
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]
Moving to Source Editor component. Filter on CHELICERAE.
Component: Developer Tools → Developer Tools: Source Editor
Attached patch proposed patch (deleted) — Splinter Review
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 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)
(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.
Could you please create a try server build which we can use to test this?
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!
(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.
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?
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.
Is this still an issue?
Flags: needinfo?(mihai.sucan)
Yes. You cannot switch the text direction with ctrl-shift-x in scratchpad with codemirror either.
Assignee: mihai.sucan → nobody
Flags: needinfo?(mihai.sucan)
Attachment #642924 - Flags: feedback?(tomer.moz.bugs)
I will look into this if nobody picks it up by next week.
Assignee: nobody → gabriel.luong
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)
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]
Summary: [RTL] Scratchpad doesn't allow ctrl-shift-x to switch text direction → Scratchpad doesn't allow ctrl-shift-x to switch text direction
Assignee: gabriel.luong → nobody
No longer depends on: 759351
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: