966072 - [Linux] UITour highlight panel appears below the menu panel

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Karl Tomlinson (:karlt)]

1. Set the browser.uitour.whitelist.add.testing preference to www-demo3.allizom.org
2. Load https://www-demo3.allizom.org/b/en-US/firefox/australis/update/
3. Click "Learn more" under "Take the tour"

A door hanger drops down from a grey icon with 3 horizontal bars.

Expected results:
While the browser window has focus, a highlight over the "Add-ons"
icon in the door hangler.

Actual results:
No highlight is visible.

Examining the window (with xwininfo) shows that the highlight window is below
the door hanger. The highlight window is a managed window, while the
door hanger is override redirect.

4. Click on the grey icon with 3 horizontal bars.

The door hanger closes and the highlight is now visible where the "Add-ons"
icon was.

Comment 1

[Matthew N. [:MattN]]

(In reply to Karl Tomlinson (:karlt) from comment #0)
> Examining the window (with xwininfo) shows that the highlight window is below
> the door hanger. The highlight window is a managed window, while the
> door hanger is override redirect.

It sounds like you have an understanding of what the problem is but I don't know enough about what you're saying to understand who can fix this. I guess this is also a panel-specific issue. Perhaps a different value of the @level attribute on the <panel> can workaround the issue?

Is this something you can help take a look at?

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to Matthew N. [:MattN] from comment #1)
> Perhaps a different value of the @level
> attribute on the <panel> can workaround the issue?

The GTK port doesn't use mPopupLevel, but the "level" <panel> attribute may do some other things.

Popup windows were originally designed to be short-lived windows and so there was only ever one popup open at a time.  There is not much available to control the z-order of multiple popups.

However, I expect that override-redirect windows will be placed above managed windows and I expect that the most recently created override-redirect window will be above other override-redirect windows.

> Is this something you can help take a look at?

I can do reviews on widget/gtk code, and I can try patches, but I'm not able to search for a solution, sorry.

I'm guessing a noautohide attribute on the highlight window is making it a managed window.  Can this be removed?  Or is that the only way to avoid a pointer grab?

Comment 3

[Matthew N. [:MattN]]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> However, I expect that override-redirect windows will be placed above
> managed windows and I expect that the most recently created
> override-redirect window will be above other override-redirect windows.

I don't know anything about these different types of windows but I know that on OS X, the most recently opened panel appears on top.

> > Is this something you can help take a look at?
> 
> I can do reviews on widget/gtk code, and I can try patches, but I'm not able
> to search for a solution, sorry.

Okay, Enn, is this something you will also have time to look into. Pretty please :)
 
> I'm guessing a noautohide attribute on the highlight window is making it a
> managed window.  Can this be removed?  Or is that the only way to avoid a
> pointer grab?

This seems plausible. The reason for the attribute is so that when the user clicks the arrow to go to the next step in the tour (part of the webpage), the panel shouldn't close in order to re-open to highlight the next panel item in the tour.

I could see changing this on Linux as a workaround if we don't get the proper fix. Perhaps instead of noautohide, I can prevent the popup from closing in the popup* event handler?

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Matthew N. [:MattN] from comment #3)
> (In reply to Karl Tomlinson (:karlt) from comment #2)
> > I'm guessing a noautohide attribute on the highlight window is making it a
> > managed window.  Can this be removed?  Or is that the only way to avoid a
> > pointer grab?
> 
> This seems plausible. The reason for the attribute is so that when the user
> clicks the arrow to go to the next step in the tour (part of the webpage),
> the panel shouldn't close in order to re-open to highlight the next panel
> item in the tour.
>
> I could see changing this on Linux as a workaround if we don't get the
> proper fix. Perhaps instead of noautohide, I can prevent the popup from
> closing in the popup* event handler?

I don't know about preventing the popup from closing in the popup* event handler.

However, the menu panel does not have noautohide (I assume) and yet it doesn't close when focus moves.  So there has been some change made to the menu panel; perhaps that same change could be made to the highlight window (instead of noautohide).

Or perhaps the highlight can be reopened on each step of the tour.
(The URL in comment 0 is no longer available, so I don't know how to reproduce.)

Comment 5

[Matthew N. [:MattN]]

(In reply to Karl Tomlinson (:karlt) from comment #4)
> However, the menu panel does not have noautohide (I assume) and yet it
> doesn't close when focus moves.  So there has been some change made to the
> menu panel; perhaps that same change could be made to the highlight window
> (instead of noautohide).

Actually, the menu panel does get noautohide when the tour causes it to open:
https://mxr.mozilla.org/mozilla-central/source/browser/modules/UITour.jsm?rev=54d9feac4e7a#711

> Or perhaps the highlight can be reopened on each step of the tour.

The highlight (panel) is being shown and hidden each time it moves, it's not simply being repositioned. This way we can change the anchor and the highlight panel will follow the proper anchor.

> (The URL in comment 0 is no longer available, so I don't know how to
> reproduce.)

The URL changed to https://www-demo3.allizom.org/en-US/firefox/29.0a2/whatsnew/ (and again soon to www.mozilla.org as the domain)

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Matthew N. [:MattN] from comment #5)
> Actually, the menu panel does get noautohide when the tour causes it to open:
> https://mxr.mozilla.org/mozilla-central/source/browser/modules/UITour.
> jsm?rev=54d9feac4e7a#711

Changing the attribute after the window has opened provides a new range of possibilities, even if it may not have been intended for such use.
Perhaps the same trick can be used to create the highlight without noautohide and then add it later, but then the highlight might also remain after focus changes to another app.

Comment 7

[Neil Deakin]

Popup attributes that relate to how the native widget is created are only checked when the popup frame/widget is created, so changing them later doesn't affect the way in which they appear.

The not hiding when clicking outside isn't done by the native widget so it happens to work (perhaps it shouldn't though to be consistent).

As a workaround though, you could make the panel hidden and show it again to force the native widget to be recreated. This might cause some loss of data in the panel, but I assume the UI tour is intended to appear at startup anyway.

By hiding I mean:

aWindow.PanelUI.panel.hidden = true;
aWindow.PanelUI.panel.clientWidth; // flush
aWindow.PanelUI.panel.hidden = false;

Comment 8

[Neil Deakin]

Also note that you'll need to do something similar when the panel is closed as well.

Comment 9

[Matthew N. [:MattN]]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> (In reply to Matthew N. [:MattN] from comment #5)
> > Actually, the menu panel does get noautohide when the tour causes it to open:
> > https://mxr.mozilla.org/mozilla-central/source/browser/modules/UITour.
> > jsm?rev=54d9feac4e7a#711
> 
> Changing the attribute after the window has opened 

I think you misread the code as it's setting the attribute a few lines before it's opened. As Neil points out, the attributes like that are only checked when the widget is created.

(In reply to Neil Deakin from comment #7)
> As a workaround though, you could make the panel hidden and show it again to
> force the native widget to be recreated.

I'm not really sure what you're getting at in comment 7 and 8. How would that help solve this bug? I'm confused. We already hide and show the highlight popup everytime it moves so that it appears on top again (at least on Windows/OS X). That doesn't seem to work on Linux.

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to Matthew N. [:MattN] from comment #9)
> (In reply to Karl Tomlinson (:karlt) from comment #6)
> > Changing the attribute after the window has opened 
> 
> I think you misread the code as it's setting the attribute a few lines
> before it's opened. As Neil points out, the attributes like that are only
> checked when the widget is created.

My use of "open" was bad.  I meant "create" not "show".

The attribute is having an effect, but differently on the menu panel than the highlight panel because on the menu panel it is added after create.

If the attributes are applied the same way to each of menu and highlight, then there is a better chance of the most recent being above the other.

Comment 11

[Neil Deakin]

> I'm not really sure what you're getting at in comment 7 and 8. How would
> that help solve this bug? I'm confused. We already hide and show the
> highlight popup everytime it moves so that it appears on top again (at least
> on Windows/OS X). That doesn't seem to work on Linux.

I tested the lines above and the highlight appeared above correctly. I mean hide the toolbar panel window, not the highlight panel, as that's the one you are changing noautohide on.

Comment 12

[Matthew N. [:MattN]]

Attachment: v.1 Recreate the PanelUI popup after changing @noautohide

Thanks Neil, now I understand what you're saying. I glossed over the fact that you were setting properties on aWindow.PanelUI.panel in comment 7 (and assumed it was on the highlight panel).

Comment 13

[Matthew N. [:MattN]]

I previously thought that each time the panel was shown, the native widget attribute would be re-applied. It does seems like this could possibly be handled better at a lower level. Is the reason this isn't a problem on Windows and Linux because @noautohide doesn't affect the native widget on those platforms?

Comment 15

[Neil Deakin]

Comment on attachment 8371995 [details] [diff] [review]
v.1 Recreate the PanelUI popup after changing @noautohide

That's seems ok as a workaround. Note that there is a performance impact here but I'm assuming it isn't too important for something that wouldn't be used normally.

Comment 16

[Neil Deakin]

(In reply to Matthew N. [:MattN] from comment #13)
> I previously thought that each time the panel was shown, the native widget
> attribute would be re-applied. It does seems like this could possibly be
> handled better at a lower level. Is the reason this isn't a problem on
> Windows and Linux because @noautohide doesn't affect the native widget on
> those platforms?

On Linux, we consider a noautohide popup to be a regular long-lived window. Other popups are expected to be short-lived. They are treated differently by the window manager and I think it also affects how the focus is treated with the two types. Other platforms don't have such a concept so don't treat either type of popup differently.

Comment 19

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/d591aee0b095

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8371995 [details] [diff] [review]
v.1 Recreate the PanelUI popup after changing @noautohide

[Approval Request Comment]
Bug caused by (feature/regressing bug #): UITour feature
User impact if declined: The highlights aren't visible for the menupanel widgets so we didn't enable the tour on Linux. With this fix, we can enable the tour on Linux for Aurora.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low-Medium risk mostly for the UITour but possibly affecting the menu panel outside the tour (if there is a bug). Possibility for the menu panel to flash/flicker.
String or IDL/UUID changes made by this patch: None

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 22

[Matthew N. [:MattN]]

Thanks!
https://hg.mozilla.org/releases/mozilla-aurora/rev/23688ad1574c

Comment 23

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 29
Build id:20140305004002

FF 30
Buid id: 20140304030204

OS:Ubuntu 12.04 x86