818276 - Scrollbar on popoups (created by javascript) disappeared after updating to v17.0.1

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Nitin Mohan]

Step to reproduce
-----------------

Go to this webpage - http://rubydoc.info/docs/yard/YARD/Registry. Click on "Class List" or "Method List" on the top-right corner.

What should happen (Expected output)
------------------
A popup window with a scroll bar should be displayed.

What happened
-------------
Popup window with NO scrollbar was displayed.

I can confirm that this happens on Firefox v17.0.1 irrespective of OS (Linux, Windows, Mac). I can also confirm that scrollbar appeared in those popups when using Firefox v16.0.2 irrespective of OS.

Comment 1

[Nitin Mohan]

I checked the same webpage on other browsers (irrespective of OS). They all displayed the popup window with the scrollbar.

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813110945
http://hg.mozilla.org/mozilla-central/rev/22288130fea2
Bad:
http://hg.mozilla.org/mozilla-central/rev/d9183f015df8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120814055342
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=22288130fea2&tochange=d9183f015df8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813211942


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/303b75594832
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813210542
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4ddd9c731adc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813210943
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=303b75594832&tochange=4ddd9c731adc

Suspected : Bug 775965

Comment 3

[Alice0775 White]

Attachment: semi-reduced html zipfile

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: reduced testcase

This testcase should show a scrollbar in the IFRAME, but does not.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Changing the 'overflow' style on an IFRAME needs to call SetDefaultScrollbarPreferences on the docshell. Prior to bug 775965 landing, this happened automatically because changing 'overflow' recreated the frame, which meant we call nsFrameLoader::Show with the new 'overflow', which called SetDefaultScrollbarPreferences for us. Now Show() doesn't get called when we rescue the old presentation.

marginwidth/marginheight attributes have the same problem.

The solution is to add some plumbing so that margin/scrollbar state on the nsSubDocumentFrame can be propagated to the child whenever they change, without going through nsFrameLoader::Show().

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> The solution is to add some plumbing so that margin/scrollbar state on the
> nsSubDocumentFrame can be propagated to the child whenever they change,
> without going through nsFrameLoader::Show().

Actually it's OK to go through Show(). We just need to set the properties on the docshell before Show() bails out, and ensure the child document is reflowed as necessary.

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 688595 [details] [diff] [review]
fix

Looks good to me, but should the code comment say "... or there is a margin."
(you seem to say so in comment 5 above).  Also, is it possible to avoid
the reflow if the margin is zero and scrollbar preferences are the default?

r=mats

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mats Palmgren [:mats] from comment #8)
> Looks good to me, but should the code comment say "... or there is a margin."
> (you seem to say so in comment 5 above).

Sure.

> Also, is it possible to avoid
> the reflow if the margin is zero and scrollbar preferences are the default?

Yes, but I don't think optimizing this case is very important. It only occurs when we reframe an IFRAME.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cd51e4e7d09

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/7cd51e4e7d09

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 688595 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 775965
User impact if declined: Scrollbars fail to appear in some (rare) cases
Testing completed (on m-c, etc.): just landed on m-c
Risk to taking this patch (and alternatives if risky): Seems like a very low-risk patch. In some relatively rare cases, it sets some extra, safe attributes on a docshell and triggers some additional reflow.
String or UUID changes made by this patch: None

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 688595 [details] [diff] [review]
fix

Low risk, fixes a pretty obvious regression - please land this on branches today so it gets into beta 4 tomorrow.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

While this doesn't fit the typical criteria for ESR landings, it's enough of a user-facing (and annoying) regression that it should go into out 17.0.2esr shipping alongside 18.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 688595 [details] [diff] [review]
fix

pre-approving for ESR17 on the assumption that this will land cleanly there. If for some reason it doesn't and a separate patch is needed, please carry forward the a+

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/732e7813017c
https://hg.mozilla.org/releases/mozilla-beta/rev/58ed9187f536

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

roc - will you be landing this as-is to esr17?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/894976bf2a99

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks a ton Ryan!

Comment 22

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on Nightly 2012-12-05.
Verified fixed on FF 18b4, Nightly 2012-12-16-03-45-00-mozilla-esr17 on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.7.5

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 19b1 on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.8.2

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 20b1 on Win 7, Ubuntu 12.04 and Mac OS X 10.8.2