956170 - [keyboard refactor] backport new keyboard visuals

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

The new keyboard CSS that landed in 1.3 needs to be copied to the refactored keyboard in progress in test_apps/demo-keyboard.

Comment 1

[David Flanagan [:djf]]

Attachment: link to patch on github

I finally had time to finish a patch! Would you review, please, Jan?

Comment 2

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Demo Keyboard screenshot

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Master keyboard screenshot

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

Hi David, the demo keyboard has some visual differences compared to master keyboard (not taking the suggestion bar into account):

* All chars seem to be bold
* Bottom row seems to be higher
* Text color of special keys (SHIFT, SPACE, etc.) does not match

See screenshots (taken from Keon device).

Comment 5

[David Flanagan [:djf]]

Thanks, Jan.  I was going to leave that fine level of detail to a UX review later, but I'll address the issues you've identified and try to get the screens matching more exactly.

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

Do you want me to run the UI tests against the new keyboard as well, or is that too early? Will require some changes in HTML attributes.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8359077 [details] [review]
link to patch on github

I've updated the pull request to fix the visual nits you identified, and a couple of others as well.  Please review again.

Comment 8

[David Flanagan [:djf]]

(In reply to Jan Jongboom [:janjongboom] from comment #6)
> Do you want me to run the UI tests against the new keyboard as well, or is
> that too early? Will require some changes in HTML attributes.

I don't even know what the UI tests do... What changes will be needed?

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

The UI tests test the actual behavior of the keyboard. But it identifies the keys by CSS selectors. So we would make sure that those keys can still be found.

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Video with key repositioning

I've been trying out, and you can see a weird bug (the background flashing is not specific to this keyboard). It happens in the last switch of this video. You see that the keys are not laid out properly. Any clue?

Comment 11

[David Flanagan [:djf]]

I can see something different flashes on the last transition in the video, but then once you move your hand away I can't tell what is wrong about the layout...

Can you reproduce it again and attach a screenshot? 

Also, are you sure that it is the new keyboard with the bad layout and not the old one? If you typed a character so you weren't at the start of a sentence, the new keyboard would display lowercase and the old one would display upper so we could tell them apart more easily.

Could this be related to bug 959200?

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

Hmm, I don't know, it's like the keys are positioned wrongly and then a reflow happens and it's OK. I think we should just land this patch and see if we can find out what causes it in follow up.

Comment 13

[David Flanagan [:djf]]

Okay.  I'll take that as an implict r+, and will set the flag.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8359077 [details] [review]
link to patch on github

review was by Jan; I'm just setting the r+ flag for him

Comment 15

[David Flanagan [:djf]]

Landed to master: https://github.com/mozilla-b2g/gaia/commit/8e09479b0d2b4865917113eeef7b5e45aa8161fa