908654 - Chat window settings button misplaced upwards

Status: RESOLVED WORKSFORME

(Firefox Graveyard :: SocialAPI: Providers, defect)

Description

[Bogdan Maris, Desktop Test Engineering [OOO until Dec 16th]]

Attachment: Screencast showing the issue

Reproducible on Firefox 24 beta 5 (buildID: 20130822154523)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:24.0) Gecko/20100101 Firefox/24.0
Reproducible on the latest Aurora (buildID: 20130823004003)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:25.0) Gecko/20100101 Firefox/25.0
Reproducible on the latest Nightly (buildID: 20130822030204)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:26.0) Gecko/20100101 Firefox/26.0

Steps to reproduce:
1. Open Firefox
2. Turn on Messenger Firefox
3. Open two chat windows
4. Click the settings button from the first window opened.
5. Minimize the chat.
6. Maximize de chat and focus the settings button.

Expected results:
The settings button maintains its position and the chat area maintains its dimension.

Actual results:
The settings button is moved upwards and the chat area increases in hight.

Notes:
1. Screencast attached showing the issue.
2. This issue reproduces on 23.0.1 but not in 22.0 so it`s a regression.

Comment 1

[u279076]

It's odd that this is a regression since we don't serve the content of this window. What you are seeing is what Facebook is serving to us. Can you please do more extensive testing to confirm? If this is a regression we need a regression window to move forward.

Comment 2

[Bogdan Maris, Desktop Test Engineering [OOO until Dec 16th]]

This issue refers to Facebook Messenger not the demo social service (there is no settings button located in the chat window).

Last good nightly: 2013-04-17
First bad nightly: 2013-04-18
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=50ab959f4bd1&tochange=3ada6a2fd0c6

Comment 3

[u279076]

Thanks Bogdan. While this bug appears specific to Facebook it does look like we may have regressed something here. Looking at this range more closely I see the following changes:
 * Bug 809733 - add css transitions for width and height to chatboxes

Mark, since you wrote this patch could you check to see if there's anything which might have regressed this behaviour?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

This issue is the one described in bug 809733 comment 12 - which from the discussion there we did work around.  I'm not sure why it's come back - maybe Shane can recall other changes in that area?

Comment 5

[Shane Caraveo (:mixedpuppy)]

I can reproduce this on osx, but it does take some effort.  I don't recall other changes in that area that would affect this, I think it is just an edge case.  Unless it is 100% always happening on windows, I don't think this is a priority to figure out, it can be shelved for now.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> I can reproduce this on osx, but it does take some effort.

I can't repro on Windows, but note that we initially "solved" this by only sending the isActive notification once the transitionend event fires.  However, note that there are *2* transitionend events - one for width and one for height.  In my testing, "height" always fires first, but for both events, the width and height of the element are identical (ie, it's almost as if they fire at the exact same time).

However, let's say the "width" transitionend event fired before "height", and that the actual height during that event was an intermediate value (ie, as if the width transition finished while height was still underway), that would explain this bug.

Shane, I just added the following to the transitionend event in socialchat.xml:

        dump("TRANSITIONEND: " + event + "/" + event.propertyName + "\n");
        dump("Width=" + this.getBoundingClientRect().width + ", Height=" + this.getBoundingClientRect().height + "\n");
        let cs = document.defaultView.getComputedStyle(this);
        dump("CS - Width=" + cs.width + ", Height=" + cs.height + "\n");

if you can repro, maybe try the same patch and see what it reports when it can be reproduced?

[Sadly, even if this turns out to be true, I'm not sure what the solution would be - but it would still be good to know]

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Based on comment 5 it doesn't look like we need to track this, and can take a low risk uplift when available to branches.

Comment 8

[Shane Caraveo (:mixedpuppy)]

I'm no longer able to reproduce this.

ISTM that the transitionend events are queued and flushed (see https://mxr.mozilla.org/mozilla-central/source/layout/style/nsTransitionManager.cpp#1042) so we're getting those events, as the name indicates, once the transition is done and we're clear to do stuff.  The only way I can see this being the issue is if we can somehow receive transitionend prior to the element actually being correctly sized.

@ehsan, is my understanding of this correct?  Is there any chance we would get transitionend for height, continue some resizing transition, then get transitionend for width?

Comment 9

[Mark Hammond [:markh] [:mhammond]]

It seems clear to me that in the general case, transitionend could fire at different times for different attributes (and indeed, that code in nsTransitionManager is handling exactly that).  In this case, we are specifying the same time period.

Shane, can you verify that we do see 2 transitionend events, one with propertyName as 'width' and one with propertyName as 'height'?  If so, I suspect we could just act once we've seen both and call this fixed?

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Oops - I meant to say:
> In this case, we are specifying the same time period.

... so it's not immediately obvious how they would fire at different times (ie, how they would get out of sync) but that's probably due to my lack of imagination :)

Comment 11

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #9)

> Shane, can you verify that we do see 2 transitionend events, one with
> propertyName as 'width' and one with propertyName as 'height'?  If so, I
> suspect we could just act once we've seen both and call this fixed?

I do see 2 events.  I initially thought it was eventPhase, but it is not. One was for height and the other for width.  We do only set isActive once, but on the first call.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> I do see 2 events.  I initially thought it was eventPhase, but it is not.
> One was for height and the other for width.  We do only set isActive once,
> but on the first call.

Right - so that implies a patch to fix this should be easy to come up with (even if it is somewhat ugly...)?

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #12)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> > I do see 2 events.  I initially thought it was eventPhase, but it is not.
> > One was for height and the other for width.  We do only set isActive once,
> > but on the first call.
> 
> Right - so that implies a patch to fix this should be easy to come up with
> (even if it is somewhat ugly...)?

I'm unable to reproduce the problem, and am not entirely convinced that this is actually the cause of the problem described.  I'd rather not patch something without a solid STR to verify we actually are fixing a problem.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> I'm unable to reproduce the problem, and am not entirely convinced that this
> is actually the cause of the problem described.  I'd rather not patch
> something without a solid STR to verify we actually are fixing a problem.

Fair enough - if Bogdan can no longer repro it, let's just close it.

Comment 15

[(no longer active)]

(And FWIW I don't know the answer to your question, Shane.  Sorry!

Comment 16

[Bogdan Maris, Desktop Test Engineering [OOO until Dec 16th]]

I am not able to reproduce the issue on 23.0.1, on the previous bad nightly from 2013-04-18, Firefox 25 Beta 11, latest Aurora and latest Nightly so I will close this bug as WFM.