Closed Bug 974804 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

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 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 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.
Attachment #8378828 - Flags: review?(adw) → review+
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/86b6763e74b1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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
Attachment #8378828 - Flags: approval-mozilla-aurora?
Attachment #8378828 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [Australis:P-]
Whiteboard: [Australis:P-]
(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.
Flags: needinfo?(ryanvm)
Sorry, wish our commit hook were a bit smarter than it apparently is.

https://hg.mozilla.org/releases/mozilla-aurora/rev/955fa8e5c6ea
Flags: needinfo?(ryanvm)
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.