Closed Bug 1163319 Opened 9 years ago Closed 9 years ago

Pocket button in hamburger menu breaks layout

Categories

(Firefox :: Pocket, defect, P1)

38 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox38.0.5 --- verified
firefox39 --- verified
firefox40 --- verified

People

(Reporter: sphilp, Assigned: Dolske)

References

Details

Attachments

(2 files, 2 obsolete files)

Affects nightly 40.0a1 (2015-05-09)

Steps:

1. Move the pocket button to the hamburger menu (right click and select 'move to menu')
2. Open the hamburger menu
3. Click the pocket button to expand the subview (it's blank right now, I assume there is a change coming from pocket soon)
4. Close and reopen the hamburger menu

Actual:

Hamburger menu layout (or maybe just height) is broken (see screenshot)

Expected:

Menu should appear as normal
Noticed this yesterday; from bug 1163265 comment 4:

Since the subviews were already behaving without this patch, I did some checking to see what broke it (since I thought it was "working" before, in that the contents scrolled as they were too big for the subview, but the menupanel itself didn't get broken)... It was broken before the previous uplift (bug 1163111), and was in fact regressed by bug 1161793.

I see Stuart's already filed bug 1163319 on the issue, so let's continue this topic over there. Since it's an existing issue, I think we should go ahead and land the uplift here in this bug, as it doesn't make things any worse.
Blocks: 1161793
Priority: -- → P1
A few notes from my debugging:

* When the button is in the menupanel, clicking it triggers onPanelViewShowing() in Pocket.jsm as expected. But the popupshowing / popupshown listeners added there don't seem to fire correctly... The first event (popupshowning) can take many seconds to fire, if it fires at all (sometimes seems related to mousing out of the panel?!). The second event (popupshown) never fires. I'm actually more confused why popupshowing fires at all, since the menupanel is already open. I'd expect neither of these events to fire, and the code in bug 1161793 seems to just not work for subviews?

I worked around this, but it didn't actually help. :)

* Ultimately, it's the panel.sizeTo() call [in main.js :: resizePanel()] that's causing this. Commenting that out fixes the broken menupanel, but causes some other glitches... I sometimes get a content area that's roughly normal but with a tiny iframe in it, or when moving the button between the menu/toolbar I'll get a content area sized for the other location.

I suspect the problem is that the subview is resizing the menupanel instead of the actual subview area, and to fix the other glitches we'll need to make it always resize the right thing to the current size. (Or somehow have things Just Work -- still seems to me that as long as the iframe gets sized correctly, everything else should be fine.)
Attached patch Testing hack (obsolete) — Splinter Review
This is on top of bug 1163265. Fixes the menupanel sizing but other things get glitchy, as noted in last comment.
Attached patch Patch v.1 (obsolete) — Splinter Review
Ok, this seems to work.

The basic idea is that normally you don't have to explicitly do any panel sizing -- either the panel adapts to your content (in a standalone panel), or the content should adapt to the subview's size (since it's fixed).

This is a little tricky here, because there's an iframe in play, which provides a fixed-size viewport to its contents. Which throws a wrench into the way things should Just Work.

The problem here is further exacerbated by:

* iframes default to 100x100 in XUL. (By the skin's global.css, which seems bizarre.) In HTML it defaults to 300x150, so yay for inconsistency. But this basically means if you don't do _something_ with an iframe, you're going to get a crappy pre-set size.

* Setting the iframe width to 100% would be nice, except the <hbox id="pocket-panel-container"> flexes to eat up space instead. This seems like a useless wrapper, so I just nuked it.

Fixing these issues also conveniently means that this should take care of problems with scrollbars in the subview, unless the Pocket content is too large to innately fix in the fixed size. But it's working good for me so far.

A somewhat separate issue, as I mentioned in the last comment, is that the usual popup events don't fire / don't fire consistently for a subview. I've fixed these with setTimeout(...,0) to spin the event loop. setTimeout can be a bad hack, but it seems reasonable here. At least, I feel comfortable with the outset setTimeout because it clearly fixes the issue with ViewShowing firing slightly too soon, but I'm a bit less sure if the inner setTimeout takes care of any race issue from bug 1161793... I sorta suspect that the issues bug 1161793 was trying to fix are actually the issues I'm fixing here, though.

I've tested this on OS X, and everything seems to be working fine with combinations of logged in / logged out, toolbar / menupanel, first click / repeated click, and moving between the menu panel and toolbar.
Assignee: nobody → dolske
Attachment #8603775 - Attachment is obsolete: true
Attachment #8603790 - Flags: review?(jaws)
Comment on attachment 8603790 [details] [diff] [review]
Patch v.1

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

The changes to the Pocket side looked fine overall. Just one question regarding timing below. I'm making a build and will hammer on it and will let you know if I find any regressions.

::: browser/components/pocket/Pocket.jsm
@@ +31,5 @@
> +      window.pktUI.pocketButtonOnCommand(event);
> +      window.setTimeout(function() {
> +        window.pktUI.pocketPanelDidShow(event);
> +      }, 0);
> +    }, 0);

Just to confirm: Is there a scenario where the iframe might not have loaded by the time this calls pocketPanelDidShow? If it gets called before the iFrame loads that's where we run into some of the issues we hit in bug 1161793.

::: browser/components/pocket/main.js
@@ +472,5 @@
> +        }
> +
> +        // TODO: Not sure if this is needed for the addon in a standalone panel
> +        // TODO : Animate the change if given options.animate = true
> +        // getPanel().sizeTo(options.width, options.height);

FYI: You can remove these TODOs plus the commented out code, we no longer need it.
I ran a build with all 3 pending patches bug 1163265, bug 1163360, and bug 116319 in Mac OS 10.9.5 and everything seems to be working well, both in the toolbar and in the menu panel. 

I also tried to reproduce the race condition that was discussed above as a concern with the setTimeout method and could not cause the error to happen, so that hack seems to be working here too.

Looking good!
Comment on attachment 8603790 [details] [diff] [review]
Patch v.1

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

I'm fine with the first setTimeout but I would prefer if we set a load event listener on the iframe unless readyState says its already loaded
Attachment #8603790 - Flags: review?(jaws) → review+
(In reply to Nate Weiner from comment #5)

> Just to confirm: Is there a scenario where the iframe might not have loaded
> by the time this calls pocketPanelDidShow? If it gets called before the
> iFrame loads that's where we run into some of the issues we hit in bug
> 1161793.

Hmm, maybe? I think even bug 1161793 didn't ensure this, at least I would be mildly surprised if |popupshown| was guaranteed to only fire after the iframe loads. I suspect that since we're loading chrome:// it only takes one or two spins of the event loop and so this almost always works (via setTime or popupshow), but I wouldn't place much confidence in that. :)

I'm implementing what Jared suggested in comment 7.


> FYI: You can remove these TODOs plus the commented out code, we no longer
> need it.

Done.
Attached patch Patch v.2Splinter Review
It appears that when the button is in the menupanel, the subview's iframe is always (in my testing) already loaded and so we can call pktUI.pocketPanelDidShow() immediately. However, then the button is in the toolbar, the iframe is never (in my testing) already loaded and so we need to use the event listener. I'm not really sure why the two cases behave differently, but at least it means the two code paths seem reliable to test.

I was had problems to get the listener to get called at all, until I found that a _capturing_ listener works.

I also stopped passing the event into the pktUI calls. It's not currently using them, and given the different events/phases being used I'm wary of needlessly exposing these implementation details to that code.
Attachment #8603790 - Attachment is obsolete: true
(In reply to Justin Dolske [:Dolske] from comment #2)
> A few notes from my debugging:

One other interesting note from yesterday:

Before this patch we were setting iframe.width, and now we set iframe.style.width. The former seems to need... something... to happen before it actually updates the width. I was setting .width, but immediately checking .clientWidth was returning 100px. Which is surprising, because using accessing .clientWidth / .clientHeight is a nice trick to immediately trigger any pending reflows. Sometimes after setting .width the iframe size just wasn't changing at all -- I wonder if that's a layout bug with XUL panels + iframes?

Anyway, setting style.width worked fine and avoided this problem entirely.
Comment on attachment 8603882 [details] [diff] [review]
Patch v.2

[Triage Comment]

Required for Pocket / 38.0.5 release.
Attachment #8603882 - Flags: approval-mozilla-release+
Attachment #8603882 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/cc162b5c443c
https://hg.mozilla.org/releases/mozilla-release/rev/32b69592b334
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Iteration: --- → 40.3 - 11 May
Flags: qe-verify?
Flags: firefox-backlog+
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 40.0a1 (buildID: 20150511030203), latest Aurora 39.0a2 (buildID: 20150511004005) and Firefox 38.0.5 Beta 1 (buildID: 20150510205200).
Depends on: 1164253
You need to log in before you can comment on or make changes to this bug.