1076158 - [Flame][Video] the option (...) button is not responsive on the first touch

Status: VERIFIED FIXED

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

Description

[No-Jun Park [:njpark]]

STR: 
Open video app, select a video
Pause the video, and touch screen to show overlay
Tap option (...) button

Actual:
the option menu shows up after 3~4 taps

Expected:
The option menu shows up on first tap

Version info:
Gaia-Rev        86a3626055ed58b39525424126870dc0a503e79f
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/c20912812877
Build-ID        20141001000202
Version         34.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141001.093448
FW-Date         Wed Oct  1 09:35:01 EDT 2014
Bootloader      L1TC10011800

This does not reproduce on 319MB 2.0 Flame.

Video link:
https://www.dropbox.com/s/cuks9vwp2ovlm1z/VID_unresponsiveOptionbtn.mp4?dl=0

Comment 1

[No-Jun Park [:njpark]]

[Blocking Requested - why for this release]:
Noticeably bad UI experience

Comment 2

[Sri Kasetti]

Hema, Can you assign someone to analyze this..

Comment 3

[Marcia Knous [:marcia]]

Similar bug I filed: Bug 1075115

Comment 4

[Hema Koka [:hema]]

Blocking Reason: options menu show up on first tap, other wise it will lead to user frustration (looks like regression)

Punam, Can you check whats going on?

Thanks
Hema

Comment 5

[Punam Dahiya [:pdahiya]]

Able to replicate the issue on 2.2 flame kk buildId:20141001040205, sometimes it take 2-3 taps to open options view, will try to increase options button touch are and see if that fixes the issue

Comment 6

[Punam Dahiya [:pdahiya]]

Attachment: PR with fix of bug 1076158

Hi Wilson

As discussed, I am attaching the PR that moves options icon inside gaia-header 10px more to left so that touch area doesn't conflict with edge-gestures. Please review. Thanks

Comment 7

[Punam Dahiya [:pdahiya]]

Setting NI flag for No-Jun to help validate attached patch fixes options icon responsiveness issue. Thanks

Comment 8

[Wilson Page [:wilsonpage]]

Comment on attachment 8499861 [details] [review]
PR with fix of bug 1076158

Looks good to me. Flagging Amy to make sure this doesn't impact any visual stuff.

Comment 9

[No-Jun Park [:njpark]]

Tried out the patch, it is definitely more responsive than before.  I can open the options dialog with at most 2 touches, and it feels more responsive.  Once I locate the exact place to touch, then it opens the dialog consistently.

Comment 10

[Amy Lee [:amylee] UX]

(In reply to Wilson Page [:wilsonpage] from comment #8)
> Comment on attachment 8499861 [details] [review]
> PR with fix of bug 1076158
> 
> Looks good to me. Flagging Amy to make sure this doesn't impact any visual
> stuff.

Can you attach a screenshot? Having issues flashing this patch. Thanks

Comment 11

[Punam Dahiya [:pdahiya]]

Attachment: Video Options Screen Shot

Comment 12

[Amy Lee [:amylee] UX]

Comment on attachment 8499861 [details] [review]
PR with fix of bug 1076158

The options icon now sits 7px more to the left compared to the other apps that also have an options icon in the header (i.e messages app and camera image preview). Do we need to move the icon over? I'd like to keep the position of the options icon consistent across the apps. If we aren't having any responsive issues with the options icon in other apps, we should keep the position of the options icon the same across apps. 

I would also check to make sure the header transparency/colour in the video player is the same as the header in image preview in camera and that the video player background colour is the same as image preview in camera. If the colour changes are out of scope of this bug then I can file a new one.

Thanks!

Comment 13

[Wilson Page [:wilsonpage]]

(In reply to Amy Lee [:amylee] from comment #12)
> Comment on attachment 8499861 [details] [review]
> PR with fix of bug 1076158
> 
> The options icon now sits 7px more to the left compared to the other apps
> that also have an options icon in the header (i.e messages app and camera
> image preview). Do we need to move the icon over? I'd like to keep the
> position of the options icon consistent across the apps. If we aren't having
> any responsive issues with the options icon in other apps, we should keep
> the position of the options icon the same across apps. 

pdahiya: Is this not any issue in other apps? If not we need to identify what is different in Video app.

> I would also check to make sure the header transparency/colour in the video
> player is the same as the header in image preview in camera and that the
> video player background colour is the same as image preview in camera. If
> the colour changes are out of scope of this bug then I can file a new one.

Yes this is out of scope.

Comment 14

[Punam Dahiya [:pdahiya]]

Attachment: PR updating option icon to button

Thanks Amy for ui-review. I agree we should have option icon consistent in all the apps where used in gaia-header. I have attached PR removing 7px shift to left. 

Attached PR has changed anchor tag to button for option icon. I have tested and seeing responsiveness similar to camera preview option icon (using theme-media class for gaia-header).

No-Juan, Can you pl. test the attached PR and confirm if the option icon responsiveness with this patch is acceptable. Thanks

Comment 15

[Punam Dahiya [:pdahiya]]

(In reply to Wilson Page [:wilsonpage] from comment #13)
> (In reply to Amy Lee [:amylee] from comment #12)
> > Comment on attachment 8499861 [details] [review]
> > PR with fix of bug 1076158
> > 
> > The options icon now sits 7px more to the left compared to the other apps
> > that also have an options icon in the header (i.e messages app and camera
> > image preview). Do we need to move the icon over? I'd like to keep the
> > position of the options icon consistent across the apps. If we aren't having
> > any responsive issues with the options icon in other apps, we should keep
> > the position of the options icon the same across apps. 
> 
> pdahiya: Is this not any issue in other apps? If not we need to identify
> what is different in Video app.

wilson: Messaging app is using theme-communications for gaia-header. Camera and Video app are using theme-media, the only difference for Video app was anchor tag for option icon which I have updated in attachment 8501193 [details] [review]. As noted above,  with the patch the responsiveness for video app option icon is similar to camera preview options icon. 


> > I would also check to make sure the header transparency/colour in the video
> > player is the same as the header in image preview in camera and that the
> > video player background colour is the same as image preview in camera. If
> > the colour changes are out of scope of this bug then I can file a new one.
> 
> Yes this is out of scope.

Comment 16

[Wilson Page [:wilsonpage]]

Comment on attachment 8501193 [details] [review]
PR updating option icon to button

Code looks fine. I still understand why this would fix the issue though.

Comment 17

[No-Jun Park [:njpark]]

(In reply to Punam Dahiya from comment #14)
> Created attachment 8501193 [details] [review]
> PR updating option icon to button
> 
> Thanks Amy for ui-review. I agree we should have option icon consistent in
> all the apps where used in gaia-header. I have attached PR removing 7px
> shift to left. 
> 
> Attached PR has changed anchor tag to button for option icon. I have tested
> and seeing responsiveness similar to camera preview option icon (using
> theme-media class for gaia-header).
> 
> No-Juan, Can you pl. test the attached PR and confirm if the option icon
> responsiveness with this patch is acceptable. Thanks

The icon responsiveness here with this patch is better than before, quite acceptable.

Comment 18

[Punam Dahiya [:pdahiya]]

Attachment: Screen Shots video options icon

Attaching screen shot of video app options icon with 8501193 patch. Thanks

Comment 19

[David Flanagan [:djf]]

Punam, do I understand correctly that this new patch just changes the tag name and does not shift the position of the element? If so, then you probably don't even need a UI review for it.  If No-Jun approves, then I'd think this can land without Amy's review.

Thanks for noticing the <a> vs <button> difference between the apps. I wonder if <button> tags get some automatic touch area fluffing that <a> tags do not.  Or if there is some special case in the edge gesture detection code that looks for nearby buttons but not nearby links.

Comment 20

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #19)
> Punam, do I understand correctly that this new patch just changes the tag
> name and does not shift the position of the element? If so, then you
> probably don't even need a UI review for it.  If No-Jun approves, then I'd
> think this can land without Amy's review.
> 
That's correct, with new patch UI stays same. I will cancel ui-review request for this patch.

> Thanks for noticing the <a> vs <button> difference between the apps. I
> wonder if <button> tags get some automatic touch area fluffing that <a> tags
> do not.  Or if there is some special case in the edge gesture detection code
> that looks for nearby buttons but not nearby links.

There's a slight improvement seen in touch with button tag on top half which was missing with anchor tag.

Comment 21

[Punam Dahiya [:pdahiya]]

Patch landed on master
https://github.com/mozilla-b2g/gaia/commit/4e4bc2051096a6719b9775a90b03a00f7a361ea1

Comment 22

[Punam Dahiya [:pdahiya]]

Comment on attachment 8501193 [details] [review]
PR updating option icon to button

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bug 1015248 
[User impact] if declined: Slow responsiveness while touching video options
[Testing completed]: On master
[Risk to taking this patch] (and alternatives if risky): None. One line patch that changes anchor tag to use button tag to help increase options icon responsiveness. Patch also removes a redundant closing header tag.

[String changes made]:None

Comment 23

[Punam Dahiya [:pdahiya]]

Just realized, because of bug 1078536 patch in master attachment 8501193 [details] [review] will not be a straight uplift to 2.1. Here's the link to 2.1 version of the fix. Thanks
https://github.com/mozilla-b2g/gaia/pull/24903

Comment 24

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/cf8cbc192232987958dfc44f298f7ebc00fcc3cc

Comment 25

[Rachel Pribble [:rpribble]]

This issue STILL OCCURS on the Flame v2.1 and v2.2. Same issue as bug 1081562.

Environmental Variables:

Device: Flame 2.1 KK (319 MB)
BuildID: 20141011000201 (full flash)
Gaia: f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1
Gecko: d813d79d3eae
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master KK (319 MB)
BuildID: 20141011040204 (full flash)
Gaia: 95f580a1522ffd0f09302372b78200dab9b6f322
Gecko: 3f6a51950eb5
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

The touch area for the share options in the camera are too low.

Comment 26

[sarsenyev]

Verified the issue is fixed on 2.2 and 2.1 Flame devices
Ellipses button is quickly response on the first touch

Flame 2.2 KK (319mb Full Flash)
Device: Flame 2.2 Master
BuildID: 20141022040201
Gaia: 27a1d1baaa8e375b70e043efee67d5f2206c330b
Gecko: ae4d9b4ff2ee
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Flame 2.1
Device: Flame 2.1 KK (319mb Full Flash)
BuildID: 20141022001201
Gaia: 3d9cc667f4e929861a9a77c41096bbf5a9c1bde0
Gecko: 928b18f7d8ff
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0