Last Comment Bug 802457 - Modal XUL dialog regression with FF 17 beta
: Modal XUL dialog regression with FF 17 beta
Status: RESOLVED FIXED
[qa?]
: regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 17 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-16 18:54 PDT by Alex Vincent [:WeirdAl]
Modified: 2012-11-19 09:27 PST (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
XPI testcase (2.35 KB, application/x-xpinstall)
2012-10-16 18:54 PDT, Alex Vincent [:WeirdAl]
no flags Details
fix (5.59 KB, patch)
2012-10-17 21:28 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
beta patch (5.36 KB, patch)
2012-10-22 19:04 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
dialog (7.74 KB, image/jpeg)
2012-11-19 09:26 PST, Mihaela Velimiroviciu (:mihaelav)
no flags Details

Description Alex Vincent [:WeirdAl] 2012-10-16 18:54:44 PDT
Created attachment 672129 [details]
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] 2012-10-16 18:56:19 PDT
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 2012-10-17 14:04:52 PDT
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 4 Alex Keybl [:akeybl] 2012-10-17 16:13:30 PDT
(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] 2012-10-17 16:25:10 PDT
(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) (Exited; email my personal email if necessary) 2012-10-17 19:53:12 PDT
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) (Exited; email my personal email if necessary) 2012-10-17 20:03:30 PDT
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) (Exited; email my personal email if necessary) 2012-10-17 20:04:09 PDT
The fixed-pos hbox is positioned at y=1073738764.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-17 20:22:53 PDT
Mixing right/bottom positioning with intrinsic window sizing is the problem here, I think.
Comment 10 Alex Vincent [:WeirdAl] 2012-10-17 20:26:24 PDT
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) (Exited; email my personal email if necessary) 2012-10-17 21:28:26 PDT
Created attachment 672668 [details] [diff] [review]
fix
Comment 12 Mats Palmgren (vacation) 2012-10-18 12:20:30 PDT
Comment on attachment 672668 [details] [diff] [review]
fix

r=mats
Comment 13 Alex Vincent [:WeirdAl] 2012-10-19 16:38:42 PDT
Note:  The fix does not apply cleanly to FF 17 Beta branch.
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 09:30:55 PDT
(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) (Exited; email my personal email if necessary) 2012-10-22 18:56:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f18a8893e7ff
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-22 19:04:55 PDT
Created attachment 674090 [details] [diff] [review]
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] 2012-10-23 03:56:51 PDT
https://hg.mozilla.org/mozilla-central/rev/f18a8893e7ff
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-23 08:26:46 PDT
(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] 2012-10-23 13:10:09 PDT
I can confirm that the patch for beta fixes the original issue we spotted this for.  Thanks!
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-24 15:38:08 PDT
Comment on attachment 674090 [details] [diff] [review]
beta patch

Thanks, will remove qawanted, approving for uplift.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-26 02:08:55 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/be982655e24c
https://hg.mozilla.org/releases/mozilla-beta/rev/f0cf6f6f857c
Comment 22 Mihaela Velimiroviciu (:mihaelav) 2012-11-19 09:26:15 PST
Created attachment 683166 [details]
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?

Note You need to log in before you can comment on or make changes to this bug.