Kevin Grandon :kgrandon Reporter
	Description • 10 years ago

Possibly related to the recent hardware button dispatching changes, I'm now unable to take screenshots on the flame.

STR:
- Hold power + Volume Down buttons

Expected Result:
A screenshot is taken, and a notification appears.

Actual Result:
No screenshot is taken.

Gaia-Rev        c5bad6d78c5fe168e3bb894fc5cb70902c9b19b1
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/87d4d6d5facc
Build-ID        20141121160202
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  39
FW-Date         Thu Oct 16 18:19:14 CST 2014
Bootloader      L1TC00011880

	Kevin Grandon :kgrandon Reporter
	Comment 1 • 10 years ago

Tzu-Lin - Could you check to see if this is related to bug 1014418?

	Tzu-Lin Huang [:dwi2][:tzhuang] Assignee
	Comment 2 • 10 years ago

I am working on it.
(keep the ni flag for reminding)

	Tzu-Lin Huang [:dwi2][:tzhuang] Assignee
	Comment 3 • 10 years ago

Hi Kevin,

I am able to take screenshot on flame with volume-down + power button, using today's build and latest gaia on master

Gaia-Rev        aad40f6d6eb8f626c6a20db55b9f00d2e832f113
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/8c02f3280d0c
Build-ID        20141123160201
Version         36.0a1

https://www.youtube.com/watch?v=mGA6xfcn-aM

But the time latency of taking screenshot is longer than it is before. Because we are listening to key events (like mozbrowserbeforekeydown/mozbrowserbeforekeyup, ...etc ) from gecko. Gecko needs more time to decide which frame to dispatch these events.

I am wondering what is your STR of unable to take screenshots?

	Tzu-Lin Huang [:dwi2][:tzhuang] Assignee
	Comment 4 • 10 years ago

If timing issue matters, we might need to ask UX's opinion. 

Maybe we have to shorten 'hold interval' of volume-down + power.

	Kevin Grandon :kgrandon Reporter
	Comment 5 • 10 years ago

(In reply to Tzu-Lin Huang [:dwi2][:tzhuang] from comment #3)
> I am wondering what is your STR of unable to take screenshots?

Still able to reproduce on latest Flame KK nightly build. Both volume down and power buttons work individually, but not for taking a screenshot.

I don't think it's a timing issue as eventually when holding the two buttons down the phone will power off instead of taking a screenshot. Trying to debug this...

	Tzu-Lin Huang [:dwi2][:tzhuang] Assignee
	Comment 6 • 10 years ago

May I know your gecko and gaia version and maybe base image version? 

It looks weird to me.

	Kevin Grandon :kgrandon Reporter
	Comment 7 • 10 years ago

Running the check_versions.sh script I see the following details: 

Gaia-Rev        c5bad6d78c5fe168e3bb894fc5cb70902c9b19b1
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/8c02f3280d0c

The gecko build points to Nov 19, which is a bit weird because I *just* ran the nightly OTA update, so maybe the builds are stale? I'm also building gecko myself now to test.

	Gregor Wagner [:gwagner]
	Comment 8 • 10 years ago

Tony, do you think we should add this to our daily smoke tests? I can't file bugs right now because screenshots are not working for me.

	Tony Chung [:tchung]
	Comment 9 • 10 years ago

(In reply to Gregor Wagner [:gwagner] from comment #8)
> Tony, do you think we should add this to our daily smoke tests? I can't file
> bugs right now because screenshots are not working for me.

	No-Jun Park [:njpark]
	Comment 10 • 10 years ago

I can repro the bug with this build:
Gaia-Rev        c5bad6d78c5fe168e3bb894fc5cb70902c9b19b1
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/8c02f3280d0c
Build-ID        20141124040203
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  40
FW-Date         Tue Oct 21 15:59:42 CST 2014
Bootloader      L1TC10011880

I am aware of the time change, but in this case the shot does not happen at all. my STR is:
- reboot,
- on homescreen, take screenshot by power+voldown 

the phone restarts instead of taking screenshots.

	Tony Chung [:tchung]
	Comment 11 • 10 years ago

(In reply to Tony Chung [:tchung] from comment #9)
> (In reply to Gregor Wagner [:gwagner] from comment #8)
> > Tony, do you think we should add this to our daily smoke tests? I can't file
> > bugs right now because screenshots are not working for me.

Yes, i think thats a fair request to add for daily test coverage.   not able to screenshot breaks QA and dev testing which is needed for development cycle.  Peter, please make sure this is included in 2.2 testing.

Lets get a regression window please.

	Peter Bylenga [:PBylenga]
	Comment 12 • 10 years ago

I'll add it in to Smoke Test coverage, for us we can take screenshots if we first depress volume down and then hit power.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 13 • 10 years ago

bug 1031560 changed the hold to 750ms, I think?

I get the same result as PeterB.

	Kevin Grandon :kgrandon Reporter
	Comment 14 • 10 years ago

Hmm, is the bug then that pressing the power button first no longer works? This feels like a regression to me, can we still get a window for this?

	Pi Wei Cheng [:piwei] (inactive)
	Comment 15 • 10 years ago

Confirmed both the bug and workaround method on comment 12. Holding down both Power button and volume down button at the same time will reboot the phone after a couple of seconds; if I press volume down button first then add power button, screenshot can be taken.

2.1 is unaffected. Getting the window now.

	Peter Bylenga [:PBylenga]
	Comment 16 • 10 years ago

Added it to smoketests.  This is failing smoketests, 

https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=100&sortfield=created_on&sortdirection=desc&filter-id=14645

	Pi Wei Cheng [:piwei] (inactive)
	Comment 17 • 10 years ago

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20141107024412
Gaia: 3ca3172b9122fedf46c8411e92c48b724104aae5
Gecko: 45b2fb3afbce
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20141107030312
Gaia: 940d52127168dc694882967ccae37f6e294c9566
Gecko: 60cc0b101ce9
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Gaia & Last Working Gecko - issue DOES repro
Gaia: 940d52127168dc694882967ccae37f6e294c9566
Gecko: 45b2fb3afbce

First Broken Gecko & Last Working Gaia - issue does NOT repro
Gaia: 3ca3172b9122fedf46c8411e92c48b724104aae5
Gecko: 60cc0b101ce9

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/3ca3172b9122fedf46c8411e92c48b724104aae5...940d52127168dc694882967ccae37f6e294c9566

Caused by Bug 1014418.

	Jayme Mercado [:JMercado] (Inactive 3/4/2016)
	Comment 18 • 10 years ago

Broken by Bug 1014418 as suspected in comment 1.  Tzu-Lin what is your recommendation

	Tzu-Lin Huang [:dwi2][:tzhuang] Assignee
	Comment 19 • 10 years ago

Now I could reproduce with volume-down and power key pressed at the same time or power key pressed and held first then volume-down.

It could be caused by bug 1014418. I am looking into it.

	Tzu-Lin Huang [:dwi2][:tzhuang] Assignee
	Comment 20 • 10 years ago

I found the root cause.

TL; DR
System app starts listening to keydown/keyup event after bug 1014418. And after bug 989198, gecko dispatches one more keydown/keyup event to embedder(System app). This extra keydown/keyup event confuses state machine in hardware_buttons.js. We should ignore this extra keydown/keyup event in System app.


The actual key event sequence of the patch in bug 989198 would be:
  1. mozbrowserbeforekeydown event in embedder frame (i.e. System app)
  2. keydown event in embedder frame (*)
  3. keydown event in embedded frame
  4. mozbrowserafterkeydown event in embedder frame

(*): Step 2 is the reason why we cannot take screenshot. Because we process keydown event and translate to volume-down-button-press at step 2, and translate it again at step 4. 

However we must listen to keydown/keyup event in System app, because sometimes the focus is on System app (For example, when user pull down utility tray). I think the solution would be: when System app receives keydown/keyup, we examine its event.target. If it is for iframe, we should ignore this key event.

	Tzu-Lin Huang [:dwi2][:tzhuang] Assignee
	Comment 21 • 10 years ago

Hi Tim, 

Would you help to review the patch? thanks

The rationale of this patch is described at https://bugzilla.mozilla.org/show_bug.cgi?id=1103339#c20 and https://bugzilla.mozilla.org/show_bug.cgi?id=1094066#c1

	Tzu-Lin Huang [:dwi2][:tzhuang] Assignee
	Comment 22 • 10 years ago

rebase to latest master and wait for Gaia-Try result
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=e00d1d5d8158

	Tzu-Lin Huang [:dwi2][:tzhuang] Assignee
	Comment 23 • 10 years ago

landed on master
https://github.com/mozilla-b2g/gaia/commit/7878734c7baf7d0c665c2f838168fce21cdc95b6

	Yeojin Chung [:YeojinC]
	Comment 24 • 10 years ago

This issue is verified fixed on Flame 2.2.

Result: The screenshot is taken when the power button and volume down buttons are pressed at the same time.

Device: Flame 2.2 (319mb, KK, Full Flash)
BuildID: 20141201040205
Gaia: 39214fb22c203e8849aaa1c27b773eeb73212921
Gecko: 08be3008650f
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0