1094028 - [Video]Video thumbnail Select view is not showing blue outline sometimes or taking longtime after thumbnail selection

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[vsireesha246]

This issue is reproduced in Master and V2.0 as well.

STR:
1.Open video app with more than 15 to 20 videos in device.
2.Click on select button (dotted one)
3.Select top most 2 thumbnails and scroll to bottom upto end thumbnail.
4.Try to select thumbnail at the bottom one.(try to do this STR 3 & 4 repeated to reproduce)

Actual:the thumbnail select count is increased (in top of the header),but the blue color solid "outline" is not shown sometimes.

Expected:The blue outline should show always,otherwise end user it looks like the thumbnail is not selected.

Comment 1

[vsireesha246]

Attachment: Bug_1094028.patch

Hi John,

Would you please review the attached patch and if you are fine with this patch i will upload PR for it.

Thanks..
Sireesha

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8517211 [details] [diff] [review]
Bug_1094028.patch

Review of attachment 8517211 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Russ,

May you help to review this patch? I have my plate really full now. Thanks.

Comment 3

[Russ Nicoletti [:russn]]

Comment on attachment 8517211 [details] [diff] [review]
Bug_1094028.patch

Hi Sireesha,

I can recreate the issue without your patch and it seems your patch solves the issue as I cannot recreate with your patch. However, the css you removed does have a purpose: to avoid the very last thumbnail from getting "stuck" behind the toolbar when in select view. Similar to [1] for the thumbnail list view.

I'm giving r- because the patch would produce a regression in functionality. Nonetheless, I can't offer an explanation as to why the css you removed causes the problems highlighting selected thumbnails.

Punam, do you have any insight as to why the css that is removed as part of this patch would cause the issues described in the STR?

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/video/style/video.css#L191

Comment 4

[vsireesha246]

Hi Russn,

With my patch also the issue can be reproducible.
The root cause is something else,i thought Gallery app don't have this change and working fine.
So i removed the css in video app (which is that patch) and i tried to reproduce the same and it got worked.
But again i tested same issue is occurred.

Is it timing issue,it is taking too much time to show the blue outline sometimes and sometimes it will not show at all.

It occurs easily once we scroll to bottom and try to select the last thumbnail.Somes times after selection only title & size text will blink.

As of know i don't have any clue about this issue.

Thanks..
Sireesha

Comment 5

[Doyun Kim]

[Blocking Requested - why for this release]:

Comment 6

[Punam Dahiya [:pdahiya]]

Hi Russ
On debugging in nightly, I see selected class is getting applied and removed with every tap as expected.
I tried to replicate the issue with today's build (BuildId: 20141107073659) v188 base image Flame and not seeing the bug.

Sireesha, Can you pl. help validate if this issue is replicable in latest build v188 base image. Thanks!

Comment 7

[Punam Dahiya [:pdahiya]]

> Punam, do you have any insight as to why the css that is removed as part of
> this patch would cause the issues described in the STR?
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/video/style/video.
> css#L191

margin-bottom in select view is for last video item to not hide behind footer toolbar. I failed to link how changing the margin-bottom is helping with the fix.

Comment 8

[Rachelle Yang [:ryang][[e-mail]]]

Hi Sireesha, would you please kindly test on Flame 2.0?

Comment 9

[Rachelle Yang [:ryang][[e-mail]]]

Hi Marco, 

could you please kindly find someone to help and provide comments on this per comment 1, 3, 4? 

Thank you very much!

Comment 10

[vsireesha246]

(In reply to Rachelle Yang [:ryang][[e-mail]] from comment #8)
> Hi Sireesha, would you please kindly test on Flame 2.0?

Yes,this is reproduced in Flame latetst V2.0.
This issue is very random and we need to select the last thumbnail always to reproduce this issue and we need to have more than 20+ video in video app.

Thanks..
Sireesha

Comment 11

[Wesly Huang (EPM)]

[Triage] Given the UX impact level and current timing of 2.0 and 2.1, nom. to 2.2 instead (as can repro. in m/c).

Comment 12

[Marco Chen [:mchen]]

(In reply to Rachelle Yang [:ryang][[e-mail]] from comment #9)
> Hi Marco, 
> 
> could you please kindly find someone to help and provide comments on this
> per comment 1, 3, 4? 
> 
> Thank you very much!

Hi Hema,

As I know the video app is what your team can help.
So could you give a hand here?

Thanks.

Comment 13

[Russ Nicoletti [:russn]]

Hema, I took a look at this and wasn't able to figure out what is going on. I can discuss with David later this afternoon during our 1:1.

Comment 14

[Hema Koka [:hema]]

(In reply to Russ Nicoletti [:russn] from comment #13)
> Hema, I took a look at this and wasn't able to figure out what is going on.
> I can discuss with David later this afternoon during our 1:1.

Okay, thanks Russ! 

There were similar issues on camera with the blue highlighting on tap and it was due to css active state inconsistencies (platform issue). Check with Justin too since he dealt with similar issue.

Thanks
Hema

Comment 15

[Russ Nicoletti [:russn]]

Attachment: video showing STR

After discussing with David, I tested again with APZ off and was not able to recreate. I can only recreate with APZ on. One other discovery: scrolling the thumbnails "fixes" the highlighting. See attached video.

I added 'will-change: scroll-position' to the #thumbnails section in video.css but that did not have any effect.

Comment 16

[Russ Nicoletti [:russn]]

Based on comment 15, my assumption is this issue is related to APZ. Vivien, I believe you were involved with APZ at some point (not sure how recently). Can you comment and/or reassign as appropriate? Thanks.

Comment 17

[vsireesha246]

I can reproduce this issue in V2.0 after APZ OFF case as well.

Comment 18

[David Flanagan [:djf]]

Setting needinfo for Vivien based on comments 15 and 16.

Comment 19

[David Flanagan [:djf]]

Changing needinfo to Fabrice, since I've heard that Vivien is OOO. Fabrice: do you know about apz, or could you direct this bug to someone who does?

Comment 20

[sasikala]

Attachment: Bug_1094028_Latest.patch

Hi Russn,

We have done the changes for this issue(v2.0) by adding the overflow: hidden for thumbnail inner element. Added this because there is internal scroll happening for thumbnail selected element due to which the css is not updating.

Kindly review the changes.

Thanks!

Comment 21

[[:fabrice] Fabrice Desré]

(In reply to David Flanagan [:djf] from comment #19)
> Changing needinfo to Fabrice, since I've heard that Vivien is OOO. Fabrice:
> do you know about apz, or could you direct this bug to someone who does?

Kats, help!

Comment 22

[Russ Nicoletti [:russn]]

Comment on attachment 8522047 [details] [diff] [review]
Bug_1094028_Latest.patch

Hi, thanks for the patch. Unfortunately, I am able to recreate the problem with the patch applied.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

So far I'm unable to reproduce this locally. However based on the video and comment 17 I'm inclined to say this is not directly related to APZ. It seems like the touch events are going through just fine and the state changes take effect, but layout doesn't invalidate the right things so it doesn't repaint right away.

Comment 24

[David Flanagan [:djf]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> So far I'm unable to reproduce this locally. However based on the video and
> comment 17 I'm inclined to say this is not directly related to APZ. It seems
> like the touch events are going through just fine and the state changes take
> effect, but layout doesn't invalidate the right things so it doesn't repaint
> right away.

So if this isn't an APZ bug, are you saying it is a layout bug? What product and component should it be recategorized as?

Comment 25

[David Flanagan [:djf]]

(In reply to Russ Nicoletti [:russn] from comment #22)
> Comment on attachment 8522047 [details] [diff] [review]
> Bug_1094028_Latest.patch
> 
> Hi, thanks for the patch. Unfortunately, I am able to recreate the problem
> with the patch applied.

Russ: did you try the patch on 2.0? It sounds like something different may be happening in that version, since, according to comment #17, the bug reproduces even with APZ off. So if a CSS change can fix this in 2.0 that might be worth landing in the 2.0 tree.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Flanagan [:djf] from comment #24)
> So if this isn't an APZ bug, are you saying it is a layout bug? What product
> and component should it be recategorized as?

That's what it looks like to me, yes. Core::Layout would be where I would put it.

Comment 27

[David Flanagan [:djf]]

Russ: we've pushed this bug off to core:layout, but please keep it in mind, especially during your RTL work. If we get really lucky, maybe you'll be able to find some CSS simplifications for RTL that will also work around whatever the underlying issue is here.

Also see comment 25 where I meant to needinfo you originally.

Comment 28

[Jet Villegas (inactive)]

(In reply to David Flanagan [:djf] from comment #27)
> Russ: we've pushed this bug off to core:layout

OK, we'll need more info please. Regression window, if possible.

Comment 29

[Russ Nicoletti [:russn]]

(In reply to David Flanagan [:djf] from comment #25)
> (In reply to Russ Nicoletti [:russn] from comment #22)
> > Comment on attachment 8522047 [details] [diff] [review]
> > Bug_1094028_Latest.patch
> > 
> > Hi, thanks for the patch. Unfortunately, I am able to recreate the problem
> > with the patch applied.
> 
> Russ: did you try the patch on 2.0? It sounds like something different may
> be happening in that version, since, according to comment #17, the bug
> reproduces even with APZ off. So if a CSS change can fix this in 2.0 that
> might be worth landing in the 2.0 tree.

I did try the patch on 2.0. I was not able to recreate with the patch. However, Sireesha says it is reproducible with the patch (comment 4). From my testing, it is easily reproducible in 2.0 without the patch and hard to reproduce with so it would seem the css removed by the patch is significant somehow. Regarding the behavior in master, I was not able to reproduce without the patch. I have not compared the css between 2.0 and master, but I will do that given the difference in behavior between 2.0 and master.

Comment 30

[vsireesha246]

HI Russ,

Can you please try the patch attached in https://bugzilla.mozilla.org/show_bug.cgi?id=1094028#c20 and this patch is different from the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1094028#c4.

So would you please recheck comment20 patch by Sasikala in V2.0.

Thanks..
Sireesha

Comment 31

[Russ Nicoletti [:russn]]

I'm finding it's unpredictable to recreate this issue. With my latest testing with v2.0, I'm not able to reproduce. I performed a full flash of v2.0. Regarding the patch of comment 20, with my testing a couple of weeks ago, I was able to reproduce with that patch (see comment 22) so as far as I can tell, it's not a solution. 

No Jun, can you try to reproduce this issue with v2.0?

Sireesha, can you provide the device/build details with which you are testing and are able to reproduce the issue?

Comment 32

[vsireesha246]

V2.0
Build Identifier:20141118134312
Platform version:32.0

I just flash V2.0 latest gaia and did make.After that i took nearly 30+ videos from Camera(No Sdcard) and i tried to reproduced and after doing the STR 3 to 4 times i can recreate the issue.

Thanks..
Sireesha

Comment 33

[No-Jun Park [:njpark]]

I can readily repro this issue in 2.0 video app.  (not reproducible in gallery or any other app)
The version I used is:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ff1100ba2ab8
Build-ID        20141204000228
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141204.040425
FW-Date         Thu Dec  4 04:04:36 EST 2014
Bootloader      L1TC10011880

Comment 34

[Larissa Shapiro]

This is on our 2.2  blocker list and confirmed by No-Jun but has no progress in 2 months...anyone?

Comment 35

[Jet Villegas (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> So far I'm unable to reproduce this locally. However based on the video and
> comment 17 I'm inclined to say this is not directly related to APZ. It seems
> like the touch events are going through just fine and the state changes take
> effect, but layout doesn't invalidate the right things so it doesn't repaint
> right away.

Paint-Flashing indicates correct damaged area invalidation. We can pick this up as a Layout bug but still need a regression window.

Comment 36

[No-Jun Park [:njpark]]

qawanted for regression window

Comment 37

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to No-Jun Park [:njpark] from comment #36)
> qawanted for regression window

I was unable to reproduce this issue on Flame 2.0, Flame 2.1, Flame 2.2, and Flame 3.0 master.

Following STR, video thumbnails can be selected/highlighted without issue. Tried with 31 videos taken by Flame, attempted ~20 times on each branch. APZ is enabled by default in developer settings. I have tried with 20 videos, 30 videos, memory setting set to 512MB, shallow flashing/full flashing, using an old 2.0 build... all couldn't repro the bug. I'm guessing it could be fixed by the new base, or I'm missing something important in reproducing the issue.

Device: Flame 2.0 (319MB mem, full flash)
BuildID: 20150210000201
Gaia: 2989f2b2bd12fcc0e9c017d2db766e76a55873b8
Gecko: 2ca6a9e7cb81
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 32.0 (2.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.1 (319MB mem, full flash)
BuildID: 20150209001212
Gaia: 4a14bb118d55f3d15293c2ff55b7f29f9b0bfcdb
Gecko: 6cbe28d0bb8c
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 (319MB full flash)
BuildID: 20150210002516
Gaia: b30c8e4303595a0fcb5b640d673cf8503b954701
Gecko: 3e9fa4e70a1b
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 3.0 Master (319 full flash)
BuildID: 20150210010523
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0 Master) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

----

Leaving qawanted tag for others to attempt.

Comment 38

[Brogan Zumwalt [Inactive]]

Unable to reproduce issue on Flame 2.2 (V18D-1 AND V188-1) or Flame 3.0

Tested using full flashed builds using 319mb of memory with 100 videos on device. 15+ attempts on each build. All videos can be highlighted and de-highlighted without any graphical issues appearing when following STR from comment 0 and repro video.

Attempted on 2.2 from date where bug was reported using both current and previous base image. Also tested on today's 3.0 using current base image.

Device: Flame 2.2 (319mb full flash)
Build ID: 20141104040207
Gaia: 3c50520982560ccba301474d1ac43706138fc851
Gecko: 54d05732f29b
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.2 (319mb full flash)
Build ID: 20141104040207
Gaia: 3c50520982560ccba301474d1ac43706138fc851
Gecko: 54d05732f29b
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 3.0 Master (319mb full flash)
BuildID: 20150210010523
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0 Master) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Leaving qawanted tag in case anyone else can reproduce.

Comment 39

[Jet Villegas (inactive)]

(In reply to Pi Wei Cheng [:piwei] from comment #37)
> (In reply to No-Jun Park [:njpark] from comment #36)
> > qawanted for regression window
> 
> I was unable to reproduce this issue on Flame 2.0, Flame 2.1, Flame 2.2, and
> Flame 3.0 master.

I can definitely repro on Flame 2.1 with default settings. Turning off "Low-precision painting" makes the bug go away. May have been fixed with changes related to that code.

Comment 40

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

I cannot reproduce the bug as written in comment 0 on any build.  Using Flame 2.0 and Flame 2.2 I am able to reproduce a similar bug that prevents touch events from being recognized for 2 taps after scrolling, however, this is not the bug from comment 0 and the video.   For Flame 3.0 and Flame 2.1 I saw no issues with selection after scrolling and selecting for around 10 minutes on each build.  

All builds were pulled from the Tinderbox site and I tested builds from 12-4-2014 (per No-Jun's comment 33) as well as the most recent available to me for each branch (Flame 3.0 did not exist back in December).  I believe that this issue requires some difference in setup that is not mentioned in the comments before now beyond what is installed on the phone with 20+ videos.  I used 60 videos (10 taken with the flame camera and 50 from an x-heavy reference workload) when attempting this reproduction.  Any more information that you can provide in order to get a reproduction would be appreciated.



These builds reproduced the similar issue listed above (After scrolling selection takes 3 presses before it will select something) but not the issue seen in the video.

Environmental Variables:
Device: Flame 2.0
BuildID: 20141204132652
Gaia: 856863962362030174bae4e03d59c3ebbc182473
Gecko: 6bcc1952b543
Version: 32.0 (2.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Environmental Variables:
Device: Flame 2.0
BuildID: 20150210080507
Gaia: 2989f2b2bd12fcc0e9c017d2db766e76a55873b8
Gecko: 0a83232a0324
Version: 32.0 (2.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20141204080450
Gaia: 0462090a99093049add9268d14cbc7e44c1d1ccb
Gecko: 29d086b32a26
Version: 37.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150210132356
Gaia: b6ca0aed54fb098f8c2ca2711a917ac351fc7380
Gecko: 0f4ebef7aeb1
Version: 37.0a2 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0



Could NOT reproduce any issue:

Environmental Variables:
Device: Flame 2.1
BuildID: 20141204132549
Gaia: 473113cbdfb6ebe03dda3c0223bec3e138df9ff4
Gecko: 55b024bb551d
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Environmental Variables:
Device: Flame 2.1
BuildID: 20150210080726
Gaia: 4540eed68c929d39c596c24c2af2ed0413774f7c
Gecko: ad3a9f694f0d
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Environmental Variables:
Device: Flame 3.0
BuildID: 20150209133034
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Comment 41

[Jet Villegas (inactive)]

I can't reproduce this on trunk either. I'm going to mark it RESOLVED:WFM as we've got enough people reporting so. See my note in comment 39 if you come across a repro case.

Thanks everyone for the testing!