Bug 85503 – [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Mikhail Pozdnyakov]

figures, letters and printscreen key should be added to keyMap and windowsKeyMap in order to unskip:
fast/events/special-key-events-in-input-text.html
fast/forms/enter-clicks-buttons.html
and also improve input for
fast/events/key-events-in-input-button.html
fast/events/key-events-in-input-text.html

[Mikhail Pozdnyakov]

Created attachment 140027 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

[Gyuyoung Kim]

Comment on attachment 140027 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

View in context: https://bugs.webkit.org/attachment.cgi?id=140027&action=review

> Source/WebCore/ChangeLog:12
> +        No new tests.

Remove above line or write the reason why this patch don't need new test.

[Mikhail Pozdnyakov]

Created attachment 140168 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

fix changelog

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 140027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140027&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests.
> 
> Remove above line or write the reason why this patch don't need new test.
Done.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 140168 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

View in context: https://bugs.webkit.org/attachment.cgi?id=140168&action=review

Are there more commits related to adding keys to this file in your queue?

> LayoutTests/ChangeLog:3
> +        [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling

Nit: '[EFL] foo' instead of '[EFL]foo'

> Source/WebCore/ChangeLog:3
> +        [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling

Ditto.

> Source/WebCore/ChangeLog:9
> +        Corrected valure for printscreen key in the windowsKeyMap.

Nit: s/valure/value/.

> Source/WebCore/ChangeLog:10
> +        Figures and letters keys are added to the keyMap.
> +        Corrected valure for printscreen key in the windowsKeyMap.
> +        Capital letters keys are added to the windowsKeyMap.

This description can be moved to the respective methods they are related to below.

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:97
> +    // Set alphabet to the keyMap.
> +    const char* lows = "abcdefghijklmnopqrstuvwxyz";
> +    const char* caps = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
> +    const int lowsBase = static_cast<int>(*lows);
> +    const int capsBase = static_cast<int>(*caps);
> +
> +    for (unsigned int i = 0; i < 26; i++) {
> +        keyMap().set(String(lows + i, 1), String::format("U+%04X", lowsBase + i));
> +        keyMap().set(String(caps + i, 1), String::format("U+%04X", capsBase + i));
> +    }
> +
> +    // Set digits to the keyMap
> +    const int digitBase = static_cast<int>('0');
> +    for (unsigned int i = 0; i < 10; i++)
> +        keyMap().set(String::number(i), String::format("U+%04X", digitBase + i));

How about

  static void addCharacterToKeyMap(char character)
  {
      keyMap().set(String(&character, 1), String::format("U+%04X", character));
  }

  static void createKeyMap()
  {
      // ...

      for (char c = 'a'; c <= 'z'; c++)
          addCharacterToKeyMap(c);
      for (char c = 'A'; c <= 'Z'; c++)
          addCharacterToKeyMap(c);
      for (char c = '0'; c <= '9'; c++)
          addCharacterToKeyMap(c);

      // ....
  }

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:155
> -    const char* alphabet = "abcdefghijklmnopqrstuvwxyz";
> +    const char* lows = "abcdefghijklmnopqrstuvwxyz";
> +    const char* caps = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
>      for (unsigned int i = 0; i < 26; i++) {

This can be simplified in a way similar to the one I described above.

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:209
> +    if (keyName == "Print")
> +        return String("");

Are you sure this is correct? Neither the Qt nor the GTK+ port does something similar.

[Mikhail Pozdnyakov]

Created attachment 140514 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (From update of attachment 140168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140168&action=review
> 
> Are there more commits related to adding keys to this file in your queue?
> 
> > LayoutTests/ChangeLog:3
> > +        [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling
> 
> Nit: '[EFL] foo' instead of '[EFL]foo'
> 
> > Source/WebCore/ChangeLog:3
> > +        [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling
> 
> Ditto.
> 
> > Source/WebCore/ChangeLog:9
> > +        Corrected valure for printscreen key in the windowsKeyMap.
> 
> Nit: s/valure/value/.
> 
> > Source/WebCore/ChangeLog:10
> > +        Figures and letters keys are added to the keyMap.
> > +        Corrected valure for printscreen key in the windowsKeyMap.
> > +        Capital letters keys are added to the windowsKeyMap.
> 
> This description can be moved to the respective methods they are related to below.
> 
> > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:97
> > +    // Set alphabet to the keyMap.
> > +    const char* lows = "abcdefghijklmnopqrstuvwxyz";
> > +    const char* caps = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
> > +    const int lowsBase = static_cast<int>(*lows);
> > +    const int capsBase = static_cast<int>(*caps);
> > +
> > +    for (unsigned int i = 0; i < 26; i++) {
> > +        keyMap().set(String(lows + i, 1), String::format("U+%04X", lowsBase + i));
> > +        keyMap().set(String(caps + i, 1), String::format("U+%04X", capsBase + i));
> > +    }
> > +
> > +    // Set digits to the keyMap
> > +    const int digitBase = static_cast<int>('0');
> > +    for (unsigned int i = 0; i < 10; i++)
> > +        keyMap().set(String::number(i), String::format("U+%04X", digitBase + i));
> 
> How about
> 
>   static void addCharacterToKeyMap(char character)
>   {
>       keyMap().set(String(&character, 1), String::format("U+%04X", character));
>   }
> 
>   static void createKeyMap()
>   {
>       // ...
> 
>       for (char c = 'a'; c <= 'z'; c++)
>           addCharacterToKeyMap(c);
>       for (char c = 'A'; c <= 'Z'; c++)
>           addCharacterToKeyMap(c);
>       for (char c = '0'; c <= '9'; c++)
>           addCharacterToKeyMap(c);
> 
>       // ....
>   }
> 
> > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:155
> > -    const char* alphabet = "abcdefghijklmnopqrstuvwxyz";
> > +    const char* lows = "abcdefghijklmnopqrstuvwxyz";
> > +    const char* caps = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
> >      for (unsigned int i = 0; i < 26; i++) {
> 
> This can be simplified in a way similar to the one I described above.
> 

Fixed all above.

> > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:209
> > +    if (keyName == "Print")
> > +        return String("");
> 
> Are you sure this is correct? Neither the Qt nor the GTK+ port does something similar.

This function is used to evaluate text() and unmodifiedText() properties for the event, which should be empty in this case.
Actually GTK+ port has something similar as gdk_keyval_to_unicode() returns 0 for printscreen key.

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (From update of attachment 140168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140168&action=review
> 
> Are there more commits related to adding keys to this file in your queue?
There is also bug85479 fixing numeric pad keys related tests.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 140514 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

Looks good, thanks!

[WebKit Review Bot]

Comment on attachment 140514 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

Rejecting attachment 140514 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ng file Source/WebCore/platform/efl/EflKeyboardUtilities.cpp
Hunk #2 FAILED at 84.
Hunk #3 succeeded at 126 (offset 11 lines).
Hunk #4 succeeded at 161 with fuzz 1 (offset 22 lines).
Hunk #5 succeeded at 209 (offset 22 lines).
1 out of 5 hunks FAILED -- saving rejects to file Source/WebCore/platform/efl/EflKeyboardUtilities.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Gustavo No..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12703548

[Gyuyoung Kim]

Rebase please.

[Mikhail Pozdnyakov]

Created attachment 142176 [details]
to be landed.

[WebKit Review Bot]

Comment on attachment 142176 [details]
to be landed.

Clearing flags on attachment: 142176

Committed r117238: <http://trac.webkit.org/changeset/117238>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.