Bug 23156 – Windows: No support for platform keyboard shortcut right shift + right control to set writing direction.

• Component: HTML Editing
• Status: NEW

[Xiaomei Ji]

Chrome bug: http://code.google.com/p/chromium/issues/detail?id=1845

Step to reproduce:
1. press "Compose Mail" in Gmail
2. focus cursor on the text area of the mail's body, press right shift + right ctrl.


Expected result:
Cursor should be moved from the left to the right of the text area.

What happens:
Cursor no movement

Note:
Every textbox/textarea (even with English text or no text at all) should be aligned to the right when pressing the right ctrl + right shift, and aligned to the left when pressing the left ctrl + left shift.

I think this bug is about the Windows convention that Ctrl+Shift changes the base writing direction (which indirectly affects alignment). Am I correct?

Note that WebKit/mac has support for the Mac OS X key bindings for changing the base writing direction (see writingDirectionKeyBindingsEnabled() in WebHTMLView.mm and the methods that consult it).

[Hironori Bono]

Sorry not to have describe its details.
This bug technically consists of two bugs:
1. We need to change the Windows convention of CTRL+SHIFT keys, including adding methods to distinguish left keys and right keys;
2. The WebCore::executeMakeTextDirectionLeftToRight() function (in EditorCommand.cpp) does not change the "dir" attribute of the selected node, as the Editor::setBaseWritingDirection() function does. It only changes its CSS attributes, such as "--webkit-direciton". So, the function causes a serious inconsistency between DOM attributes and CSS properties.

To fix the second bug, we need to:
(a) Change the WebCore::executeMakeTextDirectionLeftToRight() function to call the Editor::setBaseWritingDirection() function to change the writing direction, or;
(b) Add code to the function to change the DOM attributes.

Even though I have confirmed either option work, I would like to ask you which option is better.

Regards,

Hironori Bono

(In reply to comment #2)
> Sorry not to have describe its details.
> This bug technically consists of two bugs:

Then it would be much better to have two separate bugs in Bugzilla about it.

> 1. We need to change the Windows convention of CTRL+SHIFT keys, including
> adding methods to distinguish left keys and right keys;

Okay. Hopefully there is a higher-level way to describe the key combination for changing the writing direction (maybe it is even user-configurable), but this seems like a clear task: recognizing that the user has issued the command to change the writing direction.

> 2. The WebCore::executeMakeTextDirectionLeftToRight() function (in
> EditorCommand.cpp) does not change the "dir" attribute of the selected node, as
> the Editor::setBaseWritingDirection() function does. It only changes its CSS
> attributes, such as "--webkit-direciton". So, the function causes a serious
> inconsistency between DOM attributes and CSS properties.

I think you are mixing up two separate concepts: the "text" or "character-level" directionality and the "paragraph-level" or "base" writing direction.
 
> To fix the second bug, we need to:
> (a) Change the WebCore::executeMakeTextDirectionLeftToRight() function to call
> the Editor::setBaseWritingDirection() function to change the writing direction,

No, setting the text direction of a range should not change the paragraph-level writing direction.

> or;
> (b) Add code to the function to change the DOM attributes.

I do not think that would be necessary. setBaseWritingDirection has the necessary code to achieve what the command does, in a way that as far as I can tell is compatible with IE.

Perhaps some of your confusion stems from the fact that the character-level commands have a modern implementation as editor commands, and the paragraph-level commands have a lot of their logic (such as for validation and state checking) still in WebKit (in WebHTMLView.mm). I think a good first step could be to modernize the way the paragraph-level commands are implemented. I think that will make the differences between the concepts clearer and make addressing issue (1) easier.

[Hironori Bono]

> > (b) Add code to the function to change the DOM attributes.
>
> I do not think that would be necessary. setBaseWritingDirection has the
> necessary code to achieve what the command does, in a way that as far as I can
> tell is compatible with IE.

Sorry for my stupid description.
This "the function" means the WebCore::executeMakeTextDirectionLeftToRight() function. I noticed the Editor::setBaseWritingDirection() has code to change the DOM attributes.

Regards,

Hironori Bono

(In reply to comment #4)
> > > (b) Add code to the function to change the DOM attributes.
> >
> > I do not think that would be necessary. setBaseWritingDirection has the
> > necessary code to achieve what the command does, in a way that as far as I can
> > tell is compatible with IE.
> 
> Sorry for my stupid description.
> This "the function" means the WebCore::executeMakeTextDirectionLeftToRight()
> function. I noticed the Editor::setBaseWritingDirection() has code to change
> the DOM attributes.

executeMakeTextDirectionLeftToRight() implements functionality that, as far as I know, does not exist in other engines, and a choice was made to implement it using the CSS properties 'unicode-bidi' and 'direction'. Why do you feel that executeMakeTextDirectionLeftToRight() needs to use DOM attributes, and how does it relate to this bug?

[Hironori Bono]

> executeMakeTextDirectionLeftToRight() implements functionality that, as far as
> I know, does not exist in other engines, and a choice was made to implement it
> using the CSS properties 'unicode-bidi' and 'direction'. Why do you feel that
> executeMakeTextDirectionLeftToRight() needs to use DOM attributes, and how 
> does it relate to this bug?

Sorry again, I forgot describing it in this thread. (I'm mixed this thread and another internal discussion.)

Maybe we are somewhat misunderstanding WebKit, however, we mapped a Right Ctrl + Right Shift key combination to this executeMakeTextDirectionLeftToRight() function in our EditorClientImpl::handleKeyboardEvent() implementation but it did not change the "dir" attribute and caused the original problem.
So, I assumed this function should change the "dir" attribute as I wrote in my previous response.

If this mapping is wrong, is it possible to give us better mapping functions?
(To read the _changeBaseWritingDirectionTo, it seems to call the Editor::setBaseWritingDirection() function. Nevertheless, I'm not sure I'm correct.)

(In reply to comment #6)
> Sorry again, I forgot describing it in this thread. (I'm mixed this thread and
> another internal discussion.)
> 
> Maybe we are somewhat misunderstanding WebKit, however, we mapped a Right Ctrl
> + Right Shift key combination to this executeMakeTextDirectionLeftToRight()
> function in our EditorClientImpl::handleKeyboardEvent() implementation but it
> did not change the "dir" attribute and caused the original problem.
> So, I assumed this function should change the "dir" attribute as I wrote in my
> previous response.
> 
> If this mapping is wrong, is it possible to give us better mapping functions?
> (To read the _changeBaseWritingDirectionTo, it seems to call the
> Editor::setBaseWritingDirection() function. Nevertheless, I'm not sure I'm
> correct.)

Yes, I think the correct mapping for the Ctrl+Shift (or equivalent) actions is to Editor::setBaseWritingDirection(). That function has specific code to set the 'dir' DOM attribute on text entry elements. It is also the function that implements the Writing Direction submenu of the context menu you get when you right-click in a text entry element or in editable HTML. I think Ctrl+Shift should have the same effect as choosing the appropriate item from the context menu, so it follows that it should call in to the same function.

[Hironori Bono]

> Yes, I think the correct mapping for the Ctrl+Shift (or equivalent) actions is
> to Editor::setBaseWritingDirection(). That function has specific code to set
> the 'dir' DOM attribute on text entry elements. It is also the function that
> implements the Writing Direction submenu of the context menu you get when you
> right-click in a text entry element or in editable HTML. I think Ctrl+Shift
> should have the same effect as choosing the appropriate item from the context
> menu, so it follows that it should call in to the same function.

Thank you for your answers and descriptions.
Your description makes me notice this is a bug of our implementation of the EditorClientImpl class.

Sorry for confusing you.

Regards,

Hironori Bono

[Xiaomei Ji]

Hi Mitz,

Thanks for your help on solving this issue!

After I filed the bug, Jeremy mentioned to me that "From [e-mail]'s responses to some of the bugs we've opened recently it looks like in the cases where Windows & OS X diverge, webkit tries to match OSX's behavior.".

Now I understand what you meant better.

In this case, it is not a Webkit bug. And I am closing it.

Thanks,
Xiaomei

I am closing it.

(In reply to comment #9)
> Hi Mitz,
> 
> Thanks for your help on solving this issue!
> 
> After I filed the bug, Jeremy mentioned to me that "From [e-mail]'s
> responses to some of the bugs we've opened recently it looks like in the cases
> where Windows & OS X diverge, webkit tries to match OSX's behavior.".

That is a good description of the way many things are in WebKit right now, but it is not meant as a guideline. It is not a goal of the WebKit project to introduce Mac OS X-like behaviors on platforms that have different behaviors and conventions. Ideally, WebKit should adopt a behavior that matches the platform, by means of port-specific definitions and code or by means of runtime configuration options.

To put it in terms of some of the bugs you may be referring to, it means that it would be wrong to suggest that the Mac OS X port of WebKit should draw an insertion point with a directionality flag, like Windows does sometimes, but it may not be wrong to suggest that the Windows/Cairo port should draw the insertion point like that; or that there should be a WebCore setting for the kind of insertion point to draw.

> Now I understand what you meant better.
> 
> In this case, it is not a Webkit bug. And I am closing it.

I think this bug has raised at least one valid WebKit issue, which is that the Windows ports need to respond to Ctrl+Shift by changing the writing direction, similarly to how the Mac OS X port changes the writing direction in response to the Mac OS X key combination for the same action. I think this issue should be resolved and I believe the way to resolve it is through a WebKit patch. A bugs.webkit.org bug is a fine way to bring this issue to resolution. You are welcome to either file a new bug or re-open this one, perhaps re-titling it to better reflect the issue.

[Jeremy Moskovich]

reopening...

[Hironori Bono]

Created attachment 28061 [details]
a proposed fix (WebKit part)

Sorry for my slow responses.
Even though this is not a complete fix for this bug (because it is caused by our implementation of the EditorClientImpl class), I needed to add some values to the PlatformKeyboardEvent class so the EditorClientImpl class can distinguish right-modifier keys and left ones.

[Alexey Proskuryakov]

What will be the meaning of m_ctrlKey after this change? Will it be left Ctrl key, or any one? In most cases, we don't care which side the key was at.

We need to expose right/left information to DOM, see this FIXME in KeyboardEvent.cpp:

    , m_keyLocation(key.isKeypad() ? DOM_KEY_LOCATION_NUMPAD : DOM_KEY_LOCATION_STANDARD) // FIXME: differentiate right/left, too

[Hironori Bono]

Thank you for your comments.

(In reply to comment #13)
> What will be the meaning of m_ctrlKey after this change? Will it be left Ctrl
> key, or any one? In most cases, we don't care which side the key was at.

To keep compatibility with the current WebKit, I thought 'm_ctrlKey' should represent any control keys. Should we add comments to clarify this?

> We need to expose right/left information to DOM, see this FIXME in KeyboardEvent.cpp:
> 
>     , m_keyLocation(key.isKeypad() ? DOM_KEY_LOCATION_NUMPAD : DOM_KEY_LOCATION_STANDARD) // FIXME: differentiate right/left, too

Thank you for noticing this.
I left this class unmodified in my previous change, because I'm wondering which value we should set when we pressed a right modifier key and a left modifier key, e.g. a right-shift key and a left-control key. As far as I read the DOM Event spec, it does not write anything about such edge case. (This is also the reason why my change just adds all right-modifier keys to the PlatformKeyboardEvent class.)

(*1) http://www.w3.org/TR/DOM-Level-3-Events/events.html#Events-KeyboardEvent

Even though I prefer setting DOM_KEY_LOCATION_RIGHT when one of right modifier keys are pressed, I'm not so confident of my opinion at all.
Would it be possible to give me your thoughts?

[Alexey Proskuryakov]

(In reply to comment #14)
> To keep compatibility with the current WebKit, I thought 'm_ctrlKey' should
> represent any control keys. Should we add comments to clarify this?

Please do, this wasn't clear from the context.

> I left this class unmodified in my previous change, because I'm wondering which
> value we should set when we pressed a right modifier key and a left modifier
> key, e.g. a right-shift key and a left-control key.

KeyboardEvent::m_keyLocation contains information about the latest key that was pressed, not on modifiers state, so I don't think that pressing several modifier keys at once changes anything.

In practice, it's possible that underlying OS events do not contain all the necessary information (see e.g. a FIXME in KeyEventMac.mm, function isKeyUpEvent()), but we should try to handle common cases.

Perhaps the changes to event classes should be made in a separate bug, that this one would depend upon? They are not directly related to LTR text.

[Darin Adler]

Comment on attachment 28061 [details]
a proposed fix (WebKit part)

Should tbe right keys be treated as entirely separate from the non-right ones? What about the code already in WebCore that calls shiftKey()? If we really are going to have separate booleans like this, then I think it need to be shiftKey(), leftShiftKey(), and rightShiftKey() functions, with shiftKey() returning m_shiftKey | m_rightShiftKey. Or maybe you intend m_shiftKey to be true if either the left or right key is down?

If no platform-independent code is going to be calling these new functions, then I think the Chromium code should match the other platforms and allow you to get to a lower level platform event object. This class is supposed to define a platform-independent interface to keys for use in platform-independent code. If this is an interface defined only for Chromium and used only in Chromium, then I think you should consider doing it differently.

But maybe we do have a cross-platform need to distinguish right and left modifiers. I'm not sure. Given the title of the bug it seems that really this is a Chromium-specific bug about needing to set the shift key boolean even when the right shift key is the one held down, and doesn't require any change to the cross-platform code at all, but I really can't tell.

If we're going to add booleans for the right-key versions of these modifiers, then the patch needs to also modify the implementation on other platforms so that those new booleans are set on those platforms too. It's bad manners to add these for one platform and have them all be false on all the other platforms.

I'm going to say review- for now given these concerns?

[Hironori Bono]

Thank you for your comments.
They make sense, my change is too dependent on Chromium and we should not put such platform-dependent code there.

By the way, I'm wondering if there is a plan of fixing the KeyboardEvent class so that it can expose left/right information in near future.
If there is a plan, we should wait implementing Chromium-dependent code until the KeyboardEvent class is updated to avoid duplicates.

Sorry for my stupid question in advance.

(In reply to comment #16)
> (From update of attachment 28061 [details] [review])
> Should tbe right keys be treated as entirely separate from the non-right ones?
> What about the code already in WebCore that calls shiftKey()? If we really are
> going to have separate booleans like this, then I think it need to be
> shiftKey(), leftShiftKey(), and rightShiftKey() functions, with shiftKey()
> returning m_shiftKey | m_rightShiftKey. Or maybe you intend m_shiftKey to be
> true if either the left or right key is down?
> 
> If no platform-independent code is going to be calling these new functions,
> then I think the Chromium code should match the other platforms and allow you
> to get to a lower level platform event object. This class is supposed to define
> a platform-independent interface to keys for use in platform-independent code.
> If this is an interface defined only for Chromium and used only in Chromium,
> then I think you should consider doing it differently.
> 
> But maybe we do have a cross-platform need to distinguish right and left
> modifiers. I'm not sure. Given the title of the bug it seems that really this
> is a Chromium-specific bug about needing to set the shift key boolean even when
> the right shift key is the one held down, and doesn't require any change to the
> cross-platform code at all, but I really can't tell.
> 
> If we're going to add booleans for the right-key versions of these modifiers,
> then the patch needs to also modify the implementation on other platforms so
> that those new booleans are set on those platforms too. It's bad manners to add
> these for one platform and have them all be false on all the other platforms.
> 
> I'm going to say review- for now given these concerns?
> 

[Jeremy Moskovich]

(Just restating a few things to be clear, apologies if this is obvious)

This issue relates to the Windows platform in general and is not specific to Chromium.
The Windows convention is to use left shift + left control and right shift + right control to toggle writing direction.

In addition, according to comment #13 there is a cross-platform use case for differentiating these.

Darin/Dan: Seeing as this is not Chromium-only, are you OK with adding leftShiftKey()/rightShiftKey() functions?