974804 - Calling recreatePopup on the menu panel before it's ready leads to a computed width of 0 for the panel scroller

Status: RESOLVED FIXED

(Firefox :: General, defect, P3)

Description

[Matthew N. [:MattN]]

Attachment: v.1 Don't unhide the panel in recreatePopup if it was hidden

See bug 949663 comment 12 which points to recreatePopup from bug 966072 causing the menu panel layout to be broken when UITour is the first thing to open the menu panel.

The problem is in PanelUI.ensureReady which lazily makes sure the panel menu is ready to use:
* It returns a promise and if called subsequent times, it simply returns the existing promise stored as PanelUI._readyPromise.
* recreatePopup would be called from UITour.jsm's showMenu before PanelUI.ensureReady was called. This would change PanelUI.panel.hidden to false from it's default value of true.
* The following width would be 0 inside ensureReady's task which would lead to a tall and super narrow #PanelUI-contents-scroller:
>         let cstyle = window.getComputedStyle(this.scroller);
>         let widthStr = cstyle.width;
This seems to happen because PanelUI.panel.hidden = false and that messes up the computed width of the menu panel.

I don't really understand why the task needed the menu panel to be hidden in order to get the proper computed width but I understand enough to see that UITour.recreatePopup shouldn't be setting aPanel.hidden = false if it was already hidden and that fixes this issue.

Comment 1

[Matthew N. [:MattN]]

Comment on attachment 8378828 [details] [diff] [review]
v.1 Don't unhide the panel in recreatePopup if it was hidden

Review of attachment 8378828 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/UITour.jsm
@@ +882,5 @@
>      // After changing popup attributes that relate to how the native widget is created
>      // (e.g. @noautohide) we need to re-create the frame/widget for it to take effect.
> +    if (aPanel.hidden) {
> +      // If the panel is already hidden, we don't need to recreate it? Check if it fixes the original Linux bug.
> +      // Maybe just flush?

Oops, I cleaned up these comments:
> // If the panel is already hidden, we don't need to recreate it but flush                                      
> // in case someone just hid it. 

I tested that this still fixes bug 966072.

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 8378828 [details] [diff] [review]
v.1 Don't unhide the panel in recreatePopup if it was hidden

Review of attachment 8378828 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for figuring this out.  r+, but if ensureReady's task really needs the panel to be hidden, wouldn't it be better to hide it when it starts?  It seems to already allow for the possibility that it may be hidden when it starts, because it unhides it at the end.

Comment 3

[Drew Willcoxon :adw]

I should be clearer: recreatePopup should not ultimately unhide the panel if it's hidden when it's called, but in addition, maybe ensureReady should hide the panel when it starts.

Comment 4

[Matthew N. [:MattN]]

(In reply to Drew Willcoxon :adw from comment #3)
> I should be clearer: recreatePopup should not ultimately unhide the panel if
> it's hidden when it's called, but in addition, maybe ensureReady should hide
> the panel when it starts.

Right, I agree with this and considered also fixing ensureReady. Since I don't really understand why ensureReady needs to be hidden for the proper computed width I didn't want to mess with it since the recreatePopup change was obvious. I also don't expect any other code to have a good reason to unhide the panel outside of panelUI.

Comment 5

[Matthew N. [:MattN]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=3b1ed1dd8ceb

Push: https://hg.mozilla.org/integration/fx-team/rev/86b6763e74b1

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86b6763e74b1

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8378828 [details] [diff] [review]
v.1 Don't unhide the panel in recreatePopup if it was hidden

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 966072
User impact if declined: There is potential for the UITour to cause the menu panel layout to break if the first thing it does is call showMenu("appMenu") right after startup.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low risk, pretty straightforward patch.
String or IDL/UUID changes made by this patch: None

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Landed with Gijs' blessing on IRC.

https://hg.mozilla.org/releases/mozilla-aurora/rev/34b3b73f135f

Comment 9

[Matthew N. [:MattN]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> Landed with Gijs' blessing on IRC.
> 
> https://hg.mozilla.org/releases/mozilla-aurora/rev/34b3b73f135f

This doesn't belong on Holly so should have had Australis in the commit message. There will be merge conflicts as that function shouldn't exist on Holly.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Sorry, wish our commit hook were a bit smarter than it apparently is.

https://hg.mozilla.org/releases/mozilla-aurora/rev/955fa8e5c6ea