1015295 - [Keyboard] Update to use gaia-header

Status: RESOLVED FIXED

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

Comment 1

[Yan Or]

Attachment: Pull request

Kevin,

Please redirect r? as needed.  Thanks.

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8429820 [details] [review]
Pull request

Hey, you should probably use the modules page (https://wiki.mozilla.org/Modules/FirefoxOS) to find the right reviewer, or use the history in github of the repo. In this case, I think it would make sense for Rudy to review this perhaps.

Rudy - could you please take a look at this? If you don't have time, feel free to delegate to me. Thanks!

Comment 3

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

Comment on attachment 8429820 [details] [review]
Pull request

I tried to find another app that is utilizing the web component for header but found nothing, so please help me to review this with the following questions,

1. Is the whole web component thing production level?

2. Will this work when running Gaia with Firefox browser Nightly?
   From my tests, it seems not working.

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8429820 [details] [review]
Pull request

(In reply to Rudy Lu [:rudyl] from comment #3)
> Comment on attachment 8429820 [details] [review]
> Pull request
> 
> I tried to find another app that is utilizing the web component for header
> but found nothing, so please help me to review this with the following
> questions,

There are a bunch of open pull requests, and they are currently trying to land code. Whether or not this lands in 2.0 is yet to be seen.

> 1. Is the whole web component thing production level?

Basic usage is. There are a few platform bugs still, but we've found workarounds for most of them.

> 2. Will this work when running Gaia with Firefox browser Nightly?
>    From my tests, it seems not working.

It should work! You will need to set the 'dom.webcomponents.enabled' flag to true.

Comment 5

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

Ok, thanks for the info.

Landed to Gaia master to see how it goes,
https://github.com/mozilla-b2g/gaia/commit/0e5d49bcc6e6501b82819dbef82a783f79a68da6

Comment 6

[Yan Or]

Wasn't expecting this to land.  Need to change the attribute names per:

https://bugzilla.mozilla.org/show_bug.cgi?id=1011807#c14

Should I reopen this ticket and update the patch or create a new patch?

Comment 7

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

(In reply to Yan Or from comment #6)
> Wasn't expecting this to land.  Need to change the attribute names per:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1011807#c14
> 
> Should I reopen this ticket and update the patch or create a new patch?

Oops, it's my fault to land this without asking for your opinion again.
With bug 1020908, do you think we should back this out first and have a new/correct patch?

Comment 8

[Yan Or]

Rudy, please back this out for now.  We most likely won't land gaia-header in 2.0.

Sorry about the confusion.

Comment 9

[Kevin Grandon :kgrandon]

Reverted: https://github.com/mozilla-b2g/gaia/commit/073f92c8c204c333170cc81dd05a20c9cdd6b57e

We should probably stop asking for reviews if we're not ready to land. Instead you can ask for feedback?

Comment 10

[Yan Or]

Attachment: Rebased pull request

Comment 11

[Wilson Page [:wilsonpage]]

yor: I'm not sure how to test this. When/how does the keyboard have a header?

Comment 12

[Wilson Page [:wilsonpage]]

Ah found it :)

'Settings' > 'Keyboard' > 'Built-In Keyboard'

Comment 13

[Wilson Page [:wilsonpage]]

Comment on attachment 8443246 [details] [review]
Rebased pull request

Looks fine to me. Don't want to land until we have decided where gaia-header is going to live (gaia or gaia-components). That may require some tweaks to this patch.

This patch will need some tweaks when new gaia-header lands (with centered logic). Therefore converting to f+.

Comment 14

[Yan Or]

Comment on attachment 8443246 [details] [review]
Rebased pull request

Rebased off latest master.

Wilson, please take a quick look at this and land it if ok.

Comment 15

[Wilson Page [:wilsonpage]]

Comment on attachment 8443246 [details] [review]
Rebased pull request

Landed on 'master' https://github.com/mozilla-b2g/gaia/commit/a7f25f36897b98dbb11c37f55cc1bbe57fcd326d

Comment 16

[Kevin Grandon :kgrandon]

Reverted for being highly likely of causing a Gij perma-fail:

1) manipulate keyboard settings Launch built-in keyboard's settings page:
00:28:39     INFO -    NoSuchElement: (7) Unable to locate element: #back
00:28:39     INFO -    Remote Stack:
00:28:39     INFO -    <none>
00:28:39     INFO -        at Error.MarionetteError (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/error.js:67:13)
00:28:39     INFO -        at Object.Client._handleCallback (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:476:19)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:510:21
00:28:39     INFO -        at TcpSync.send (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/drivers/tcp-sync.js:153:10)
00:28:39     INFO -        at Object.send (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:457:36)
00:28:39     INFO -        at Object.Client._sendCommand (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:503:19)
00:28:39     INFO -        at Object._findElement (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:1303:19)
00:28:39     INFO -        at Object.findElement (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:1352:32)
00:28:39     INFO -        at Object.MarionetteHelper.waitForElement (/builds/slave/test/gaia/node_modules/marionette-helper/index.js:139:19)
00:28:39     INFO -        at Object.Base.waitForElement (/builds/slave/test/gaia/apps/settings/test/marionette/app/base.js:56:31)
00:28:39     INFO -        at Object.KeyboardApp.goBackToSettingsApp (/builds/slave/test/gaia/apps/settings/test/marionette/app/keyboard_app.js:71:27)
00:28:39     INFO -        at Context.<anonymous> (/builds/slave/test/gaia/apps/settings/test/marionette/tests/keyboard_settings_test.js:24:17)
00:28:39     INFO -        at callFn (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:223:21)
00:28:39     INFO -        at Test.Runnable.run (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:216:7)
00:28:39     INFO -        at Runner.runTest (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:373:10)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:451:12
00:28:39     INFO -        at next (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:298:14)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:308:7
00:28:39     INFO -        at next (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:246:23)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:270:7
00:28:39     INFO -        at done (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:185:5)
00:28:39     INFO -        at callFn (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:228:7)
00:28:39     INFO -        at Hook.Runnable.run (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:216:7)
00:28:39     INFO -        at next (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:258:10)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:270:7
00:28:39     INFO -        at done (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:185:5)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:199:9
00:28:39     INFO -        at Object.executeHook (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:369:18)
00:28:39     INFO -        at process._tickCallback (node.js:415:13)

https://tbpl.mozilla.org/?rev=aa249d683122bbbf4371d5902a770beac451dd01&tree=Gaia-Try

You probably want to get this patch reviewed by a keyboard peer before re-landing.

Comment 17

[Yan Or]

Comment on attachment 8443246 [details] [review]
Rebased pull request

Fixed marionette test.

Rudy, please review.

Comment 18

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

Comment on attachment 8443246 [details] [review]
Rebased pull request

I Cannot find the modified marionette test in the patch.
Wrong link?

Comment 19

[Yan Or]

Attachment: New PR

Sorry, didn't realize the old PR has been closed.

Comment 20

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

Comment on attachment 8472368 [details] [review]
New PR

It's ok.

This looks good to me, but I have 2 questions,
 1. The title "Keyboard Settings" would not be centered, and this is a known issue, do you think this could be addressed by the gaia-header component?

 2. The color the title is slightly changed, could we customize it to make it the same as the original color?

Thanks.

Comment 21

[Yan Or]

Rudy,

1. Yes, I've raised a separate issue about some header titles not centering.  This should be fixed in gaia-header.

2. We are doing a visual refresh of all the apps and this is the standardized color for settings related screens.

Thanks.

Comment 22

[Wilson Page [:wilsonpage]]

Comment on attachment 8472368 [details] [review]
New PR

LANDED (master) https://github.com/mozilla-b2g/gaia/commit/e57ea93196e997336934d6e47323755c5bdd80e9