Modal XUL dialog regression with FF 17 beta

RESOLVED FIXED in Firefox 17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: WeirdAl, Assigned: roc)

Tracking

({regression, testcase})

17 Branch
mozilla17
x86_64
Windows 7
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed, firefox18+ fixed)

Details

(Whiteboard: [qa?])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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.
status-firefox16: --- → unaffected
status-firefox17: --- → affected
tracking-firefox17: --- → ?

Comment 2

5 years ago
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
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?

Comment 3

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

Comment 4

5 years ago
(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.
Assignee: nobody → roc
(Reporter)

Comment 5

5 years ago
(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.
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.
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.
The fixed-pos hbox is positioned at y=1073738764.
Mixing right/bottom positioning with intrinsic window sizing is the problem here, I think.
(Reporter)

Comment 10

5 years ago
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).
status-firefox16: unaffected → ---
Created attachment 672668 [details] [diff] [review]
fix
Attachment #672668 - Flags: review?(matspal)
status-firefox18: --- → affected
tracking-firefox17: ? → +
tracking-firefox18: ? → +
tracking-firefox19: ? → ---
Comment on attachment 672668 [details] [diff] [review]
fix

r=mats
Attachment #672668 - Flags: review?(matspal) → review+
(Reporter)

Comment 13

5 years ago
Note:  The fix does not apply cleanly to FF 17 Beta branch.
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f18a8893e7ff
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.
Attachment #674090 - Flags: approval-mozilla-beta?
Attachment #674090 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f18a8893e7ff
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(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.
Keywords: qawanted, verifyme
(Reporter)

Comment 19

5 years ago
I can confirm that the patch for beta fixes the original issue we spotted this for.  Thanks!
Comment on attachment 674090 [details] [diff] [review]
beta patch

Thanks, will remove qawanted, approving for uplift.
Attachment #674090 - Flags: approval-mozilla-beta?
Attachment #674090 - Flags: approval-mozilla-beta+
Attachment #674090 - Flags: approval-mozilla-aurora?
Attachment #674090 - Flags: approval-mozilla-aurora+
Keywords: qawanted, verifyme
https://hg.mozilla.org/releases/mozilla-aurora/rev/be982655e24c
https://hg.mozilla.org/releases/mozilla-beta/rev/f0cf6f6f857c
status-firefox17: affected → fixed
status-firefox18: affected → fixed
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?
Whiteboard: [qa?]
You need to log in before you can comment on or make changes to this bug.