844466 - When opening a link from a modal dialog, the tab strip doesn't show the new tab until you close the dialog

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Mayank Bansal]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130223 Firefox/22.0
Build ID: 20130223031157

Steps to reproduce:

1. Start fresh profile
2. Go to Nightly - > options - > options - >advanced - > update -> show update history. 
3. A dialogue box appears with the update history.
4. Click on any  "details" link
5. For added effect, click several times on the "details" link.


Actual results:

A tab should open in the background with the link opened.


Expected results:

No new tab is shown, until you close all the dialogue boxes. The page loads, but no new tab is opened.

Comment 1

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a17cb882eb6b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121213 Firefox/20.0 ID:20121213162730
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2cf3cca4171e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121213 Firefox/20.0 ID:20121213171921
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a17cb882eb6b&tochange=2cf3cca4171e

Suspected:
4cb7e17d0e3d	Dão Gottwald — Bug 263433 - 'text-link' XUL widget should open tabs rather than windows. r=enn

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Dao - can you investigate the options for this regression vs. intended behaviour?  This doesn't sound like a blocker because I'm assuming people don't go to 'show details' in their history that often but can this happen in other, larger web-breaking circumstances?

Comment 3

[Dão Gottwald [:dao]]

The page opens, the tab bar just doesn't update until you close the dialog. I don't think this needs to be tracked. It also doesn't affect web content in any way.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

It may not break web content but on closer inspection, we can't predict all the ways this can regress the user experience (and what about addons?) can you look at a potential fix we can get testing around or a backout if necessary?

Comment 5

[Dão Gottwald [:dao]]

(In reply to Lukas Blakk [:lsblakk] from comment #4)
> It may not break web content but on closer inspection, we can't predict all
> the ways this can regress the user experience (and what about addons?)

I don't know what this means.

> can
> you look at a potential fix we can get testing around or a backout if
> necessary?

A backout isn't necessary even if we don't fix this.

Comment 6

[Mayank Bansal]

IMO this bug doesnt affect normal browsing in any way.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Just tried reproducing this on FF20 Mac OS X and was not able to.  With a clean profile, clicking on 'details' in show update history opens tab after tab in the background of the dialog window.  QA - can you do some poking around here to determine the platforms affected here?

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to Lukas Blakk [:lsblakk] from comment #4)
> > It may not break web content but on closer inspection, we can't predict all
> > the ways this can regress the user experience (and what about addons?)
> 
> I don't know what this means.

Might there be add-ons that expect behaviour in a XUL widget to show content in browser tabs even without closing the widget window?
> 
> > can
> > you look at a potential fix we can get testing around or a backout if
> > necessary?
> 
> A backout isn't necessary even if we don't fix this.

We've already got one dupe and FF20 beta just released last Friday, it looks like this might be more noticeable to users and fixing it worthwhile.

Comment 10

[Alice0775 White]

See, browser.preferences.instantApply,
Modal option dialog is  Windows only.

Comment 11

[u279076]

Mac: details link opens in a new window (not affected by this bug)
Win: details link opens in a new tab (affected by this bug)
Lin: details link opens in a new tab (not affected by this bug

This appears to be Windows only.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Lukas Blakk [:lsblakk] from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > (In reply to Lukas Blakk [:lsblakk] from comment #4)
> > > It may not break web content but on closer inspection, we can't predict all
> > > the ways this can regress the user experience (and what about addons?)
> > 
> > I don't know what this means.
> 
> Might there be add-ons that expect behaviour in a XUL widget to show content
> in browser tabs even without closing the widget window?

No, I don't think there's any add-on impact.

> We've already got one dupe and FF20 beta just released last Friday, it looks
> like this might be more noticeable to users and fixing it worthwhile.

It is worth fixing and I'm looking into it, but this may be complicated and, if I get it fixed, too risky to uplift. In that case, I wouldn't consider this issue severe enough to warrant a backout.

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch v1: start tab opening animation via mozRequestAnimationFrame

https://tbpl.mozilla.org/?tree=Try&rev=2ba05c0f96c1

Comment 14

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #13)
> Created attachment 719956 [details] [diff] [review]
> patch
> 
> https://tbpl.mozilla.org/?tree=Try&rev=2ba05c0f96c1

Lots of intermittent failures on OS X.

Comment 15

[Dão Gottwald [:dao]]

Attachment: patch v2: start tab opening animation off MozAfterPaint

This uses MozAfterPaint rather than mozRequestAnimationFrame. This would mean that we'd waste a frame before starting the tab-opening animation.

https://tbpl.mozilla.org/?tree=Try&rev=dd9f061285b4

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch v3: start tab opening animation synchronously

This flushes layout and starts the animation synchronously. The layout flush might be redundant, since some previous code in addTab seems to have done it already.

I believe this is visibly more responsive than waiting for MozAfterPaint.

https://tbpl.mozilla.org/?tree=Try&rev=f57c2be05c01

Comment 17

[Dão Gottwald [:dao]]

Both v2 and v3 are basically green on try, v3 just needs browser_bug585785.js adjusted.

Comment 18

[Dão Gottwald [:dao]]

Attachment: patch v3.1: start tab opening animation synchronously

Comment 19

[Dão Gottwald [:dao]]

Marking wontfix for Firefox 20 since the fix seems too risky to be uplifted to beta. Aurora might be feasible if we land it soon.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 721657 [details] [diff] [review]
patch v3.1: start tab opening animation synchronously

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

LGTM.

Comment 21

[Dão Gottwald [:dao]]

Attachment: patch v3.2: start tab opening animation synchronously

as discussed on IRC

Comment 22

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d27445d1eac5

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d27445d1eac5

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 721686 [details] [diff] [review]
patch v3.2: start tab opening animation synchronously

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 263433
User impact if declined: tab-opening animation is delayed until the user closes the modal dialog blocking the browser window
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): somewhat risky. we can alternatively wontfix this bug for Firefox 21
String or UUID changes made by this patch: none

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 721686 [details] [diff] [review]
patch v3.2: start tab opening animation synchronously

Considering where we are in the cycle & based on the fact that we do not know the impact of this not getting fixed for Fx20,lets go ahead with the uplift on aurora and see how it plays out.

Comment 26

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd308f80fedb

Comment 27

[tabmix.onemen]

After this bug landed transitionend is not fired if the new tab is move before transitionend

for example:
var newTab = gBrowser.addTab("about:blank", {relatedToCurrent: false});
gBrowser.moveTabTo(newTab, 0);

result:
transitionend event handler is not run and _handleNewTab is not run

Comment 28

[Dão Gottwald [:dao]]

(In reply to onemen.one from comment #27)
Please file a new bug.

Comment 29

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0

The new tab is now opened when clicking Details.

Comment 30

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 - 20130514181517.

Comment 31

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old