Closed Bug 908654 Opened 7 years ago Closed 6 years ago

Chat window settings button misplaced upwards

Categories

(Firefox Graveyard :: SocialAPI: Providers, defect)

23 Branch
x86
macOS
defect
Not set

Tracking

(firefox23- affected, firefox24- affected, firefox25- affected, firefox26- affected)

RESOLVED WORKSFORME
Tracking Status
firefox23 - affected
firefox24 - affected
firefox25 - affected
firefox26 - affected

People

(Reporter: bogdan_maris, Unassigned)

Details

(Keywords: regression)

Attachments

(1 file)

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.
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.
Component: SocialAPI → SocialAPI: Providers
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
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?
Flags: needinfo?(mhammond)
Version: Trunk → 23 Branch
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?
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy)
(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]
Flags: needinfo?(mhammond) → needinfo?(mixedpuppy)
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.
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?
Flags: needinfo?(mixedpuppy) → needinfo?(ehsan)
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?
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 :)
(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.
(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...)?
(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.
(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.
Flags: needinfo?(ehsan) → needinfo?(bogdan.maris)
(And FWIW I don't know the answer to your question, Shane.  Sorry!
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(bogdan.maris)
Resolution: --- → WORKSFORME
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.