Closed Bug 818276 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect)

17 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox17 --- affected
firefox18 + verified
firefox19 + verified
firefox20 + verified
firefox-esr17 18+ verified

People

(Reporter: nitinmohan87, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(3 files)

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.
I checked the same webpage on other browsers (irrespective of OS). They all displayed the popup window with the scrollbar.
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
Blocks: 775965
Status: UNCONFIRMED → NEW
Component: General → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Assignee: nobody → roc
Attached file reduced testcase
This testcase should show a scrollbar in the IFRAME, but does not.
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().
Attached patch fixSplinter Review
Attachment #688595 - Flags: review?(matspal)
(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 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
Attachment #688595 - Flags: review?(matspal) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/7cd51e4e7d09
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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
Attachment #688595 - Flags: approval-mozilla-beta?
Attachment #688595 - Flags: approval-mozilla-aurora?
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.
Attachment #688595 - Flags: approval-mozilla-beta?
Attachment #688595 - Flags: approval-mozilla-beta+
Attachment #688595 - Flags: approval-mozilla-aurora?
Attachment #688595 - Flags: approval-mozilla-aurora+
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 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+
Attachment #688595 - Flags: approval-mozilla-esr17+
roc - will you be landing this as-is to esr17?
Duplicate of this bug: 820960
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
Keywords: verifyme
Verified fixed on FF 19b1 on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.8.2
Verified fixed on FF 20b1 on Win 7, Ubuntu 12.04 and Mac OS X 10.8.2
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.