There are several issues here: - ideally, we shouldn't need a multiview at all inside a toolbar/standalone panel. - in my testing, we set the panel height 3-4 times when opening the main panel. That shouldn't be necessary. - we have no tests for this code (that is, for the height handling) - the desired behaviour (never have scrollbars under any circumstances) is not very well specified, that is, the code evolved to handle several cases that caused us to get it wrong, which may well have been/seemed entirely correct at the time, but it's hard to now clearly evaluate why the code functions the way it does.

When opening the panel, _syncContainerWithMainView gets called four times, for the following reasons: 1. As a result of the popupshowing event 2. After some elements have been added by another popupshowing listener. The size is increased by one pixel here over the value computed by case one. 3. When the maximum height is set (_setMaxHeight) in the popupshown listener, ignoreMutations is set and cleared. Clearing this state causes _syncContainerWithMainView to be called. The computed height is the same as case two. 4. When either _setMaxHeight or _syncContainerWithMainView sets the height, this causes a mutation, as the style attribute has just changed. Could we combine these by only doing the recomputation if a mutation actually occurred?

(In reply to Neil Deakin from comment #1) > When opening the panel, _syncContainerWithMainView gets called four times, > for the following reasons: > > 1. As a result of the popupshowing event > 2. After some elements have been added by another popupshowing listener. The > size is increased by one pixel here over the value computed by case one. > 3. When the maximum height is set (_setMaxHeight) in the popupshown > listener, ignoreMutations is set and cleared. Clearing this state causes > _syncContainerWithMainView to be called. The computed height is the same as > case two. > 4. When either _setMaxHeight or _syncContainerWithMainView sets the height, > this causes a mutation, as the style attribute has just changed. > > Could we combine these by only doing the recomputation if a mutation > actually occurred? I'm not really sure which of these you're suggesting to combine... Just the last two?

At least those ones. In addition, if the listener is step 1 responds after the one is step 2, it might be be possible only set the size if a mutation occurred there too.

[Tracking Requested - why for this release]: Fixes a regression in the Application Menu for which the footer of the main view is misaligned when the History view is open. This regression was introduced in bug 1206233 which landed in Firefox 51. It also fixes a regression with the height of the Downloads Panel, that is only partially fixed in bug 1292345 for Firefox 50.

Tracking 51+ so we can fix this regression.

Jared, can you take a look? This applies the proof of concept to the panels. Maybe you'll find out the reasons for the issues below sooner that I can. The only place where this works flawlessly is the Downloads Panel. You can test malware downloads using: https://testsafebrowsing.appspot.com/ The Application Menu has an issue where, apparently, the sync layout we're relying upon doesn't happen, so we don't detect the new height and don't do the transition while opening a subview. However, it works when closing a subview. The Control Center has still height calculation issues for scrollable areas, even after applying the patch in bug 1293242. I wonder if it's the same issue that can be seen in the History subview of the Application Menu. Overall, the animation on Mac OS X still looks janky, even if we don't do any work related to it in the multiview code.

Regarding the proof of concept patch: I loaded chrome://browser/content/customizableui/test.xul in my local build and the transitions are really smooth. However, setting mozStackSizingIgnore (so the blue outline appears) on one of the overflowed vbox's still leaves the scrollbar with one of the REMOVABLE rows outside of the scroll view. Did you intend to have all description elements in the body visible? Regarding the work in progress patch: The downloads panel is only correct when there is a download in the session. The panel is not tall enough when there are no downloads in the session. The text that says "No downloads for this session" isn't visible but the footer is visible. The application menu and control center are both too short, but I can confirm that the animation looks correct when entering and exiting subviews. I thought this may happen because the onTransitionEnd code fires too soon since there are actually two properties that we are transitioning on and this code shouldn't run until the `transform` transition has ended (it is running now when the `opacity` transition ends). I adjusted the onTransitionEnd function to return early if event.propertyName is "opacity" but that didn't fix the issues for me.

Daniel, any idea why the sync layout I'd expect to happen in the TODO at line 203 here is not always done? https://reviewboard.mozilla.org/r/75012/diff/1#4 Or is there any way I can debug this without reading tons of layout logs? I have to wait one tick for the height to be updated, which is not what I want because I don't want a paint event to happen. The process of determining the new height should not result in the controls being actually painted. What I want to do is to change the height of the Firefox main menu gradually using a transition, with the target height being determined by the layout engine instead of manual computation. There is also a proof of concept patch that works in a simpler case.

I don't know, offhand. How often is "not always"? For initial debugging, I'd suggest printing out the getComputedStyle()-reported height of the element in question (as well as the height of whatever contents/container you expect it to be deriving its height from) at various points in time... That might reveal something being out-of-place, but I'm not sure. (heads-up: I may be away for the next few days (I'm called for Jury Duty in my local court system tomorrow, and might get selected for a trial), so I may not have many cycles to assist with debugging. I'm also hoping that perhaps some Firefox-frontend person who's written a similar sort of patch before [with XUL & dynamic measuring/resizing-with-a-transition] might be better able to debug/diagnose what's going on here.)

(In reply to Daniel Holbert [:dholbert] from comment #11) > I'd suggest printing out the getComputedStyle()-reported height of the > element in question (as well as the height of whatever contents/container > you expect it to be deriving its height from) at various points in time... > That might reveal something being out-of-place, but I'm not sure. Heh, I've tried this route and it didn't reveal anything interesting except that we didn't get the new height. > perhaps some Firefox-frontend person who's written a similar sort of patch > before [with XUL & dynamic measuring/resizing-with-a-transition] might be > better able to debug/diagnose what's going on here.) The front-end debugging doesn't seem to help, and the platform behavior is different than what I would expect, so we might only find something that works by trial and error without understanding why it works. That's why I'm asking someone with better knowledge of how layout works exactly. Some more specific questions I have: - Are we hitting some optimization or incremental layout case? - Is it really guaranteed that we won't paint the intermediate state if we do two sync layouts in the same event loop tick? - Is there any better way to predict the new size by simulating a reflow without actually doing it? If you're not available or don't know the answers to these questions, is there anyone else that can help?

Adding dbaron for needinfo regarding comment 10, 11, and 12.

(In reply to :Paolo Amadini from comment #12) > - Are we hitting some optimization or incremental layout case? Not one that jumps out at me. > - Is it really guaranteed that we won't paint the intermediate state if we > do two sync layouts in the same event loop tick? I believe it is, yeah. (Except in cases where your script gets interrupted/suspended via alert() or via the slow-script dialog -- but that's not what's happening here.) > - Is there any better way to predict the new size by simulating a reflow > without actually doing it? Not that I'm aware of. > If you're not available or don't know the answers to these questions, is there anyone else that can help? It sounds like this needs some debugging time from a layout hacker. Mats, tn, xidorn all might be candidates. (I'm probably too backlogged to be able to give this the attention it'd deserve for at least a few more days.)

Ah, after all, the issue I've seen with the wrong height wasn't the layout engine's fault at all! The information in comment 14 is still really useful and tells me that the approach I'm taking here is solid. The problem was purely in the front-end for the History subview, because we delete all the menu items on the showing event and then populate the list asynchronously: https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/browser/components/customizableui/CustomizableWidgets.jsm#205-206 So, in the tick where we compute the new height, the list isn't populated yet. Thanks to the support added in bug 1259093, I added an aEvent.detail.addBlocker call at the beginning to delay showing the subview, and this solved the issue giving us a smooth height transition. Now the only thing I have to figure out is what is causing the scrollbars to appear in many cases, because too little vertical space is allocated for the potentially scrolling area.

I suspect that fixing bug 1293242 properly might resolve most of the problems with the scrollbars that I'm currently seeing.

Paolo, is this important for the 51 release? It looks like something that may be more reasonable to aim at 52 or 53. If we should wontfix this for 51 and instead push on finding an owner for bug 1293242, please let me or Gerry know. Thanks!

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18) > Paolo, is this important for the 51 release? No, definitely not important for 51! > If we should wontfix this for 51 > and instead push on finding an owner for bug 1293242, please let me or Gerry > know. Thanks! Having more eyes on bug 1293242 would be nice, Neil and myself are working on it but it's layout code which implies lots of complexity and manual debugging.

Is this still a P1?

This is needed for the perf work of Photon. Flagging ni for Florian in case Paolo can't get to it and if some hand-off is necessary.

At present, this is waiting for me to test the builds in bug 1293242.

Comment on attachment 8865215 [details] Bug 1009116 - Redo resizing architecture of panelmultiview. Setting feedback as this is related to bug 1354141 comment 40.

I'll rebase this over bug 1354141 as the latter will likely land before the dependencies, but you may start to take a look at this, it works fine on all the multiviews in my testing on trunk.

Comment on attachment 8865215 [details] Bug 1009116 - Redo resizing architecture of panelmultiview. https://reviewboard.mozilla.org/r/136888/#review141174 Hey, I hope it's okay if I drop in here. I'm a tad worried about this patch - particularly because I was under the impression that the goal was to accomplish the panel behaviours _without_ requiring synchronous layout or style flushes... and yet, I still see a number of places where that's happening (and once where it's happening in a loop). Is this just an intractable problem? Are sync flushes just how this feature has to be implemented? ::: browser/components/customizableui/PanelMultiView.jsm:217 (Diff revision 3) > + * > + * @param changeFn > + * This synchronous function is called to make the DOM changes > + * that will result in a new height of the viewStack. > + */ > + _transitionHeight(changeFn) { This function worries me a bit... There are a few places where we're dirtying the DOM and then querying layout. We're guaranteed to do sync reflows here, unless I'm mistaken. :/ Is this just not avoidable? ::: browser/components/customizableui/PanelMultiView.jsm:376 (Diff revision 3) > - if (this._shouldSetPosition()) { > - this._panel.adjustArrowPosition(); > - } > - > - if (this._shouldSetHeight()) { > + for (let element of viewNode.getElementsByTagName("description")) { > + element.style.removeProperty("height"); > + let height = element.getBoundingClientRect().height; > + if (height) { > + element.style.height = Math.ceil(height) + "px"; > - this._adjustContainerHeight(); > - } > - } > + } We're querying layout for a rect, and then potentially dirtying the DOM, in a loop. This is going to cause us to thrash. Is there no way to avoid this? Or perhaps to batch all of the height changes in a requestAnimationFrame?

(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #29) > Is this just an intractable problem? Are sync flushes just how this feature > has to be implemented? We discussed this, and this solution still immediately provides a performance benefit for all panelmultiview elements and doesn't cause visible glitches, that are unfortunately still present in the patches landed in bug 1354141. > There are a few places where we're dirtying the DOM and then querying > layout. We're guaranteed to do sync reflows here, unless I'm mistaken. :/ > Is this just not avoidable? Yes, we need to do this exactly two times, and exactly at the beginning of the transition. This is guaranteed to avoid painting the unstable intermediate state that we use to calculate the target height, and would cause the panel window size to change. There is possibly an alternative method that consists in re-implementing some of the logic from stack layout in JavaScript, but I'm not sure if it has better performance that having the layout code do all the work for us. We might be trading C++ layout code execution time for synchronous JavaScript code execution time. However, we can investigate this in a follow-up. > ::: browser/components/customizableui/PanelMultiView.jsm:376 > We're querying layout for a rect, and then potentially dirtying the DOM, in > a loop. This is going to cause us to thrash. Is there no way to avoid this? > Or perhaps to batch all of the height changes in a requestAnimationFrame? This is unfortunate, and is a workaround for a layout engine bug. It must be explicitly enabled, and only the Identity Block and the Downloads Panel currently need it. Again, this is better than the current workarounds that don't solve all the issues with resizing these two panels. Hope this clarifies!

(In reply to :Paolo Amadini from comment #30) > We discussed this, and this solution still immediately provides a > performance benefit for all panelmultiview elements and doesn't cause > visible glitches, that are unfortunately still present in the patches landed > in bug 1354141. We don't have visible glitches, because I hacked around it :)

(In reply to Mike de Boer [:mikedeboer] from comment #31) > We don't have visible glitches, because I hacked around it :) Ah, I think I misread bug 1363756, but it looks like that is just about finding a better implementation for an existing workaround.

(In reply to :Paolo Amadini from comment #30) > (In reply to Mike Conley (:mconley) - At work week, slow to respond from > comment #29) > > ::: browser/components/customizableui/PanelMultiView.jsm:376 > > We're querying layout for a rect, and then potentially dirtying the DOM, in > > a loop. This is going to cause us to thrash. Is there no way to avoid this? > > Or perhaps to batch all of the height changes in a requestAnimationFrame? > > This is unfortunate, and is a workaround for a layout engine bug. It must be > explicitly enabled, and only the Identity Block and the Downloads Panel > currently need it. Again, this is better than the current workarounds that > don't solve all the issues with resizing these two panels. But my understanding is that, given this code snippet: > ::: browser/components/customizableui/PanelMultiView.jsm:376 > (Diff revision 3) > > + for (let element of viewNode.getElementsByTagName("description")) { > > + element.style.removeProperty("height"); > > + let height = element.getBoundingClientRect().height; > > + if (height) { > > + element.style.height = Math.ceil(height) + "px"; > > - this._adjustContainerHeight(); > > - } > > - } > > + } We could: - remove all the height style props in 1 loop - then query all the height properties in another loop - then set all the properties in one loop - then adjust the container height outside the loop. which would cut the number of sync flushes to 2 instead of 2N (for N <description> elements in the viewNode), which seems worth it?

Does this patch currently depend on fixes in bug 1293242 or not? I'd like to test it out before r+'ing, but it's not clear to me how best to do that at this stage, also because it still needs rebasing anyway...

Comment on attachment 8865215 [details] Bug 1009116 - Redo resizing architecture of panelmultiview. https://reviewboard.mozilla.org/r/136888/#review142518 ::: browser/base/content/browser.css:11 (Diff revision 3) > @namespace html url("http://www.w3.org/1999/xhtml"); > @namespace svg url("http://www.w3.org/2000/svg"); > > :root { > --identity-popup-expander-width: 38px; > - --panelui-subview-transition-duration: 150ms; > + --panelui-subview-transition-duration: 1s; I assume this is just for testing? ::: browser/components/customizableui/CustomizableWidgets.jsm:179 (Diff revision 3) > viewId: "PanelUI-history", > shortcutId: "key_gotoHistory", > tooltiptext: "history-panelmenu.tooltiptext2", > defaultArea: CustomizableUI.AREA_PANEL, > onViewShowing(aEvent) { > + aEvent.detail.addBlocker(new Promise((resolve, reject) => { This feels like it could be split out to a separate patch and land more quickly. I already used it when doing a demo of some stuff at the end of the Photon work week. Have you audited the other views as to whether they might want something similar? ::: browser/components/customizableui/PanelMultiView.jsm:252 (Diff revision 3) > + // in the same tick as the code below, but we can do this before > + // setting the starting height in case the transition is not needed. > + this._panel.removeAttribute("width"); > + this._panel.removeAttribute("height"); > + > + if (oldHeight != newHeight) { If the oldHeight was obtained lazily and potentially mid-transition, this check doesn't seem right? ::: browser/components/customizableui/PanelMultiView.jsm:255 (Diff revision 3) > + // needed, we have to set the height to the old value, then force a > + // synchronous layout so the panel won't resize unexpectedly. This seems like it's just another type of workaround along the lines of the setTimeout() that bug 1354141 added and bug 1363756 tracks removing. Do we actually know what, specifically, is going wrong, and can't we just fix the panel code so it doesn't initially get it wrong and only gets it right after another re-layout for other reasons or something? ::: browser/components/customizableui/PanelMultiView.jsm:334 (Diff revision 3) > + if (this._panel.alignmentPosition.startsWith("before_")) { > + maxHeight = this._panel.getOuterScreenRect().bottom - > + this.window.screen.availTop; > + } else { > + maxHeight = this.window.screen.availTop + > + this.window.screen.availHeight - > + this._panel.getOuterScreenRect().top; > + } This should have a comment. What are we really doing here? Why does the position matter? I wonder if this set of changes would be easier to understand (and potentially bisect if we break *something*, which seems almost inevitable given how many edgecases we had to deal with during Australis that we've now partly forgotten about) if it was split up into multiple commits with separate descriptions of what issues were being fixed by what particular changes... but I don't know if that would be hard to do retrospectively at this point? ::: browser/components/customizableui/PanelMultiView.jsm:344 (Diff revision 3) > + maxHeight -= this.window.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils) > + .getBoundsWithoutFlushing(arrowBox) > + .height; What guarantees that layout has happened again here and that the transition is finished? Also, we should (probably do, after the rebase) cache and reuse the dwu here. ::: browser/components/customizableui/PanelMultiView.jsm:348 (Diff revision 3) > + // Add a fixed 8px margin to account for variable borders. > + this._viewStack.style.maxHeight = (maxHeight - 8) + "px"; This is smelly. Should this be a variable of sorts? Why 8px? ::: browser/components/customizableui/content/panelUI.inc.xul:14 (Diff revision 3) > flip="slide" > position="bottomcenter topright" > noautofocus="true"> > <panelmultiview id="PanelUI-multiView" mainViewId="PanelUI-mainView"> > <panelview id="PanelUI-mainView" context="customizationPanelContextMenu"> > - <vbox id="PanelUI-contents-scroller"> > + <vbox id="PanelUI-contents-scroller" flex="1"> What required this and does this still work in customize mode?

(In reply to :Gijs from comment #33) > which would cut the number of sync flushes to 2 instead of 2N (for N > <description> elements in the viewNode), which seems worth it? Definitely, but I think this will mainly benefit the subviews of the Control Center and Downloads Panel, where the number of visible description elements is greater than one. In the main Control Center view there is normally just one description element visible, so this won't make a lot of difference. That's why I just implemented the simplest thing. I can still do the approach above as a demonstration of good performance practice though? (In reply to :Gijs from comment #34) > Does this patch currently depend on fixes in bug 1293242 or not? Yes, it does. > I'd like to > test it out before r+'ing, but it's not clear to me how best to do that at > this stage, also because it still needs rebasing anyway... You can pull the current MozReview changeset and rebuild the older revision, which I know is time consuming, or wait until I've rebased the patch on a more recent changeset, which is still time consuming because an incremental C++ rebuild perform horribly anyways as soon as one header has changed...

(In reply to :Gijs from comment #35) > > + --panelui-subview-transition-duration: 1s; > > I assume this is just for testing? Sure. > ::: browser/components/customizableui/PanelMultiView.jsm:252 > > + if (oldHeight != newHeight) { > > If the oldHeight was obtained lazily and potentially mid-transition, this > check doesn't seem right? If we don't enter the block, then the layout code will handle all the sizing automatically, but without any transition. I think the failure case you're talking about is when the current height we measured is slightly different than the real one at this point in time, but by chance equal to the final height we want after the transition. The panel will just jump back to the right final height without a transition. > ::: browser/components/customizableui/PanelMultiView.jsm:255 > (Diff revision 3) > > + // needed, we have to set the height to the old value, then force a > > + // synchronous layout so the panel won't resize unexpectedly. > > This seems like it's just another type of workaround along the lines of the > setTimeout() that bug 1354141 added and bug 1363756 tracks removing. Do we > actually know what, specifically, is going wrong, and can't we just fix the > panel code so it doesn't initially get it wrong and only gets it right after > another re-layout for other reasons or something? Yes, this is definitely so. If we find the panel issue we can just wait for MozAfterPaint here. While the workaround is for a different case, the solution might converge with bug 1363756. > I wonder if this set of changes would be easier to understand (and > potentially bisect if we break *something*, which seems almost inevitable > given how many edgecases we had to deal with during Australis that we've now > partly forgotten about) if it was split up into multiple commits with > separate descriptions of what issues were being fixed by what particular > changes... but I don't know if that would be hard to do retrospectively at > this point? Apart from the obvious things like waiting on the history view, the other changes are all linked together, so I don't think it's possible to separate them in the first place. I agree we'll have to deal with some edge cases after this lands. > ::: browser/components/customizableui/PanelMultiView.jsm:344 > > + maxHeight -= this.window.QueryInterface(Ci.nsIInterfaceRequestor) > > + .getInterface(Ci.nsIDOMWindowUtils) > > + .getBoundsWithoutFlushing(arrowBox) > > + .height; > > What guarantees that layout has happened again here and that the transition > is finished? The height of the arrow never changes, so this doesn't matter. I'll add a comment. > Also, we should (probably do, after the rebase) cache and reuse the dwu here. Yes, I noticed the other changeset did that already, so I didn't bother adding the shared getter here. That might also become a lazy getter by the way. > ::: browser/components/customizableui/PanelMultiView.jsm:348 > (Diff revision 3) > > + // Add a fixed 8px margin to account for variable borders. > > + this._viewStack.style.maxHeight = (maxHeight - 8) + "px"; > > This is smelly. Should this be a variable of sorts? Why 8px? Mostly because any attempt to measure this accurately would probably hit rounding errors in some cases, and more importantly would end up depending on the arrowpanel structure, for example breaking if we move borders to other elements. So, I went with something that is much greater than any borders we're likely to have and still looks fine on screen. > ::: browser/components/customizableui/content/panelUI.inc.xul:14 > (Diff revision 3) > > flip="slide" > > position="bottomcenter topright" > > noautofocus="true"> > > <panelmultiview id="PanelUI-multiView" mainViewId="PanelUI-mainView"> > > <panelview id="PanelUI-mainView" context="customizationPanelContextMenu"> > > - <vbox id="PanelUI-contents-scroller"> > > + <vbox id="PanelUI-contents-scroller" flex="1"> > > What required this and does this still work in customize mode? The enclosing element changed from flexbox layout to XUL layout. With bug 1293242 fixed and the architecture change, it should fix the issue with the footer of the main view jumping around when closing the panel. I've tested customize mode and it looks fine.

By the way, the animation with new architecture is *way* smoother than the old one, also more than I remember from the last time I compared the two. We may have made good performance improvements to the platform since then.

(In reply to :Paolo Amadini from comment #37) > The enclosing element changed from flexbox layout to XUL layout. With bug > 1293242 fixed and the architecture change, it should fix the issue with the > footer of the main view jumping around when closing the panel. I've tested > customize mode and it looks fine. I've actually tested again without the flexbox change and I don't see the issue I remembered with the main view of the panel. This also surprisingly removes the dependency on bug 1293242 except in presence of wrapping text, now that the stack sizing part is moved to bug 1364115. I need to double check this again with a full build tomorrow, but it looks like we're closer to landing this than I expected.

(In reply to :Paolo Amadini from comment #36) > (In reply to :Gijs from comment #33) > > which would cut the number of sync flushes to 2 instead of 2N (for N > > <description> elements in the viewNode), which seems worth it? > > Definitely, but I think this will mainly benefit the subviews of the Control > Center and Downloads Panel, where the number of visible description elements > is greater than one. In the main Control Center view there is normally just > one description element visible, so this won't make a lot of difference. > That's why I just implemented the simplest thing. I can still do the > approach above as a demonstration of good performance practice though? I think we should do it, yes. Also because I misread the deletion so it's actually 1 vs. N layout flushes (which simplifies the resulting code if we split it out - 2 loops instead of 4) and because we know this stuff all adds up, and because we might also end up using it for the Photon stuff (see bug 1364738 - you can test by flipping the browser.photon.structure.enabled pref), and finally because I'm not sure if there's a layout bug on file that will fix this, that we'll get patched before then. I'm still a bit confused because the summary of bug 1293242 seems like fixing that should fix the thing this is working around, but you also said this patch depends on bug 1293242... so what do we need the workaround for, and is there a better fix on the horizon? Was there some other question this needinfo was for? (Sorry if I missed it, it's late, but I did re-read your comments several times... maybe I'm just blind. :-( )

(In reply to :Gijs from comment #40) > I'm still a bit confused because the summary of bug 1293242 seems like > fixing that should fix the thing this is working around, but you also said > this patch depends on bug 1293242... so what do we need the workaround for, > and is there a better fix on the horizon? The patch currently in bug 1293242 fixes a general block-in-box issue and this also affected the main view, but only because of the change to move away from the flexbox model, which I can now confirm was unnecessary and I just reverted. > Was there some other question this needinfo was for? (Sorry if I missed it, > it's late, but I did re-read your comments several times... maybe I'm just > blind. :-( ) No, that's all! I'll post an updated version that does not depend on the patch currently in bug 1293242, but depends on the spin-off bug 1364115.

There will be builds you can try if you don't want to rebuild the MozReview changeset locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c40f4ead7f1edd6b8d94c8cac030ccc870c2c37

Comment on attachment 8865215 [details] Bug 1009116 - Redo resizing architecture of panelmultiview. https://reviewboard.mozilla.org/r/136888/#review143498 ::: browser/base/content/browser.js:7740 (Diff revision 4) > button.setAttribute("class", "identity-popup-permission-remove-button"); > let tooltiptext = gNavigatorBundle.getString("permissions.remove.tooltip"); > button.setAttribute("tooltiptext", tooltiptext); > button.addEventListener("command", () => { > let browser = gBrowser.selectedBrowser; > - // Only resize the window if the reload hint was previously hidden. > + this._permissionList.removeChild(container); Are we sure we don't need to call descriptionHeightWorkaround here? ::: browser/components/customizableui/PanelMultiView.jsm:600 (Diff revision 4) > + // in the same tick as the code below, but we can do this before > + // setting the starting height in case the transition is not needed. > + this._panel.removeAttribute("width"); > + this._panel.removeAttribute("height"); > + > + if (oldHeight != newHeight) { > > ::: browser/components/customizableui/PanelMultiView.jsm:252 > > > + if (oldHeight != newHeight) { > > > > If the oldHeight was obtained lazily and potentially mid-transition, this > > check doesn't seem right? > > If we don't enter the block, then the layout code will handle all the sizing > automatically, but without any transition. I think the failure case you're > talking about is when the current height we measured is slightly different > than the real one at this point in time, but by chance equal to the final > height we want after the transition. The panel will just jump back to the > right final height without a transition. Jumping doesn't sound nice... can we avoid this somehow, maybe by keeping track of whether we're mid-transition? ::: browser/components/customizableui/PanelMultiView.jsm:690 (Diff revision 4) > + // anchor to the available margin of the screen, based on whether the > + // panel actually opened towards the top or the bottom. We use this to > + // limit its maximum height, which is relevant when opening a subview. > + let maxHeight; > + if (this._panel.alignmentPosition.startsWith("before_")) { > + maxHeight = this._panel.getOuterScreenRect().bottom - Does this flush, and is there some way we can get it without flushing?

(In reply to :Paolo Amadini from comment #30) > (In reply to Mike Conley (:mconley) - At work week, slow to respond from > comment #29) > > There are a few places where we're dirtying the DOM and then querying > > layout. We're guaranteed to do sync reflows here, unless I'm mistaken. :/ > > Is this just not avoidable? > > Yes, we need to do this exactly two times, and exactly at the beginning of > the transition. This is guaranteed to avoid painting the unstable > intermediate state that we use to calculate the target height, and would > cause the panel window size to change. > > There is possibly an alternative method that consists in re-implementing > some of the logic from stack layout in JavaScript, but I'm not sure if it > has better performance that having the layout code do all the work for us. > We might be trading C++ layout code execution time for synchronous > JavaScript code execution time. However, we can investigate this in a > follow-up. > > > ::: browser/components/customizableui/PanelMultiView.jsm:376 > > We're querying layout for a rect, and then potentially dirtying the DOM, in > > a loop. This is going to cause us to thrash. Is there no way to avoid this? > > Or perhaps to batch all of the height changes in a requestAnimationFrame? > > This is unfortunate, and is a workaround for a layout engine bug. It must be > explicitly enabled, and only the Identity Block and the Downloads Panel > currently need it. Again, this is better than the current workarounds that > don't solve all the issues with resizing these two panels. The requestAnimationFrame suggestion seems like a fine thing to explore to me. There is also some more async-ness in the new panel implementation, and we're working out the last few glitches there, so far, without adding more sync stuff. That said, the implementation this is replacing has oodles of sync reflows already - https://bugzilla.mozilla.org/buglist.cgi?quicksearch=whiteboard%3Aohnoreflow%20summary%3ApanelUI&list_id=13589958 shows at least: bug 1356670 bug 1358446 bug 1358703 and I'm pretty confident that moving this to happen less, and only at the start of transition instead of mid-transition all the time, will make things better in terms of in-the-field perf, as a first step. Plus we're significantly reducing code complexity here. So on the whole I think this can land more-or-less as-is and will be significantly better, and then we can chase down remaining issues afterwards.

I thought I tested this but I messed up in hg. Now that I'm actually testing this, I'm seeing this. Is this an unrelated bug in the downloads panel, or something?

(to be clear, both this and the other screenshot are mid-animation) I'm also seeing temporary scrollbars in the description in the subview in the identity panel, which I believe weren't there before. There are also temporary scrollbars in subviews in the main panel when opening history and other panels, but those were always there and already have a bug on file for them.

(In reply to :Gijs from comment #47) > I'm also seeing temporary scrollbars in the description in the subview in > the identity panel, which I believe weren't there before. I'll look in to this, probably we need -moz-hidden-unscrollable there.

(In reply to :Gijs from comment #45) > The requestAnimationFrame suggestion seems like a fine thing to explore to me. Yes, I was thinking we could use it to do the changes required at the start of the animation with the best timing. > There is also some more async-ness in the new panel implementation, and > we're working out the last few glitches there, so far, without adding more > sync stuff. In fact going forward I'd like to see how much code we can port from each other. > So on the whole I think this can land more-or-less as-is and will be > significantly better, and then we can chase down remaining issues afterwards. I'll file a few bugs before landing.

(In reply to :Gijs from comment #46) > Created attachment 8868648 [details] > Downloads - missing contents > > I thought I tested this but I messed up in hg. Now that I'm actually testing > this, I'm seeing this. Is this an unrelated bug in the downloads panel, or > something? Paolo, can you reproduce this and/or do you know what might be causing this? Is there maybe async state being fetched to show the downloads panel? It only reproduces on first opening for me, second/nth openings look good. Maybe we need to add a blocker like for the history view, that waits for whatever state it is to arrive first, or something?

(In reply to :Gijs from comment #50) > Paolo, can you reproduce this and/or do you know what might be causing this? This is something I hadn't tested again after removing the dependency from bug 1293242. The easiest solution is to change the description element to be single line - I don't think we ever wrap this text anyways. If we needed multi-line text here, we would have to call the description height workaround function when the element becomes visible after removing a download.

(In reply to :Gijs from comment #44) > > - // Only resize the window if the reload hint was previously hidden. > > + this._permissionList.removeChild(container); > > Are we sure we don't need to call descriptionHeightWorkaround here? This is done later below, after we've set the visibility of the reload description element. > > If we don't enter the block, then the layout code will handle all the sizing > > automatically, but without any transition. I think the failure case you're > > talking about is when the current height we measured is slightly different > > than the real one at this point in time, but by chance equal to the final > > height we want after the transition. The panel will just jump back to the > > right final height without a transition. > > Jumping doesn't sound nice... can we avoid this somehow, maybe by keeping > track of whether we're mid-transition? I don't think we should be concerned about this, we're likely talking about a few pixels and something not even noticeable, and the important thing is that we don't break the final state. > > + maxHeight = this._panel.getOuterScreenRect().bottom - > > Does this flush, and is there some way we can get it without flushing? This doesn't flush, it calls the platform-specific functions to get the window boundaries.

(In reply to :Paolo Amadini from comment #51) > change the description element to be single line I've done this and added the "panel-view-body-unscrollable" class. I've also changed the workaround code to set the height on wrapping description elements that currently have a single line, because my recent tests with the Downloads Panel show that it's still required for proper layout in some cases, and I think it's better to address the issue thoroughly than wonder why it doesn't work in the future.

I still have to investigate the leak in bug 1364911. In the meantime, I've started a new tryserver build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12fc7ac9cde55927efe9c4fb069669a6ce4372be

For the moment I'll reuse bug 1363756 for the investigation related to the second synchronous layout, and I've filed bug 1367111 for the requestAnimationFrame investigation. Now that bug 1364911 landed in inbound, this might be ready to land as well, but I had to rebase it so I'll start a new tryserver build first.

Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d16236446af Redo resizing architecture of panelmultiview. r=Gijs

Gijs, any suggested STR for this bug?

(In reply to Grover Wimberly IV [:Grover-QA] from comment #61) > Gijs, any suggested STR for this bug? OK. To test this, turn the photon pref *off*. Then check the sizing, animation, and appearance of the following panels and their subviews: - the identity panel (on http, https, https ev and chrome (e.g. about:preferences) pages, including the subview that appears when clicking the arrow, and ideally both when you've granted no permissions, and when you've granted a lot of them (you can use http://permission.site/ to help with this)) - the downloads panel (with no downloads, or with several downloads, and with some 'blocked' downloads that cause an arrow pointing to a subview to appear - see e.g. http://testsafebrowsing.appspot.com/ for a way to test that) - the main pre/non-photon hamburger panel and its subviews (Help, web developer, history, synced tabs, and bookmarks (for which you'd have to move the bookmarks button to the main panel menu) would all be good to check) - opening the aforementioned subviews (except 'help') as individual panels when those buttons are in the toolbar - opening the aforementioned subviews (except 'help') when those buttons are in the overflow panel (again, with the photon pref *off*) Note that I'm not even sure how well some of these scenarios behaved *before* this bug, so if you notice anything untoward we'd have to be quite careful checking whether this bug actually regressed that or whether it was a pre-existing issue.

(In reply to :Gijs from comment #62) > - opening the aforementioned subviews (except 'help') as individual panels > when those buttons are in the toolbar > - opening the aforementioned subviews (except 'help') when those buttons are > in the overflow panel (again, with the photon pref *off*) That's amazing. I didn't even know either of these was possible. I can see why Australis took such a long time to implement! I believe the height of the panel will now be incorrect if the main view is taller than the available space, but this affects the Photon version as well - try opening the Developer Tools submenu on a smaller screen after placing the window in such a way that the main menu button is in the middle of the screen. We should just share the code that sets the maximum height, and use the anchor position before the panel opens instead of the computed panel position after the panel opens. I'll file a bug if there isn't one already.

(In reply to :Paolo Amadini from comment #63) > (In reply to :Gijs from comment #62) > > - opening the aforementioned subviews (except 'help') as individual panels > > when those buttons are in the toolbar > > - opening the aforementioned subviews (except 'help') when those buttons are > > in the overflow panel (again, with the photon pref *off*) > > That's amazing. I didn't even know either of these was possible. I can see > why Australis took such a long time to implement! We only added the second one in the last few weeks because of the changes we're making in the overflow panel. In the worst case we can back that change out on beta, but it'd be messy.

I'm noticing information is cut off when looking up information on a website, when the photon pref is off as well as on. Is this related to this bug?

(In reply to Grover Wimberly IV [:Grover-QA] from comment #65) > Created attachment 8875864 [details] > InformationCutOff.png > > I'm noticing information is cut off when looking up information on a > website, when the photon pref is off as well as on. > > Is this related to this bug? This looks like bug 1369339. Can you re-test in tomorrow's nightly?

Verified on Windows, Mac, and Ubuntu.