945996 - cannot take screenshots in some devices because no home button

Status: RESOLVED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

* on software home button devices (galaxy nexus, nexus 4), you can't take a screenshot of the lock screen because there's no home button displayed then

* on some tablet devices, we'll likely the same issue

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/14329

Pointer to Github pull-request

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 8342027 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/14329

Makes volume-down+sleep the shortcut for screenshots.

Now always works for softbutton and tablet devices, and matches Android convention.

Upon reflection, should probably be long-press of the combo, and ensure it does *not* fire the sleep/volume events. So just asking feedback about the idea as a whole.

Changing the shortcut would also address bug 940230 (flatfish screenshots).

We'd also need to let the Dev Tools team know so they can update their code.

Comment 4

[Dietrich Ayala (:dietrich)]

Also cannot take a screenshot of notification screen when pulled down, since covers software home button.

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8342027 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/14329

The code looks fine to me, except that I wonder about the change to firing this on release instead of onpress.  See my comments on github about that.  Seems like that makes it harder to get the keystrokes right. But I haven't actually tried the patch out to see how it feels.

Is the screenshot feature documented anywhere? What are the implications of changing it? Does the support team need to be notified? Is this a dev-doc-needed case?  Should we add the new screenshot key combination but keep the old one to ease the transition?

Do we need UX approval for a change like this?

Finally, while you're working on screenshot stuff, I just noticed a message in logcat today complaining about mozGetAsFile at b2g/chrome/content/shell.js:1141.  That needs to change to use the async toBlob() function instead, if you care to tack a gecko change on to this gaia change.

Comment 6

[Dietrich Ayala (:dietrich)]

UX, asking basic feedback on this change. Primarily a developer feature, and the status quo is broken in some circumstances *and* were the outlier.

Comment 7

[Dietrich Ayala (:dietrich)]

This is also just much easier to do with your hand than home+sleep (which is awkward and takes two hands).

Comment 8

[Stephany Wilkes]

It seems like the question here is "Is this patch, which makes volume-down+sleep the shortcut for screenshots, acceptable?" Since it's developer focused, we're fine with this. We don't have much else we'd suggest instead.

If I've misinterpreted something, please let me know.

Comment 9

[Caspy7]

Apologies. In making a change it somehow cleared the needinfo, but oddly the needinfo had no assignee (dunno how that works).
Either way, I'd still like to add a ping to this bug as Flatfish devices are being sent out to testers and this will be extremely helpful for testing and bug reporting.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19606

Updated patch.

Failing two tests in hardware buttons test, investigating.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19606/files

Passing the tests now.

Tim, can you do a review, especially with an eye on the test changes?

While the hardware button tests are passing now, I didn't fully understand a couple of the assertions in the related tests, so not sure "passing" means "correct" ;)

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8429658 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19606/files

You are right, I've left some comment on Github.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 8429658 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19606/files

Ok, there's still an outstanding comment, but I'm not sure there's anything that has to be done about it before landing this, so requesting final review. I'll squash commits and update the commit msg prior to landing.

"We also need to figure out should the user press Volume Down before HOLD_INTERVAL or REPEAT_DELAY?"

I think the answer is yes, but we didn't handle that before if not. Those values are only 50ms difference, which doesn't matter much here.

Also, there are failures on the Travis run, but don't have anything to do with this patch, looks like.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8429658 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19606/files

For the purpose of the review, this bug is r+'d for landing after you squash the commits and ensure there are no test breakage (maybe rebase the branch too?).

Yet after the last review, I realized we can always take the screenshot with software home button or swipe up (if it's enabled), so comment 0 is not really happening (the description is "one CANNOT take a screenshot on these devices). So if we still want to change the combo of taking screenshot after finding that fact, please proceed to land this bug.

Comment 16

[Dietrich Ayala (:dietrich)]

> For the purpose of the review, this bug is r+'d for landing after you squash
> the commits and ensure there are no test breakage (maybe rebase the branch
> too?).

Yes, definitely. I've been rebasing, running the tests locally and checking Travis as I go ;)

> Yet after the last review, I realized we can always take the screenshot with
> software home button or swipe up (if it's enabled), so comment 0 is not
> really happening (the description is "one CANNOT take a screenshot on these
> devices). So if we still want to change the combo of taking screenshot after
> finding that fact, please proceed to land this bug.

Not during FTU and some other fullscreen uses. Also, a bunch of places don't actually work well with software home button. And since no production devices support software home button, those places don't really get fixed.

Also, the current home+sleep is awkward enough. Imagine how awkward swipeup+sleep will be :)

I'll canvas dev-gaia to get some thoughts before landing.

Comment 17

[Aras Balali Moghaddam]

Just want to add my 2 cents here. The software home button does not have any visual feedback right now but IMHO that is a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1018040
If that changes in future then using the slide from edge to take an screenshot -- we have to watch out not to capture the transition.

For what is worth, galexy s4 has an interesting feature which is taking a screenshot by just swiping edge of your hand on the screen. In some strange way, it sort of resembles scanning the screen with your hand. Its simple, easy to remember and also cleans your phone/tablet a little but everytime you do it :P... well that depends actually. Anyways, I thought I mention that here in case an special hand gesture like that could become available in FirefoxOS.

Cant wait for this to land, taking screenshots with app manager is not very convenient.

Comment 18

[Marco Zehe (:MarcoZ)]

Dietrich, did you by chance test this with screen reader enabled? Does the screen reader toggle (volume up, down, up, down, up, down, then announcement, then up, down, up, down, up, down again) still work to turn screen reader on and off, still work?

Comment 19

[Dietrich Ayala (:dietrich)]

(In reply to Marco Zehe (:MarcoZ) from comment #18)
> Dietrich, did you by chance test this with screen reader enabled? Does the
> screen reader toggle (volume up, down, up, down, up, down, then
> announcement, then up, down, up, down, up, down again) still work to turn
> screen reader on and off, still work?

Yep, works still :)

Comment 20

[Dietrich Ayala (:dietrich)]

Green Travis run: https://travis-ci.org/mozilla-b2g/gaia/builds/27703252

Comment 21

[Dietrich Ayala (:dietrich)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20633

Green Travis run in previous comment, just cloned the branch because rebase hell.

Carrying over r+.

Comment 22

[Dietrich Ayala (:dietrich)]

One failure in latest Travis run, but looks unrelated to this change.

https://travis-ci.org/mozilla-b2g/gaia/jobs/27805475

The test is apps/callscreen/test/unit/handled_call_test.js. Confirmed that it passes locally.

Tim, any idea if this failure could be related to this change somehow?

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I have no idea...

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/3b184ab2fa6fc8bca15fa6dd738cd02a8f60074e