802457 - Modal XUL dialog regression with FF 17 beta

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alex Vincent [:WeirdAl]]

Attachment: XPI testcase

Steps to reproduce:
(1) Install the attached extension in a Firefox profile
(2) There will be a new toolbar in Firefox with an "Open dialog!" button.
(3) Click on the button.

Expected results:
A dialog box appears with the words "powered by example.com" and a button below it.

Actual results:
Dialog box appears, but its contents are all black.

Comment 1

[Alex Vincent [:WeirdAl]]

My colleagues at APN discovered this in testing our company's extension in Firefox 17 beta.  This testcase is near-minimal:  almost any reduction of it leads to the black box disappearing.

Comment 2

[Loic]

The issue appears in FF18 firstly then has been backported into FF16/17.
If I disable HWA, the dialog box contents is invisible but the background stays gray (default). With HWA enabled, background is black.
In Firefox 16, the dialox box is "malformed" and the OK button can't be pressed: http://i.imgur.com/uc5fV.jpg

m-c
good=2012-09-17
bad=2012-09-18
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3f0587ce1774&tochange=0d3b17a88d5f

My guess goes to this suspected bug:
Robert O'Callahan — Bug 788877. Don't create nsDisplayFixedPosition items when the root scroll frame is inactive. r=Cwiiis,a=akeybl

Comment 3

[Loic]

For the record, the changelog for m-a repo:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=7676a9a06403&tochange=006174be2306

Comment 4

[Alex Keybl [:akeybl]]

(In reply to Alex Vincent [:WeirdAl] from comment #1)
> My colleagues at APN discovered this in testing our company's extension in
> Firefox 17 beta.  This testcase is near-minimal:  almost any reduction of it
> leads to the black box disappearing.

Can you confirm that the dialog box appears as desired in FF16? I see you marked that version as unaffected, but the suspected bug 788877 is in that version as well.

Comment 5

[Alex Vincent [:WeirdAl]]

(In reply to Alex Keybl [:akeybl] from comment #4)
> Can you confirm that the dialog box appears as desired in FF16? I see you
> marked that version as unaffected, but the suspected bug 788877 is in that
> version as well.

No, the dialog appears as Loic indicates in comment 2 - I don't have it in front of me, but I remember it clearly.  I was initially concerned only with the rendering being a black box; I didn't think to test whether the button was clickable or not.

Comment 6

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

We hit a lot of
NS_ERROR("D3D10 should never need to update ThebesLayers in an empty transaction");
I'm pretty sure that's the problem.

Comment 7

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

The dialog box looks like this:

<window class="dialog" id="hello" title="Hello" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  <vbox>
    <text id="hello-label" value="powered by example.com" style="padding: 16px;" />
  </vbox>
  <hbox style="position: fixed; right: 16px; bottom: 16px;">
    <button label="OK" />
  </hbox>
</window>

It looks to me like the button *should* overlap the label, as shown in the screenshot in comment #2. You should be able to click on it though.

The problem here actually seems to be a layout problem. The root frame for the dialog has [vis-overflow=0,0,13320,1073740864], which is very bad and is causing rendering/layers to blow up.

Comment 8

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

The fixed-pos hbox is positioned at y=1073738764.

Comment 9

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

Mixing right/bottom positioning with intrinsic window sizing is the problem here, I think.

Comment 10

[Alex Vincent [:WeirdAl]]

FWIW, the person who wrote the original code that my testcase derives from did not understand XUL - but since it was never a problem, we never even noticed... we can definitely fix our code to use XUL spacers and position things correctly, but it could take a little while (lots of little fires to put out).

Comment 11

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

Attachment: fix

Comment 12

[Mats Palmgren (inactive)]

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

r=mats

Comment 13

[Alex Vincent [:WeirdAl]]

Note:  The fix does not apply cleanly to FF 17 Beta branch.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Alex Vincent [:WeirdAl] from comment #13)
> Note:  The fix does not apply cleanly to FF 17 Beta branch.

Please nominate a patch that does apply cleanly for uplift to branches.  If we can get this in before EOD PT Monday (tomorrow) we can ship this fix in 17.0 Beta 3 and get testing on it with time to catch regressions if any occur.

Comment 15

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

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

Comment 16

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

Attachment: beta patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): possibly regressed by bug 788877, but it's unclear; in any case, it's probably an old bug that we just uncovered recently
User impact if declined: Unusual dialogs in some extensions will not render
Testing completed (on m-c, etc.): none really
Risk to taking this patch (and alternatives if risky): pretty low risk. Only affects intrinsic-sized windows (quite rare --- only XUL dialog boxes, really, so not Web content), and position:fixed is probably quite rare in those. No real alternative to taking the patch.
String or UUID changes made by this patch: none.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Testing completed (on m-c, etc.): none really

It's on nightly now, so adding qawanted to verify before approving uplift.

Comment 19

[Alex Vincent [:WeirdAl]]

I can confirm that the patch for beta fixes the original issue we spotted this for.  Thanks!

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 674090 [details] [diff] [review]
beta patch

Thanks, will remove qawanted, approving for uplift.

Comment 21

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/be982655e24c
https://hg.mozilla.org/releases/mozilla-beta/rev/f0cf6f6f857c

Comment 22

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: dialog

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 RC

The dialog box contains "powered by" text and an OK button which doesn't do anything when clicked. (see attached screenshot)
Did the functionality of the addon change?