1036796 - [Keyboard] "ABC " and "12&" size need to be the same

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Carol Huang [:Carol]]

Please adjust the sizes of "ABC " and "12&" both to 1.8 rem for all the built-in Keyboards, thanks!

Comment 1

[Trishul]

If you could guide me a bit, I would love to work on this bug.

Comment 2

[Raniere Silva]

Trishul,

at the CSS we have

.keyboard-key.special-key.switch-key > .visual-wrapper > .key-element {
    font-size: 1.8rem;
}

At the default layout this layout is applied to the switch key but at the alt layout it isn't since you haven't ".switch-key". You need to change apps/keyboard/js/keyboard/layout_manager.js to add the necessary class to the key.

Do you need more information?

Comment 3

[Trishul]

Great, that's all I need and will submit the required patch asap...

Comment 4

[Trishul]

Hi, 
Unfortunately I am too occupied to work on this but sudheesh is willing to work on it

Comment 5

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Raniere Silva from comment #2)
> Trishul,
> 
> at the CSS we have
> 
> .keyboard-key.special-key.switch-key > .visual-wrapper > .key-element {
>     font-size: 1.8rem;
> }
> 
> At the default layout this layout is applied to the switch key but at the
> alt layout it isn't since you haven't ".switch-key". You need to change
> apps/keyboard/js/keyboard/layout_manager.js to add the necessary class to
> the key.
Or maybe we should make this rule (font-size: 1.8rem) applied to all special keys, and remove '.switch-key' selector, but that needs a examination on each "special key".

> 
> Do you need more information?

Comment 6

[Sudheesh Singanamalla (:ShellHacker)]

I will be working on this. Thanks Trishul.

Comment 7

[Sudheesh Singanamalla (:ShellHacker)]

L233
} else {
      spaceKeyRow.splice(spaceKeyCount, 0, {
        keyCode: this.KEYCODE_BASIC_LAYOUT,
        value: this.currentLayout.basicLayoutKey || 'ABC',
        ratio: 1.5,
        ariaLabel: 'basicLayoutKey'
      });

Shouldn't adding 
+ className: 'switch-key'
after the ariaLabel: 
fix this? Am I missing something else ? In order to keep ABC and 12& the same size, the only rule thats controlling the size is 'switch-key' ?

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #7)
> L233
> } else {
>       spaceKeyRow.splice(spaceKeyCount, 0, {
>         keyCode: this.KEYCODE_BASIC_LAYOUT,
>         value: this.currentLayout.basicLayoutKey || 'ABC',
>         ratio: 1.5,
>         ariaLabel: 'basicLayoutKey'
>       });
> 
> Shouldn't adding 
> + className: 'switch-key'
> after the ariaLabel: 
> fix this? Am I missing something else ? In order to keep ABC and 12& the
> same size, the only rule thats controlling the size is 'switch-key' ?

Yeah, that should be able to work.

However, I am not sure if we could modify the font size for all the special keys, including "Alt", [Backspace] and [Enter] key.
Let's ask for Carol about this.

Carol, could you help comment on this?
Thanks.

Comment 9

[Abin Abraham]

similar problem faced in localization of firefox OS keyboard

Comment 10

[Sudheesh Singanamalla (:ShellHacker)]

Abin, some keyboards have 4 characters in space of '12&' so I think the l10n:<xx> keyboards need to have special rules and should be tracked as separate bugs each specific to the language to which the problem occurs.

Comment 11

[Carol Huang [:Carol]]

Hi Rudy,
As our offline discussion, 'ABC' would be 1.6 rem ; 'Alt','En(IME Switch)' and '12&' would be 1.8 rem.
[Backspace],[Shift]and [Enter] key will stay the same size as what we have now.

I will update the Visual spec later. Thank you!!

Comment 12

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Carol, thanks for the update.

Hi Sudheesh,

Please be informed of the spec update mentioned in comment 11 and comment on this bug if you have any questions.

Thank you.

Comment 13

[Sudheesh Singanamalla (:ShellHacker)]

(In reply to Rudy Lu [:rudyl] from comment #12)

> Please be informed of the spec update mentioned in comment 11 and comment on
> this bug if you have any questions.

I've noted the spec update. Thank you. The Gaia branch isn't allowing me to access the keyboard app to do the required testing. None of the apps work when I click on them. Waiting for this to be fixed before I can try something at this. Is there another workaround ?

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #13)
> (In reply to Rudy Lu [:rudyl] from comment #12)
> 
> > Please be informed of the spec update mentioned in comment 11 and comment on
> > this bug if you have any questions.
> 
> I've noted the spec update. Thank you. The Gaia branch isn't allowing me to
> access the keyboard app to do the required testing. None of the apps work
> when I click on them. Waiting for this to be fixed before I can try
> something at this. Is there another workaround ?

Yes, it is Bug 1052722, and maybe you could try an older version of Firefox, like Aurora, if want to use Firefox to run Gaia.
Another option would be using simulator in App manager,
https://developer.mozilla.org/zh-TW/Firefox_OS/Running_custom_builds_in_the_App_Manager

Happy hacking and sorry for the inconvenience.

Comment 15

[Sudheesh Singanamalla (:ShellHacker)]

Rudy, Sorry for delaying, i've tried everything possible to set up a custom build for the app manager, didn't seem to work out too well, i'll just wait for Bug 1052722 to be fixed and jump on to fix this. :)
Setting a Need Info for myself.

Comment 16

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Redirect the ni to you based on comment 15. :)

Comment 17

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I'll take this, since we need to wrap up the keyboard related changes in a week or two.
Sudheesh, sorry for stealing the bug from you, and thanks for your help to start looking into this issue.

Comment 18

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V1

This is mainly UI changes, so ask for Carol's help to review.

Tim, could you please help do the code review?
Thanks.

Comment 19

[Sudheesh Singanamalla (:ShellHacker)]

Rudy, Not a problem :) By the way the code looks great :)

Comment 20

[Carol Huang [:Carol]]

Comment on attachment 8474962 [details] [review]
Patch V1

Hi Rudy,
As our offline discussion, UI changes as follow:
'ABC' '12&' and 'Alt' is 1.7 rem ; En(IME Switch)' is 1.8 rem.

And the UI looks good, thank you!!

Comment 21

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/2e0b3bd20c3fdb163ab64c639af38b7b186059a3

Thanks for the code/ui review.

--
Gaia-Try result, https://tbpl.mozilla.org/?rev=118d285dd615925db65a80235b879adb68fb8e50&tree=Gaia-Try