341950 - XUL elements in chrome hidden by content

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Tony Chang (Google)]

(this probably doesn't belong in safe browsing)

On trunk, XUL elements can't be placed above the browser element.  This breaks safe browsing which places a canvas element and a vbox over the browser element.

Comment 1

[Tony Chang (Google)]

Attachment: warning bubble under browser

Comment 2

[Jesse Ruderman]

Sounds like a roc bug.

Comment 3

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

Is this a recent regression? I thought Fritz had cooked up something to work on trunk.

Comment 4

[Tony Chang (Google)]

(In reply to comment #3)
> Is this a recent regression? I thought Fritz had cooked up something to work on
> trunk.

Yes, it was working a couple weeks ago before the DOM_AGNOSTIC landing.  Now I can't seem to place anything over the browser element.

Comment 5

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

Would you mind tracking down the regression window?

Comment 6

[Tony Chang (Google)]

(In reply to comment #5)
> Would you mind tracking down the regression window?
> 

With MOZ_CO_DATE=2006-06-17, it works.

With MOZ_CO_DATE=2006-06-18, I get the screen shot in the attachment.

Comment 7

[Tony Chang (Google)]

(In reply to comment #6)
> (In reply to comment #5)
> > Would you mind tracking down the regression window?
> > 
> 
> With MOZ_CO_DATE=2006-06-17, it works.
> 
> With MOZ_CO_DATE=2006-06-18, I get the screen shot in the attachment.
> 

Err, ignore that. That's when I submitted the change to safebrowsing so the bug is visibile.  Let me figure out the actual day.

Comment 8

[Tony Chang (Google)]

Further tests show that this isn't a recent regression (same behavior as on 2006-05-19 build).  It's just different behavior on the bonecho branch than on trunk.  I'll work around it in the safebrowsing code by using the same technique as before (i.e., before bug 329711) by setting browser.collapsed = true.

If the sidebar comes back on trunk, this isn't ideal because we don't get resize events from a collapsed browser.


My test was to run:
var vbox = document.createElement('vbox');                                                    
var cwin = document.getElementById('main-window');                                    
cwin.appendChild(vbox)
vbox.style.position = 'fixed';
vbox.style.height = '400px';
vbox.style.width = '400px';
vbox.style.top = '10px';
vbox.style.left = '10px';
vbox.style.backgroundColor = '#0f0';

In the JS shell in chrome context.  On bonecho, the green square is on top of the browser.  On trunk, it's under the browser.

Comment 9

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

ah, OK. This is just the same underlying issue that's always been there, manifesting in a different way.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

*** Bug 345478 has been marked as a duplicate of this bug. ***

Comment 11

[Ryan Jones-Ward [:sciguyryan]]

Also occurs on Windows - guessing this is cross OS.

Also, shouldn't this be critical?

-- Ryan Jones

Comment 12

[Ria Klaassen (not reading all bugmail)]

*** Bug 343791 has been marked as a duplicate of this bug. ***

Comment 13

[Ryan Jones-Ward [:sciguyryan]]

*** Bug 359158 has been marked as a duplicate of this bug. ***

Comment 14

[Ryan Jones-Ward [:sciguyryan]]

*** Bug 360805 has been marked as a duplicate of this bug. ***

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We should just fix this by changing the phishing protection display to take advantage of the frame display list linkage that roc landed the other day.

Comment 16

[Biju]

Please change summary to 
"Safe browsing notification is hidden by content area"

as it is not currently it is not visible on bugzilla search result
see http://snipurl.com/129n3

IMHO "XUL elements can't be placed above the browser element"
should be a different bug which block this one
and it should be some where under core/layout

Comment 17

[Tony Chang (Google)]

(In reply to comment #15)
> We should just fix this by changing the phishing protection display to take
> advantage of the frame display list linkage that roc landed the other day.

Actually, I think that fixing that bug fixed this bug.  With the latest trunk nightly I'm seeing the warning on top of the browser content area like what is seen on the bonecho branch.

For reference, bug 130078 is the frame display list linkage.

Comment 18

[Tony Chang (Google)]

*** Bug 362811 has been marked as a duplicate of this bug. ***

Comment 19

[Phil Ringnalda (:philor)]

*** Bug 364563 has been marked as a duplicate of this bug. ***

Comment 20

[Bill Gianopoulos [:WG9s]]

(In reply to comment #17)
> (In reply to comment #15)
> > We should just fix this by changing the phishing protection display to take
> > advantage of the frame display list linkage that roc landed the other day.
> 
> Actually, I think that fixing that bug fixed this bug.  With the latest trunk
> nightly I'm seeing the warning on top of the browser content area like what is
> seen on the bonecho branch.
> 
> For reference, bug 130078 is the frame display list linkage.
> 
Ths is odd.  I still see the issue with current windows nightlies.

Comment 21

[Tony Chang (Google)]

(In reply to comment #20)
> (In reply to comment #17)
> > For reference, bug 130078 is the frame display list linkage.
> > 
> Ths is odd.  I still see the issue with current windows nightlies.

The patch for bug 130078 was backed out so this bug exists again.

https://bugzilla.mozilla.org/show_bug.cgi?id=130078#c51

Comment 24

[Tony Chang (Google)]

If we decide to not show the content of phishing (as suggested in bug 380932), then dimming the content won't be a problem.  We can use something like browser.collapsed = true to hide the browser area or show the error in content.

Comment 25

[Dave Townsend [:mossop]]

I'd add that fixing this would be a real bonus to extension authors. It must be almost every week that someone comes into #extdev wanting to put something over the top of the webpage.

Comment 26

[u279076]

I can confirm that this bug exists on Linux and XP when running Minefield 3.0a5 build id 2007060104.

Comment 28

[Mike Connor [:mconnor]]

So, one potential solution is to reuse, in less dramatic fashion, the full stop error page we're using for malware.  Due to the infinite redirect technique for evading the blocklist, this actually helps make this more effective.  Assigning to Johnath so we can move forward with this, in the absence of compositor...

moving to M9, M8 will not be a beta.

Comment 30

[Steffen Wilberg]

Can't the safe browsing popup use the same technique as the new bookmark dialog  (star button) or the identity dialog (Larry) which display properly above the content area, if we can't fix this bug in general?

Comment 31

[Johnathan Nightingale [:johnath]]

I think we should morph this bug to cover moving phishing UI to the same "error-page, don't load the content" treatment that we added for malware in bug 380932, as suggested by mconnor in comment 28.  Updating the summary to reflect that more clearly.

By all means, let's move both phishing and malware to the "door hanger" treatment mentioned in bug 385988 when that becomes an option, but for now we should at least be consistent.

The easiest way to do this is to just add/move anti-phishing strings to the netError page as was done with malware, using the same styling as malware, etc.

The downside is that this will offer no click-through, which is something we currently offer for anti-phishing.  A second option would therefore be to add the anti-phishing-specific functionality to netError, and then use CSS to hide/unhide it depending on error.  This means adding some task-specific stuff to an otherwise generic page, I'd like to know what someone like biesi thinks about that, since he reviewed the original malware changes to netError.

The third option is to decide that we are now officially too complicated to piggyback on netError, and just fork it off to something like blacklist.xhtml, which would behave similarly to netError, but have more freedom to include special functionality.

In scenarios 2 & 3, the click-through support would also seem to require some kind of docshell flag to say "please ignore URI classifier hits on the blacklist."  I know Dave is working on that already, but I also know that docshell has apparently run out of bits to attach to new purposes.

Finally, in all cases, the URI classifier code will need to be told about this new UI, and much of the old safebrowsing UI should be removed. 

Comment 32

[Jesse Ruderman]

Johnath, I think you should make a new bug for that workaround / UI change.  This is a real bug with dependencies, a testcase, a regression range, etc.

Comment 33

[Danil Ivanov]

<!-- overlay | workaround (Comment #8) -->
<window id="main-window">
  <vbox style="position:fixed; height:400px; width:400px; top:10px; left:10px; background:#0f0">
    <deck style="min-width:100%;min-height:100%;">
      <vbox>
        <label value="???"/>
      </vbox>
    </deck>
  </vbox>
</window>

Comment 34

[Johnathan Nightingale [:johnath]]

(In reply to comment #32)
> Johnath, I think you should make a new bug for that workaround / UI change. 
> This is a real bug with dependencies, a testcase, a regression range, etc.

Yeah, I think I agree.  The more I look at this bug, particularly comments like comment 25, the more I think that the anti-phishing aspect is only one piece.  The rest, though, doesn't belong in the Phishing Protection component.

I've opened bug 399233 to track the broken anti-phishing UI, and I'll unmorph this to refer more generically (once again) to chrome content being hidden by browser content.  Based on conversation with Jesse and mconnor, I'll transfer it over to what looks like the right part of Core, and reset the nom flag to ask drivers whether they still think this one should still block FF3.

Comment 35

[Johnathan Nightingale [:johnath]]

Whoops - blocking flag got lost when it jumped from Firefox to Core, re-nom'ng.  This may or may not need to be a blocker now, given that bug 399233 has been nom'd for the anti-phishing aspects.

Comment 36

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

> vbox.style.position = 'fixed';

Ah, I finally figured out the story here!

The story is that position:fixed elements used to get native widgets created for them. On trunk they don't.

Try using "overflow:auto" instead.

Comment 37

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

Actually I'm going to mark this WONTFIX, since what happened is we broke a hack workaround for the chrome-over-content problem. Other hack workarounds exist and should be used instead.

Comment 38

[Roberto Bouzout (tito)]

Your suggestion about using "overflow:auto" works, BUT there is a problem with the borders of the elements in Firefox 3.
Can you please explain some of these "hack workarounds" to get this working as expected.

I'm using the following code:

var vbox = document.createElement('vbox');                                      
var cwin = document.getElementById('main-window');                              
cwin.appendChild(vbox)
vbox.style.position='fixed';
vbox.style.overflow= 'auto';
vbox.style.height = '400px';
vbox.style.width = '400px';
vbox.style.right = '10px';
vbox.style.top= '10px';
vbox.style.border='10px';
vbox.style.borderColor='black';
vbox.style.borderStyle='solid';
vbox.style.backgroundColor = '#0f0';

Comment 39

[Roberto Bouzout (tito)]

Attachment: without overflow:auto, firefox 3 all versions

Comment 40

[Roberto Bouzout (tito)]

Attachment: with overflow:auto, only borders hidden by content, firefox 3 all versions

Comment 41

[Roberto Bouzout (tito)]

This bug resurrected with 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091028 Minefield/3.7a1pre

Comment 42

[Roger Zimmermann]

Actually, this 'bug' resurrected already with Firefox 3.6 IMHO.

Looks like XUL 'panel' is the (only?) way to go in Firefox 3.6

In Firefox itself 'panel' is used for various elements that appear on top of 'anything', like bookmark panel, site identity popup...
http://mxr.mozilla.org/mozilla1.9.1/source/browser/base/content/browser.xul#121
http://mxr.mozilla.org/mozilla1.9.2/source/browser/themes/pinstripe/browser/browser.css#997

Comment 43

[Roberto Bouzout (tito)]

This bug is fixed on current trunk!!!!!!! YAY!

Comment 44

[Roberto Bouzout (tito)]

Attachment: Firefox 4.0b9pre

Update picture for firefox 4.