Closed Bug 924893 Opened 6 years ago Closed 6 years ago

[Keyboard] Chrome Event inputmethod-contextchange doesn't have info for the value selector

Categories

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

x86
macOS
defect
Not set

Tracking

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 C4(Nov8)
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: janjongboom, Assigned: janjongboom)

References

Details

(Whiteboard: [3rd-party-keyboard])

Attachments

(2 files)

When investigating bug 906096's failing UI tests I noticed that we indeed break the value selectors. This is because we switch from using mozKeyboard.onfocuschange to a chrome event with the name 'inputmethod-contextchange'. However we discard a lot of information that is currently send. We should make sure that the chrome event has the same info as the old onfocuschange (or change the architecture of value selector).

I have kept the old workaround in bug 906096 with a comment to this bug.
Assignee: nobody → janjongboom
Blocks: 906096
blocking-b2g: --- → koi?
Blocks: 920977
No longer blocks: 906096
Jan, do you need help for the bug or it still managable for now?
blocking-b2g: koi? → koi+
No, I know what to do, just have to make time.
Attached patch Gecko patchSplinter Review
Here's the gecko part
Attachment #817858 - Flags: review?(xyuan)
Attached file Gaia patch
Attachment #817864 - Flags: review?(rlu)
Attachment #817858 - Flags: review?(xyuan) → review+
checkin-needed for Gecko part.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d020cdb22780
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Reopening as the gaia part didn't land yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Jan, please add "[leave open]" to the whiteboard if the bug should not be resolved fixed after the merge to m-c.
Comment on attachment 817864 [details] [review]
Gaia patch

Most changes happen in System, I think it's appropriate to have Tim's review. :)
Attachment #817864 - Flags: review?(rlu) → review?(timdream)
Comment on attachment 817864 [details] [review]
Gaia patch

It's good to have a second pair of eyes, but Rudy should review this.
Attachment #817864 - Flags: review?(timdream)
Attachment #817864 - Flags: review?(rlu)
Attachment #817864 - Flags: feedback+
Target Milestone: --- → 1.2 C4(Nov8)
Whiteboard: [3rd-party-keyboard]
Comment on attachment 817864 [details] [review]
Gaia patch

Looks good to me.
I tested this on device and FF nightly.

Jan, thanks for taking care of this.
:)
Attachment #817864 - Flags: review?(rlu) → review+
Landed in https://github.com/mozilla-b2g/gaia/commit/4fb5a545dc2dc6d89e6f912f787e019a770d907c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Uplifted 4fb5a545dc2dc6d89e6f912f787e019a770d907c to:
v1.2: 47c3ccfc7682d7caeeb8a921153caf8cefc5b480
Depends on: 931351
This broke the select picker. This needs to be backed out.

John & Ryan - Can you backout the Gaia & Gecko patches here?

In other news - why isn't there an automated test preventing the date picker from breaking? Please get an integration test covering this.
Flags: needinfo?(ryanvm)
Flags: needinfo?(jhford)
Depends on: 931962
I backed out the Gaia portion with:

[v1.2 276838e] Revert "Merge pull request #12892 from comoyo/value_selector_inpu
Flags: needinfo?(jhford)
(In reply to John Ford [:jhford] -- please use 'needinfo?' instead of a CC from comment #16)
> I backed out the Gaia portion with:
> 
> [v1.2 276838e] Revert "Merge pull request #12892 from
> comoyo/value_selector_inpu

Can you back this out on master as well?
(In reply to Jason Smith [:jsmith] from comment #17)
> (In reply to John Ford [:jhford] -- please use 'needinfo?' instead of a CC
> from comment #16)
> > I backed out the Gaia portion with:
> > 
> > [v1.2 276838e] Revert "Merge pull request #12892 from
> > comoyo/value_selector_inpu
> 
> Can you back this out on master as well?

[master a8606f7] Revert "Merge pull request #12892 from comoyo/value_selector_in
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(ryanvm)
Fabrice, I guess the value selector breakage Bug 931351 is because the following commit was not uplifted to aurora, i.e. our v1.2 depends on,
https://hg.mozilla.org/mozilla-central/rev/d020cdb22780

Do you think we should not backout this from m-c and do the uplift again?
Flags: needinfo?(fabrice)
Can someone help to uplift this patch to aurora for FxOS v1.2?
https://hg.mozilla.org/mozilla-central/rev/d020cdb22780

Thank you.
Keywords: checkin-needed
(In reply to Rudy Lu [:rudyl] from comment #20)
> Fabrice, I guess the value selector breakage Bug 931351 is because the
> following commit was not uplifted to aurora, i.e. our v1.2 depends on,
> https://hg.mozilla.org/mozilla-central/rev/d020cdb22780
> 
> Do you think we should not backout this from m-c and do the uplift again?

That's fine. We can reland the patch on inbound then. We just need to be careful on landing these patches in the right order - gecko first, then gaia for each branch.
Re-landed on b2g-inbound:
https://hg.mozilla.org/integration/b2g-inbound/rev/8301158fba85

Note to sheriff: no need to backout and reland on m-c, but this needs to be uplifted to aurora.
Flags: needinfo?(fabrice)
https://hg.mozilla.org/mozilla-central/rev/8301158fba85
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Reopen to land Gaia patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Jason Smith [:jsmith] from comment #25)
> Reopen to land Gaia patch.

The Gecko part needs to uplift to aurora before landing Gaia patch everywhere. Any idea how this could happen?
Flags: needinfo?(cbook)
It needs uplifting to B2G26, not Aurora. B2G26 is currently closed due to various perma-fails since the branch was commissioned. I will land it on B2G26 once it is reopened.
Flags: needinfo?(cbook)
Whiteboard: [3rd-party-keyboard] → [3rd-party-keyboard][checkin-needed-b2g26]
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6e5acf11e24c

Please resolve the bug when the Gaia patch hits master and set status-b2g-v1.2 to fixed when uplifted to v1.2.
Whiteboard: [3rd-party-keyboard][checkin-needed-b2g26] → [3rd-party-keyboard]
Rudy, the Gaia patch should be ready for landing right? If so I could do it.
Flags: needinfo?(rlu)
Yes, it is.
Thanks for your help.
Flags: needinfo?(rlu)
master: a8606f77a25fab9497c7130eac6cf7a29a2783ac
v1.2: df049e3177ced0ca493ff0d192c65f18392bb462
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #31)
> master: a8606f77a25fab9497c7130eac6cf7a29a2783ac
> v1.2: df049e3177ced0ca493ff0d192c65f18392bb462

Tim, was something missed in the tests here?  It seems we are getting gaia-ui-test failures on v1.2 as of these commits?

  v1.2:    https://travis-ci.org/mozilla-b2g/gaia/jobs/13300770
Flags: needinfo?(timdream)
(In reply to Ben Kelly [:bkelly] from comment #32)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #31)
> > master: a8606f77a25fab9497c7130eac6cf7a29a2783ac
> > v1.2: df049e3177ced0ca493ff0d192c65f18392bb462
> 
> Tim, was something missed in the tests here?  It seems we are getting
> gaia-ui-test failures on v1.2 as of these commits?
> 
>   v1.2:    https://travis-ci.org/mozilla-b2g/gaia/jobs/13300770

Would it be possible that Travis CI is not running with b2gdesktop containing the Gecko patch?
If you could rule out that we certainly backout the patch and figure out that later.

(it's midnight here so do whatever you see fit here please, thanks)
Flags: needinfo?(timdream)
James, is v1.2 running against mozilla_b2g26_v1_2 branch?  This travis failure may be due to missing a gecko patch.
Flags: needinfo?(jlal)
From bug 929062, it seems this is running against aurora. This should probably be changed to mozilla_b2g26_v1_2.
I will do this today- Rik did most of the work but I will address the review comments myself and land this today.
Flags: needinfo?(jlal)
You need to log in before you can comment on or make changes to this bug.