Remove legacy transition code from the callscreen

RESOLVED FIXED in Firefox OS v2.2

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: drs, Assigned: thills)

Tracking

unspecified
2.1 S9 (21Nov)

Firefox Tracking Flags

(b2g-v2.2 fixed)

Details

(Whiteboard: [planned-sprint c=1])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
We have some legacy transition code in the callscreen from after bug 927862 landed, such as this:
https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L301

We should remove all references to this as the transitions are now handled in the AttentionWindow code in the System app.
(Reporter)

Updated

4 years ago
Assignee: nobody → thills
(Reporter)

Updated

4 years ago
Whiteboard: [planned-sprint] → [planned-sprint][in-sprint=v2.1-S5]
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
(Reporter)

Updated

4 years ago
Whiteboard: [planned-sprint][in-sprint=v2.1-S5] → [planned-sprint c=3][in-sprint=v2.1-S5]
(Assignee)

Comment 1

4 years ago
Created attachment 8503342 [details] [diff] [review]
1068093.diff

Hi Doug,

Looking for feedback.  I removed what Etienne suggested, plus there was some more that became useless after those.

Right now the tests are just "working", but I realize I will need to add some more to replace the ones I removed to account for this change.
Attachment #8503342 - Flags: feedback?(drs+bugzilla)
(Reporter)

Comment 2

4 years ago
Comment on attachment 8503342 [details] [diff] [review]
1068093.diff

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

Looks reasonable. This can go into review whenever you're ready. I didn't check if you missed anything, but the general approach is good.
Attachment #8503342 - Flags: feedback?(drs+bugzilla) → feedback+
(Reporter)

Comment 3

4 years ago
Let's track the in-sprint flag using the meta bug instead to avoid polluting the whiteboard of every sub-bug.
Whiteboard: [planned-sprint c=3][in-sprint=v2.1-S5] → [planned-sprint c=3]
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (Oct24)
(Reporter)

Updated

4 years ago
Whiteboard: [planned-sprint c=3] → [planned-sprint c=1]
(Assignee)

Comment 4

4 years ago
Created attachment 8504938 [details] [diff] [review]
Patch for review
Attachment #8504938 - Flags: review?(drs.bugzilla)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 5

4 years ago
Created attachment 8505081 [details] [diff] [review]
Remove legacy transition code from the callscreen (author: thills)

Posted as a patch since I have no way to comment on this without either a PR or patch posted.
Attachment #8504938 - Attachment is obsolete: true
Attachment #8504938 - Flags: review?(drs+bugzilla)
Attachment #8505081 - Flags: review?(drs+bugzilla)
(Reporter)

Comment 6

4 years ago
Comment on attachment 8505081 [details] [diff] [review]
Remove legacy transition code from the callscreen (author: thills)

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

In the future, please post changes as either a PR or patch(es).

::: apps/callscreen/js/call_screen.js
@@ -68,5 @@
> -    self.statusMessage.addEventListener('transitionend', function tend(evt) {
> -      evt.stopPropagation();
> -      self.statusMessage.removeEventListener('transitionend', tend);
> -      setTimeout(function hide() {
> -        self.statusMessage.classList.remove('visible');

I don't think we can safely remove this. Otherwise, we'll be stuck with status messages constantly visible. For example, when a member of a conference call hangs up on the call, we'll see "X has left the call" until a new status message is shown.

@@ +231,4 @@
>    _wallpaperImageHandler: function cs_wallpaperImageHandler(image) {
>      this.mainContainer.style.backgroundImage = 'url(' +
>        (typeof image === 'string' ? image : URL.createObjectURL(image)) + ')';
> +    setTimeout(this.setCallerContactImage.bind(this));

This is small enough that we should consider refactoring it so that it's part of `setCallerContactImage()`.

@@ +238,4 @@
>      var activeCallForContactImage = CallsHandler.activeCallForContactImage;
>      var blob = activeCallForContactImage && activeCallForContactImage.photo;
>  
>      this.contactBackground.classList.remove('ready');

I don't think we need this `ready` class anymore. I think the intention here was to flush the style and get the background image to fade to the new one, but this is not accomplishing that.

It's possible, though doubtful, that without this we will get a black screen or something. Please check what happens without it. This is also referred to in oncall.css:869.

::: apps/callscreen/style/oncall.css
@@ -183,5 @@
>      background: black;
>      z-index: 100;
>    }
>  
> -  /* This transition is needed to trigger a transitionend event in */

This change should be made in 'oncall_status_bar.css' as well.

::: apps/callscreen/test/unit/call_screen_test.js
@@ +411,1 @@
>      suite('once the transition is over', function() {

We don't need this suite anymore. We should keep the underlying tests, though.

@@ -776,5 @@
> -          this.sinon.clock.tick(2000);
> -          done();
> -        });
> -        test('should hide the banner', function() {
> -          assert.isFalse(bannerClass.contains('visible'));

We should keep this test and suite. See my comment in call_screen.js about this.
Attachment #8505081 - Flags: review?(drs+bugzilla) → review-
(Assignee)

Comment 7

4 years ago
Created attachment 8506142 [details] [review]
Changes after review

Hi Doug,

I put this in PR this time.  Sorry about that.  I also left the changes unsquashed for you... so there are two commits.  

Also, I tried removing the 'ready' class and the picture doesn't show up (we do get the wallpaper though).  So, I believe we need to keep that.

Thanks,
-tamara
Attachment #8505081 - Attachment is obsolete: true
Attachment #8506142 - Flags: review?(drs.bugzilla)
(Reporter)

Comment 8

4 years ago
Comment on attachment 8506142 [details] [review]
Changes after review

There are some issues to iron out here still. See the PR for my comments.
Attachment #8506142 - Flags: review?(drs.bugzilla) → review-
(Assignee)

Comment 9

4 years ago
Hi Doug, I commented on your comment on the PR:  https://github.com/mozilla-b2g/gaia/pull/25224.

If you can have a look, that will be great as need to discuss before we move this forward.

Thanks,
-tamara
Flags: needinfo?(drs.bugzilla)
(Reporter)

Comment 10

4 years ago
Answered on the PR.
Flags: needinfo?(drs.bugzilla)
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1075603
(Assignee)

Comment 12

4 years ago
Created attachment 8509771 [details] [review]
Updated PR after last review

Changes after the last review to fix the spacing and put back the wallpaper function.
Attachment #8503342 - Attachment is obsolete: true
Attachment #8506142 - Attachment is obsolete: true
Attachment #8509771 - Flags: review?(drs.bugzilla)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8509771 [details] [review]
Updated PR after last review

I'm getting a black screen when receiving an incoming call from a contact with an image. Please investigate this. I left a couple more comments on the PR.
Attachment #8509771 - Flags: review?(drs.bugzilla) → review-
(Reporter)

Comment 14

4 years ago
(In reply to Doug Sherk (:drs) (use needinfo?) from comment #13)
> I'm getting a black screen when receiving an incoming call from a contact
> with an image. Please investigate this.

I originally reproduced this on a Flame-JB build. I tried this again on a Flame-KK v188 build and I was no longer able to repro it. So I think it's safe to say that this isn't a problem.
(Assignee)

Comment 15

4 years ago
Hi Doug,

I fixed the one spacing comment on the latest PR.

Regarding the one comment about _wallpaperImageHandler needing a test, I checked and we already have two in this area:

https://github.com/tamarahills/gaia/blob/master/apps/callscreen/test/unit/call_screen_test.js#L358
https://github.com/tamarahills/gaia/blob/master/apps/callscreen/test/unit/call_screen_test.js#L376

Let me know if there was something else that needed addressing.

Thanks,
-tamara
Flags: needinfo?(drs.bugzilla)
(Reporter)

Comment 16

4 years ago
Comment on attachment 8509771 [details] [review]
Updated PR after last review

Yep, looks good.
Flags: needinfo?(drs.bugzilla)
Attachment #8509771 - Flags: review- → review+
(Reporter)

Updated

4 years ago
Target Milestone: 2.1 S7 (24Oct) → 2.1 S8 (7Nov)
(Assignee)

Comment 17

4 years ago
Created attachment 8521664 [details] [review]
Final PR after merge

New PR as I had merge conflicts.
Attachment #8509771 - Attachment is obsolete: true
(Assignee)

Comment 18

4 years ago
https://github.com/tamarahills/gaia/commit/2bb22dda265b1aaf8c5c969b6fb52188d8cb7578
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.2: --- → fixed
Resolution: --- → FIXED
Target Milestone: 2.1 S8 (7Nov) → 2.1 S9 (21Nov)
(Reporter)

Updated

4 years ago
Duplicate of this bug: 949877
You need to log in before you can comment on or make changes to this bug.