[Keyboard] Update to use gaia-header

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: yor, Assigned: yor)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

46 bytes, text/x-github-pull-request
rudyl
: review+
Details | Review
No description provided.
Blocks: gaia-header
Assignee: nobody → yor
Status: NEW → ASSIGNED
Posted file Pull request (obsolete) —
Kevin,

Please redirect r? as needed.  Thanks.
Attachment #8429820 - Flags: review?(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!
Attachment #8429820 - Flags: review?(kgrandon) → review?(rlu)
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.
Attachment #8429820 - Flags: review?(rlu)
Attachment #8429820 - Flags: review+
Attachment #8429820 - Flags: feedback?(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.
Attachment #8429820 - Flags: feedback?(kgrandon) → feedback+
Ok, thanks for the info.

Landed to Gaia master to see how it goes,
https://github.com/mozilla-b2g/gaia/commit/0e5d49bcc6e6501b82819dbef82a783f79a68da6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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?
(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?
Rudy, please back this out for now.  We most likely won't land gaia-header in 2.0.

Sorry about the confusion.
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8429820 - Attachment is obsolete: true
Posted file Rebased pull request (obsolete) —
Attachment #8443246 - Flags: review?(wilsonpage)
yor: I'm not sure how to test this. When/how does the keyboard have a header?
Flags: needinfo?(yor)
Ah found it :)

'Settings' > 'Keyboard' > 'Built-In Keyboard'
Flags: needinfo?(yor)
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+.
Attachment #8443246 - Flags: review?(wilsonpage) → feedback+
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.
Attachment #8443246 - Flags: feedback+ → review?(wilsonpage)
Attachment #8443246 - Flags: review?(wilsonpage) → review+
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.
Flags: needinfo?(yor)
Comment on attachment 8443246 [details] [review]
Rebased pull request

Fixed marionette test.

Rudy, please review.
Attachment #8443246 - Flags: review?(rlu)
Flags: needinfo?(yor)
Comment on attachment 8443246 [details] [review]
Rebased pull request

I Cannot find the modified marionette test in the patch.
Wrong link?
Attachment #8443246 - Flags: review?(rlu)
Posted file New PR
Sorry, didn't realize the old PR has been closed.
Attachment #8443246 - Attachment is obsolete: true
Attachment #8472368 - Flags: review?(rlu)
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.
Attachment #8472368 - Flags: review?(rlu) → review+
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.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.