Closed Bug 1257731 Opened 4 years ago Closed 3 years ago

[e10s] Support ctrl+shift shortcut to change direction on editor

Categories

(Core :: Editor, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox48 --- affected
firefox50 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

(Whiteboard: btpp-fixlater)

Attachments

(2 files)

This features are added by bug 98160.

2 issues.
- We should send HaveBidiKeyboards value to child
- Actually, we get key state using Windows API in nsEditorEventListenr on content process.  Does it still work into sandbox?
tracking-e10s: --- → ?
(In reply to Makoto Kato [:m_kato] from comment #0)
> This features are added by bug 98160.
> 
> 2 issues.
> - We should send HaveBidiKeyboards value to child
> - Actually, we get key state using Windows API in nsEditorEventListenr on
> content process.  Does it still work into sandbox?

Bob?
Flags: needinfo?(bobowen.code)
Additional Info.  Yesterday, When I debug this code using 64-bit Nightly, GetKeyboardState returns TRUE, but keystate isn't set.  So this code doesn't work under content sandbox.

I think we should move this to XBL key assign that doesn't use OS API, or handle it on native widget.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> (In reply to Makoto Kato [:m_kato] from comment #0)
> > This features are added by bug 98160.
> > 
> > 2 issues.
> > - We should send HaveBidiKeyboards value to child
> > - Actually, we get key state using Windows API in nsEditorEventListenr on
> > content process.  Does it still work into sandbox?
> 
> Bob?

We have problems with flash using GetKeyState for ActionScript 1 & 2 and it not working in the NPAPI sandbox.
So, it wouldn't surprise me if we had the same problem with GetKeyboardState.
Flags: needinfo?(bobowen.code)
(I'm saying "fixlater" != backlog but that should probably have zero bearing on someone working on this :)
Whiteboard: btpp-fixlater
Assignee: nobody → m_kato
On content sandbox process, GetKeyboardState() API doesn't return current keyboard state.  So we shouldn't use it.

Review commit: https://reviewboard.mozilla.org/r/57550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57550/
Comment on attachment 8759591 [details]
Bug 1257731 - Part 2. Don't use Win32 API on content process.

https://reviewboard.mozilla.org/r/57550/#review54646

I'd like to check the newer patch.

::: editor/libeditor/TypeInState.h:17
(Diff revision 1)
> +// Workaround for windows headers
> +#ifdef SetProp
> +#undef SetProp
> +#endif

Yeah, I also met this when I was writing refactoring code... (We should rename it later for safe since we're building Gecko with unified source...)

::: editor/libeditor/nsEditorEventListener.cpp:507
(Diff revision 1)
>    if (!::GetKeyboardState(keystate)) {
>      return false;
>    }

Well, this make me a little bit confused. Please add comment that ::GetKeyboardState() return true even in the sandbox but keystate isn't set as expected.

::: editor/libeditor/nsEditorEventListener.cpp:520
(Diff revision 1)
> -  if ((keystate[VK_CONTROL] & kKeyDownMask) == 0) {
> +  if (NS_FAILED(aEvent->GetCtrlKey(&modifier)) || !modifier) {
>      return false;
>    }

Could you retrieve WidgetKeyboardEvent* with nsIDOMUIEvent::AsEvent(), nsIDOMEvent::WidgetEventPtr() and WidgetEvent::AsKeyboardEvent() (Using only MOZ_ASSERT for null check of the result is enough)?

Then, you can access each members simpler (without temporary variables).

::: editor/libeditor/nsEditorEventListener.cpp:524
(Diff revision 1)
> -  if (keystate[VK_RSHIFT] & kKeyDownMask) {
> -    keystate[VK_RSHIFT] = 0;
> +  uint32_t location = nsIDOMKeyEvent::DOM_KEY_LOCATION_STANDARD;
> +  aEvent->GetLocation(&location);
> +
> +  if (location == nsIDOMKeyEvent::DOM_KEY_LOCATION_RIGHT) {

Same.

::: editor/libeditor/nsEditorEventListener.cpp:538
(Diff revision 1)
>    }
>  
>    // Scan the key status to find pressed keys. We should abandon changing the
>    // text direction when there are other pressed keys.
> -  // This code is executed only when a user is pressing a control key and a
> -  // right-shift key (or a left-shift key), i.e. we should ignore the status of
> +
> +  if (NS_FAILED(aEvent->GetAltKey(&modifier)) || modifier) {

Same.

::: editor/libeditor/nsEditorEventListener.cpp:541
(Diff revision 1)
> -  keystate[VK_LCONTROL] = 0;
> -  for (int i = 0; i <= VK_PACKET; ++i) {
> -    if (keystate[i] & kKeyDownMask) {
> -      return false;
> +    return false;
> -    }
> +  }
> +  if (NS_FAILED(aEvent->GetMetaKey(&modifier)) || modifier) {

Same. And you should check IsOS() too.
Attachment #8759591 - Flags: review?(masayuki) → review-
Comment on attachment 8759590 [details]
Bug 1257731 - Part 1. Send HaveBidiKeyboards information to content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57548/diff/1-2/
Attachment #8759591 - Flags: review- → review?(masayuki)
Comment on attachment 8759591 [details]
Bug 1257731 - Part 2. Don't use Win32 API on content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57550/diff/1-2/
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7660b0e9e5dc
Part 1. Send HaveBidiKeyboards information to content process. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/18fb01dd9920
Part 2. Don't use Win32 API on content process. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/7660b0e9e5dc
https://hg.mozilla.org/mozilla-central/rev/18fb01dd9920
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.