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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Makoto Kato [:m_kato]]

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?

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

(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?

Comment 2

[Makoto Kato [:m_kato]]

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.

Comment 3

[Bob Owen (:bobowen)]

(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.

Comment 4

[Andrew Overholt [:overholt]]

(I'm saying "fixlater" != backlog but that should probably have zero bearing on someone working on this :)

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Bug 1257731 - Part 1. Send HaveBidiKeyboards information to content process.

Review commit: https://reviewboard.mozilla.org/r/57548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57548/

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Bug 1257731 - Part 2. Don't use Win32 API on content process.

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 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8759590 [details]
Bug 1257731 - Part 1. Send HaveBidiKeyboards information to content process.

https://reviewboard.mozilla.org/r/57548/#review54650

Excellent!

Comment 9

[Makoto Kato [:m_kato]]

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/

Comment 10

[Makoto Kato [:m_kato]]

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/

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8759591 [details]
Bug 1257731 - Part 2. Don't use Win32 API on content process.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7660b0e9e5dc
https://hg.mozilla.org/mozilla-central/rev/18fb01dd9920