Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:59.0) Gecko/20100101 Firefox/59.0 Build ID 20171207235447 [Affected version]: Nightly 59.0a1 [Affected platforms]: Mac OS X 10.12 and Windows 10 x 64 [Steps to reproduce]: 1.Launch Nightly 59.0a1 2.Drop down the Bookmarks, view pocket list, History, Downloads, Synced tabs, Screenshots menu 3. Click on Bookmarks and then Bookmarking tools [Expected results]: The menu "Bookmarking tools" should dropping down without jumping. [Actual results]: When you open the "Bookmarking tools" menu, it jumps.

When you say "jumps," do you mean that the menu/panel's height suddenly shrinks without animating? That's what I see.

Ah sorry, now I see that you posted a video. Thanks. CC'ing Mike and Gijs, who have worked on Photon panel animations and resizing.

Paolo, this looks like it might be a regression from one of your changes? Valentina, can you find a regression window?

Hi Gijs, I did a mozregression and here is the push log: 6:32.68 INFO: No more inbound revisions, bisection finished. 6:32.68 INFO: Last good revision: f249f5238ef931d68cd8ede3c3617825a06290c0 6:32.68 INFO: First bad revision: 35425d2a0d903e361910194df47e829c590f3a2b 6:32.68 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f249f5238ef931d68cd8ede3c3617825a06290c0&tochange=35425d2a0d903e361910194df47e829c590f3a2b

Since we hide the previous view, we need to provide an explicit min-height on the view container. This is currently the height of the main view in all cases, but it should match the height of the tallest view that was scrolled out. We actually have to store this value separately for each new opened view, because we have to use it when going back.

59:fix-optional because P4.

I'm continuing some of the refactoring in bug 1432015 and bug 1432016 here because the next steps will fix this bug. I've updated the dependency tree to make the order clearer. The full patch stack is always available in the tryserver builds and MozReview pushes. This will introduce some more checks against re-entrancy, although not exhaustive, which means that we may start to see related failures in tests that have to be fixed. Let's see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=807612450ffe164d1a581e569aa4fd5f813daf15

Comment on attachment 8944742 [details] Bug 1424264 - Part 1 - Always update min-width and max-width before showing views. https://reviewboard.mozilla.org/r/214890/#review220538 Static analysis found 1 defect in this patch. - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint check path/to/file` (Python/Javascript/wpt) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/customizableui/PanelMultiView.jsm:819 (Diff revision 1) > } > > + set minMaxWidthPx(value) { > + let style = this.node.style; > + if (value) { > + style.minWidth = style.maxWidth = value + "px" Error: Missing semicolon. [eslint: semi]

Comment on attachment 8944742 [details] Bug 1424264 - Part 1 - Always update min-width and max-width before showing views. https://reviewboard.mozilla.org/r/214890/#review220686 r=me, but perhaps we can think of a better name for the width thing? Maybe `containerMinMaxWidth` or `panelConstrainedWidth` or something? (I don't think the px bit adds anything, also because we don't have units anywhere else and there isn't much else we can measure width in that would make sense here -- but if you feel strongly, I won't r- on that suffix :-) ) ::: browser/components/customizableui/PanelMultiView.jsm:379 (Diff revision 1) > - // Because the 'mainview' attribute may be out-of-sync, due to view node > - // reparenting in combination with ephemeral PanelMultiView instances, > - // this is the best place to correct it (just before showing). > + > + let reverse = !!aPreviousView; > + if (!reverse) { Shouldn't we keep this comment? ::: browser/components/customizableui/PanelMultiView.jsm:383 (Diff revision 1) > + // or because we are showing the main view. We have to make sure that > + // the view appears properly based on how it's been opened and on the > + // size of the previous views. That we want the view to "appear properly" is obviously true and doesn't really add anything. I think it'd be more helpful to explain individually what is happening here. Maybe keep the previous comment about the main view, then have a single line comment about minMaxWidthPx explaining it constrains the width of the parent panelmultiview (which isn't obvious from just the setter name, so maybe that should be fixed!), and a single-line comment about the header (if that's even necessary). ::: browser/components/customizableui/PanelMultiView.jsm:820 (Diff revision 1) > > + set minMaxWidthPx(value) { > + let style = this.node.style; > + if (value) { > + style.minWidth = style.maxWidth = value + "px" > + } else { It's probably worth documenting what this does and that passing 0 resets this in a doc comment here.

Comment on attachment 8944744 [details] Bug 1424264 - Part 3 - Move state related to view sizing to the PanelView class. https://reviewboard.mozilla.org/r/214894/#review220694 r=me in principle but I'm very skeptical that we can just cache these heights/widths indefinitely. Why aren't we clearing them? Is the idea that we just overwrite them whenever the view is next a 'previous' view? That seems... fragile. ::: browser/components/customizableui/PanelMultiView.jsm:572 (Diff revision 1) > // Can't use Object.assign directly with a DOM Rect object because its properties > // aren't enumerable. This comment no longer applies and should be removed. ::: browser/components/customizableui/PanelMultiView.jsm (Diff revision 1) > - for (let panelView of this._viewStack.children) { > - if (panelView.nodeName != "children") { > - panelView.__lastKnownBoundingRect = null; > - } > - } Why don't we need to reset the knownHeight/knownWidth things?

Comment on attachment 8944745 [details] Bug 1424264 - Part 3 - Use the minimum height from the older transitioning view. https://reviewboard.mozilla.org/r/214896/#review220698 ::: browser/components/customizableui/PanelMultiView.jsm:552 (Diff revision 1) > // 'current' attribute, since it's needed for other state-keeping, but use > // a separate 'in-transition' attribute instead. > previousViewNode.setAttribute("in-transition", true); > // Set the viewContainer dimensions to make sure only the current view is > // visible. > - this._viewContainer.style.height = Math.max(prevPanelView.knownHeight, this._mainViewHeight) + "px"; > + let oldestView = reverse ? nextPanelView : prevPanelView; This doesn't match previous behaviour when you're more than 2 levels in^ (or, as far as I can tell from looking at code, when going back to a main view), and `oldestView` isn't actually the oldest view. We shouldn't decrease height beyond that of the main view. r- for this. ^ like in the library panel, or library view in the hamburger panel, or character encoding in 'more' in the hamburger panel, ...

I need more brain cells than 00:20am grants me so the rest of the review (part 2) will have to wait for tomorrow. Sorry!

(In reply to :Gijs (lower availability until Jan 27) from comment #13) > r=me, but perhaps we can think of a better name for the width thing? Maybe > `containerMinMaxWidth` or `panelConstrainedWidth` or something? Technically this applies to the individual view node only, and in the future we may even want to avoid applying this to some subviews, for instance WebExtensions panels, supporting a width transition for them as well. In fact, if we made the Library panel and the main menu panel the same width, we could apply these constraints statically with a CSS class and may be able to remove this logic completely. I think it is misleading to imply that setting this always constrains the width of the parent panelmultiview. That task is done by other code, in the class for the parent element. > (I don't > think the px bit adds anything, also because we don't have units anywhere > else and there isn't much else we can measure width in that would make sense > here -- but if you feel strongly, I won't r- on that suffix :-) ) I added it mainly to remove this ambiguity from the callers: style.minWidth = n + "px"; object.minWidth = n; It's also true that most setters in the codebase except the ones on the "style" object don't want the unit, so I'm fine with dropping the suffix.

(In reply to :Gijs (lower availability until Jan 27) from comment #14) > r=me in principle but I'm very skeptical that we can just cache these > heights/widths indefinitely. Why aren't we clearing them? Is the idea that > we just overwrite them whenever the view is next a 'previous' view? That > seems... fragile. These values are relevant to retain only for views that are invisible but still open, according to the terminology in part 2. When these dimensions are captured the view must be visible, even if it is about to become invisible. Hence, the common denominator is that these apply to open views and can be cleared when the view is closed. So, the initial idea was to reset these in a "close" method on the view, which would basically do this and also call "clearNavigation". Where we call "clearNavigation" on teardown we would call "close" instead, at the same time as the view is removed from the "openViews" stack. The "clearNavigation" method would still exist separately for the mouse move case. This is more correct but doesn't have an effect apart from clearing two numeric properties that we don't use anyways. Let me know if you think I should implement this cleanup now, maybe just for clarity, or for the moment a comment on the method would be enough. If we are careful and do everything in the right order, we may be able to reset these cached dimensions also in the transition of the view from invisible to visible. I don't know yet if this will be possible at all, but for sure at the moment the code isn't ready yet, as the "current" and "in-transition" attributes both control visibility and are set at different times. In reality we need only one of these attributes. I plan to do this simplification later, also allowing the knownViews list to go away entirely.

By the way, if it helps with the review of part 2, I was under the impression that the handling of openViews was still inconsistent, sometimes including the main view and sometimes falling back to it being the default current view when the array was empty. Now the state should be aligned and always include the main view. Also, as a side note, you may notice that at this point the main view is opened in the background before the panel is even shown. This happens on XBL construction, which is synchronous and conditional on the panel not being hidden. If the callers used a method on PanelMultiView to open the panel instead, we could be able to initialize the main view asynchronously. I might be able to tackle this later, and may be required for de-XBLing the panel, but for the moment we may be able to fix the race conditions without changing how this works yet.

(In reply to :Gijs (lower availability until Jan 27) from comment #15) > (or, as far as I can tell from looking at code, when going back to a main view) > We shouldn't decrease height beyond that of the main view. As far as I can tell from my manual tests, this never happens, unless I missed something. Just so we're talking about the same thing, make sure to try out the patch if you haven't already, otherwise the paragraph below might not make much sense... > This doesn't match previous behaviour when you're more than 2 levels in^ > ^ like in the library panel, or library view in the hamburger panel, or > character encoding in 'more' in the hamburger panel, ... This is different than the behavior currently in Release, but I think is a better one. The difference is small enough that I didn't consider necessary to consulting UX, but feel free to involve them if you feel is needed. My interpretation in fact was that the current behavior was not the originally intended one, just an artifact of being too difficult to change at the time while having to support both Photon and non-Photon. The panel should never shrink below the height of the _previous_ view, not the main view. Otherwise, when navigating forward the "Bookmarking Tools" would shrink to a seemingly arbitrary height, that would be different if this was opened in the Library panel, the main menu panel, or the overflow button. Also, some views like "Bookmarking Tools" would have a shrink effect, while others like "More" wouldn't, even if they have less items, just by virtue of being first or second level subviews. To be fair, there are some subviews where we now have "tons" of empty space rather than "loads" of empty space, but if empty space is a problem I think we should instead consider shrinking views below the height of the main view even if they are first level subviews. We can always move this UX change to a different bug, but I think this is the proper way of addressing comment 0, that's why I did it here. >, and `oldestView` isn't actually the oldest view. Right, maybe it should be olderView? Suggestions for something better than "old" are also welcome! I avoided "leftmost" because it implies a specific animation and reading direction, and "startmost" doesn't sound great in this context either, although it makes for an inside joke ;-) (And we have a few instances of this term in layout code!)

(In reply to :Paolo Amadini from comment #20) > (In reply to :Gijs (lower availability until Jan 27) from comment #15) > > This doesn't match previous behaviour when you're more than 2 levels in^ > > ^ like in the library panel, or library view in the hamburger panel, or > > character encoding in 'more' in the hamburger panel, ... > > This is different than the behavior currently in Release, but I think is a > better one. The difference is small enough that I didn't consider necessary > to consulting UX, but feel free to involve them if you feel is needed. I'll doublecheck on slack anyway, as it's cheap to ask. :-) > My interpretation in fact was that the current behavior was not the > originally intended one, just an artifact of being too difficult to change > at the time while having to support both Photon and non-Photon. The panel > should never shrink below the height of the _previous_ view, not the main > view. I think this makes sense as long as this is transitive. That is, if you have e.g.: 5 rows -> 10 rows -> 2 rows then the last view has the same height as the previous view, but also: 10 rows -> 5 rows -> 2 rows should have the same height, because we shouldn't shrink when going to the 5 rows view. I guess when I looked at the code last night it wasn't clear to me that this was the case. Is it? And if so, can you elaborate on why that is? If the heights are per-panelview, then at least in theory this could lead to odd cases when doing e.g.: 1. open hamburger panel 2. open library view from there 3. while that's open, directly click the library button to re-open the library view there. This should show the library view at its "natural" height, not the one that it got because it was opened in the hamburger panel. Obviously this is an edgecase, but we should at least make sure that it doesn't break, as we've broken these edgecases in the past. > >, and `oldestView` isn't actually the oldest view. > > Right, maybe it should be olderView? Suggestions for something better than > "old" are also welcome! I avoided "leftmost" because it implies a specific > animation and reading direction, and "startmost" doesn't sound great in this > context either, although it makes for an inside joke ;-) (And we have a few > instances of this term in layout code!) Yeah, maybe 'olderView' would make more sense. Naming is hard etc.

Comment on attachment 8944743 [details] Bug 1424264 - Part 2 - Open views before the transition and close them after it. https://reviewboard.mozilla.org/r/214892/#review220840 I sympathize with guarding against re-entrancy here, but then we also really need to ensure that the `_transitioning` flag is always cleared at some point. The current situation, esp. also because we don't watch for transitioncanceled etc. seems pretty fragile. ::: browser/components/customizableui/PanelMultiView.jsm:348 (Diff revision 1) > + if (this._transitioning) { > + return; > + } Shouldn't we queue this for when the current transition is finishing? No-op'ing here feels strange, and I don't immediately see from the patch context here that we already do this... More generally this feels scary because it'll break navigation in the panels if the transition doesn't clean up properly, and given that that depends on the right events firing that feels too fragile. ::: browser/components/customizableui/PanelMultiView.jsm:490 (Diff revision 1) > + // Close the view that was not displayed, considering that the panel > + // may have been hidden in the meantime. > + if (this.openViews.length) { > + this.openViews.pop(); > + } Technically this can happen when reversing, and then popping the last openView is wrong, I think? Not sure what we should do in that case, tbh... If going back fails through a ViewShowing cancel, we should probably close the panel and we don't do that today... perhaps we should add that? ::: browser/components/customizableui/PanelMultiView.jsm:510 (Diff revision 1) > + // The panel may have been hidden in the meantime. > + if (reverse && this.openViews.length) { > + this.openViews.pop(); > + } `[].pop()` doesn't throw, so I think we don't need the .length guards? I'm also not sure what the comment here implies.

(In reply to :Paolo Amadini from comment #18) > (In reply to :Gijs (lower availability until Jan 27) from comment #14) > > r=me in principle but I'm very skeptical that we can just cache these > > heights/widths indefinitely. Why aren't we clearing them? Is the idea that > > we just overwrite them whenever the view is next a 'previous' view? That > > seems... fragile. > > These values are relevant to retain only for views that are invisible but > still open, according to the terminology in part 2. When these dimensions > are captured the view must be visible, even if it is about to become > invisible. Hence, the common denominator is that these apply to open views > and can be cleared when the view is closed. > > So, the initial idea was to reset these in a "close" method on the view, > which would basically do this and also call "clearNavigation". Where we call > "clearNavigation" on teardown we would call "close" instead, at the same > time as the view is removed from the "openViews" stack. The > "clearNavigation" method would still exist separately for the mouse move > case. > > This is more correct but doesn't have an effect apart from clearing two > numeric properties that we don't use anyways. Let me know if you think I > should implement this cleanup now, maybe just for clarity, or for the moment > a comment on the method would be enough. Well, we also use the knownHeight/Width for the 'next' panelview when reverse'ing, in which case it's theoretically possible for that size to have changed while the view wasn't visible, but we don't re-fetch the size at that point nor will it have been cleared from the last time. So I'm not sure I agree that there's "no effect", or that we can live with leaving the resetting until closing...

(In reply to :Paolo Amadini from comment #17) > (In reply to :Gijs (lower availability until Jan 27) from comment #13) > > r=me, but perhaps we can think of a better name for the width thing? Maybe > > `containerMinMaxWidth` or `panelConstrainedWidth` or something? > > Technically this applies to the individual view node only, and in the future > we may even want to avoid applying this to some subviews, for instance > WebExtensions panels, supporting a width transition for them as well. I don't understand. The width/height is explicitly set on `_viewContainer`, which is part of the panelmultiview's anon content, right? It's not specific to individual panelviews...

(In reply to :Gijs (lower availability until Jan 27) from comment #21) > should have the same height, because we shouldn't shrink when going to the 5 > rows view. I guess when I looked at the code last night it wasn't clear to > me that this was the case. Is it? And if so, can you elaborate on why that > is? Yes, this is how it works. The key part is that the minHeight set on the viewContainer and the offscreenViewStack is always the height of the older of the two views, which can be the visible one we just measured or the invisible one we measured earlier. This height is already influenced by all the views that are older than it. > If the heights are per-panelview, then at least in theory this could > lead to odd cases when doing e.g.: > > 1. open hamburger panel > 2. open library view from there > 3. while that's open, directly click the library button to re-open the > library view there. For the main view, this works because we clear the minHeight of the viewContainer when closing the panel, and we don't invoke the transition code when opening the main view. When opening subviews, we have just captured the height of the older view, which is the relevant one. There is probably an edge case when navigating backwards to a view from which elements were removed, leaving some extra space. We could reset the minHeight in the _autoResizeWorkaroundTimer to work around it, but ideally we should re-measure the view off-screen also when navigating backwards. I'm not sure it's worth doing though.

(In reply to :Gijs (lower availability until Jan 27) from comment #22) > Shouldn't we queue this for when the current transition is finishing? > No-op'ing here feels strange, and I don't immediately see from the patch > context here that we already do this... During a transition, there is an ambiguity as to what the "current" view is. Let's say we invoke this programmatically in the main menu, so we see this incoming call: showSubView "More" Then, while the transition is in progress, we see one of these two incoming calls: showSubView "Text Encoding" showSubView "Web Developer" We should clearly do two different things, but given that the API is unable to express the difference at the moment, it seems to me that the no-op is the only safe option, and as far as I know it doesn't break current practices in the code base. (We could infer something from the "anchor" argument, but it is optional, and it would be a heuristic at best. I'm not sure fixing the API is in scope here.) > More generally this feels scary because it'll break navigation in the panels > if the transition doesn't clean up properly, and given that that depends on > the right events firing that feels too fragile. So, this is reset on "popuphidden". It seems to me that it's not too bad a situation that if something fails unexpectedly or takes too long you should close and reopen the panel. The case I'm thinking about is a ViewShowing event that takes a few seconds, and it seems better to prevent navigation during that time than ending up with a broken panel appearance... If you think in general we should do something different for this case, let me know. I'll likely be able to make this more robust by the end bug 1428839, as well as change the logic more easily if needed. If you're still not convinced of this intermediate state, maybe we should leave this aside for now and put an r+ on it once you've seen the later changesets? > ::: browser/components/customizableui/PanelMultiView.jsm:490 > (Diff revision 1) > > + // Close the view that was not displayed, considering that the panel > > + // may have been hidden in the meantime. > > + if (this.openViews.length) { > > + this.openViews.pop(); > > + } > > Technically this can happen when reversing, and then popping the last > openView is wrong, I think? Not sure what we should do in that case, tbh... Uh, good point, should be fixed. > If going back fails through a ViewShowing cancel, we should probably close > the panel and we don't do that today... perhaps we should add that? I'm not sure. Even when moving forward, the cancellation does not provide any user feedback, which I think is sub-optimal anyways. It is my understanding that cancellation is already reserved to exceptional situations that should not normally happen.

(In reply to :Paolo Amadini from comment #26) > It is my > understanding that cancellation is already reserved to exceptional > situations that should not normally happen. IIRC webextension browseractions (buttons) pretend to always have a subview (they're type=view widgets as far as CUI is concerned) and use the cancel here to prevent showing the panel if there isn't a panel to show at the point where the user clicks it. The add-on can change whether or not the action (button) has an associated panel at any point in time, at runtime.

(In reply to :Gijs (lower availability until Jan 27) from comment #27) > IIRC webextension browseractions (buttons) pretend to always have a subview > (they're type=view widgets as far as CUI is concerned) and use the cancel > here to prevent showing the panel if there isn't a panel to show at the > point where the user clicks it. The add-on can change whether or not the > action (button) has an associated panel at any point in time, at runtime. Ah, interesting. As far as I know this can only work when this is a subview, it doesn't work for main views. For what it's worth, it could make sense to check this in the calling code in all cases.

To clarify comment 26, I think the current implementation has various race conditions that may surface in tests, but it kind of works in production by blocking mouse events during transitions and relying on user actions taking more than a few milliseconds. I've not tested slow subview opening, but my assumption is that it's quite broken already, but again not many users have the time to start another action from a different panel. The idea here is to make it less broken, which will allow to fix it for good later.

(In reply to :Gijs (lower availability until Jan 27) from comment #23) > Well, we also use the knownHeight/Width for the 'next' panelview when > reverse'ing, in which case it's theoretically possible for that size to have > changed while the view wasn't visible, but we don't re-fetch the size at > that point nor will it have been cleared from the last time. So I'm not sure > I agree that there's "no effect", or that we can live with leaving the > resetting until closing... Yeah, I mentioned this in another comment. I guess I'm just thinking separately about the lifetime of the cached values and about what we can do with them. For proper animation without jumping, we need to either use the known value or measure it off-screen. I would say we can live with the known height for now. Do you still think we should re-measure the views when going back instead? Also, note that we would probably want to avoid re-measuring all the older views, which would be necessary if we wanted something completely correct. This is what the XUL stack layout we used to have in the non-Photon version would have done, keeping all views into account in real time. (In reply to :Gijs (lower availability until Jan 27) from comment #24) > I don't understand. The width/height is explicitly set on `_viewContainer`, Are we talking about: set minMaxWidthPx(value) { let style = this.node.style; if (value) { style.minWidth = style.maxWidth = value + "px"; } else { style.removeProperty("min-width"); style.removeProperty("max-width"); } } "this.node" is the <panelview> element.

(In reply to :Paolo Amadini from comment #30) > (In reply to :Gijs (lower availability until Jan 27) from comment #23) > > Well, we also use the knownHeight/Width for the 'next' panelview when > > reverse'ing, in which case it's theoretically possible for that size to have > > changed while the view wasn't visible, but we don't re-fetch the size at > > that point nor will it have been cleared from the last time. So I'm not sure > > I agree that there's "no effect", or that we can live with leaving the > > resetting until closing... > > Yeah, I mentioned this in another comment. I guess I'm just thinking > separately about the lifetime of the cached values and about what we can do > with them. > > For proper animation without jumping, we need to either use the known value > or measure it off-screen. I would say we can live with the known height for > now. Do you still think we should re-measure the views when going back > instead? We can file a follow-up for it, I guess, if this is the only case where this is not correct. We re-measure going forward or for the new main view after popup closure, so I guess that means right now, going back is the only edgecase. It feels a bit fragile, to have numbers hanging around "until they get overwritten and let's assume people don't use them when they shouldn't", but we can live with it for now. > (In reply to :Gijs (lower availability until Jan 27) from comment #24) > > I don't understand. The width/height is explicitly set on `_viewContainer`, > > Are we talking about: > > set minMaxWidthPx(value) { > let style = this.node.style; > if (value) { > style.minWidth = style.maxWidth = value + "px"; > } else { > style.removeProperty("min-width"); > style.removeProperty("max-width"); > } > } > > "this.node" is the <panelview> element. Ah, sorry, you're right. I got confused with the other dimensions we're storing (knownHeight/Width)... (In reply to :Paolo Amadini from comment #26) > (In reply to :Gijs (lower availability until Jan 27) from comment #22) > > Shouldn't we queue this for when the current transition is finishing? > > No-op'ing here feels strange, and I don't immediately see from the patch > > context here that we already do this... > > During a transition, there is an ambiguity as to what the "current" view is. > Let's say we invoke this programmatically in the main menu, so we see this > incoming call: > > showSubView "More" > > Then, while the transition is in progress, we see one of these two incoming > calls: > > showSubView "Text Encoding" > showSubView "Web Developer" > > We should clearly do two different things, but given that the API is unable > to express the difference at the moment, it seems to me that the no-op is > the only safe option, and as far as I know it doesn't break current > practices in the code base. > > (We could infer something from the "anchor" argument, but it is optional, > and it would be a heuristic at best. I'm not sure fixing the API is in scope > here.) > > > More generally this feels scary because it'll break navigation in the panels > > if the transition doesn't clean up properly, and given that that depends on > > the right events firing that feels too fragile. > > So, this is reset on "popuphidden". It seems to me that it's not too bad a > situation that if something fails unexpectedly or takes too long you should > close and reopen the panel. The case I'm thinking about is a ViewShowing > event that takes a few seconds, and it seems better to prevent navigation > during that time than ending up with a broken panel appearance... I think we should have a timeout and/or listeners for transitioncancel if we don't already, and perhaps a .catch() for exceptions or whatever, that resets _transitioning and just forces the visible view to match the desired-current view. > If you think in general we should do something different for this case, let > me know. I'll likely be able to make this more robust by the end bug > 1428839, as well as change the logic more easily if needed. If you're still > not convinced of this intermediate state, maybe we should leave this aside > for now and put an r+ on it once you've seen the later changesets? I'm happy to keep reviewing, but I'll note that I am uncomfortable that the approach is basically: - rewrite everything to do it "right" - in the process, fix bits we notice are broken - hopefully by the end all the current issues are fixed. This doesn't uplift well. We're already at 12 or so csets and there's no way relman will take that to beta 4 weeks from now, we're not done yet, and I am not confident that we'll have finished writing + revieweing things, landed things, worked out the inevitable regressions that a major refactoring like this always involves, and then requested approval within the next week or two (which I think is basically our window for uplift of something of this size to 59 - if it happens at all). Given this, and given :heycam is not around anymore, I would really prefer to focus on the (in my view) immediate regressions here and in bug 1424823 et al. without requiring the entire refactoring, in a way that lets us fix them for 59. This particular bug might be fix-optional for 59 (though that also makes me sad) but the other bug(s) really isn't, and there's no progress there. Like, I don't object to refactoring per se, but I think we do need to prioritize the immediate issues we're having.

(In reply to :Gijs (lower availability until Jan 27) from comment #31) > Given this, and given :heycam is not around anymore, I would really prefer > to focus on the (in my view) immediate regressions here and in bug 1424823 > et al. without requiring the entire refactoring, in a way that lets us fix > them for 59. I was thinking of backing out bug 1392340 from Beta, since the issue it fixes is not as visible as the one in this bug, and additionally it causes bug 1424823. What happens there is a platform issue and we know it was surfaced by bug 1392340, but we still don't know why. Since we may expect slower turnaround on understanding the actual reason, it may be safer to back out the offending change.

(In reply to :Paolo Amadini from comment #32) > (In reply to :Gijs (lower availability until Jan 27) from comment #31) > > Given this, and given :heycam is not around anymore, I would really prefer > > to focus on the (in my view) immediate regressions here and in bug 1424823 > > et al. without requiring the entire refactoring, in a way that lets us fix > > them for 59. > > I was thinking of backing out bug 1392340 from Beta, since the issue it > fixes is not as visible as the one in this bug, and additionally it causes > bug 1424823. What happens there is a platform issue and we know it was > surfaced by bug 1392340, but we still don't know why. Since we may expect > slower turnaround on understanding the actual reason, it may be safer to > back out the offending change. That'd wfm.

(In reply to :Paolo Amadini from comment #28) > (In reply to :Gijs (lower availability until Jan 27) from comment #27) > > IIRC webextension browseractions (buttons) pretend to always have a subview > > (they're type=view widgets as far as CUI is concerned) and use the cancel > > here to prevent showing the panel if there isn't a panel to show at the > > point where the user clicks it. The add-on can change whether or not the > > action (button) has an associated panel at any point in time, at runtime. > > Ah, interesting. As far as I know this can only work when this is a subview, > it doesn't work for main views. For what it's worth, it could make sense to > check this in the calling code in all cases. So... since the ViewShowing event for browser actions cannot prevent the panel from opening, the browser actions asks CustomizableUI to close the panel right after the event has been canceled. This obviously has to happen synchronously with the panel opening event, and means that "panelhidden" will frequently be called while we are in the middle of the showSubView function. There will be more work required to make Part 2 behave properly in all these cases. I've just checked that this part can be taken out of this patch series and moved to a later bug.

Fx59b5+ are no longer affected via the backout in bug 1433143.

Comment on attachment 8944745 [details] Bug 1424264 - Part 3 - Use the minimum height from the older transitioning view. https://reviewboard.mozilla.org/r/214896/#review221968 ::: browser/components/customizableui/PanelMultiView.jsm:761 (Diff revision 2) > // Clear the main view size caches. The dimensions could be different > // when the popup is opened again, e.g. through touch mode sizing. > - this._mainViewHeight = 0; > this._viewContainer.style.removeProperty("min-height"); > this._viewStack.style.removeProperty("max-height"); > - this._viewContainer.style.removeProperty("min-width"); > + this._viewContainer.style.removeProperty("width"); Technically the width change here seems unrelated? Maybe it's supposed to be in a different patch?

Note that there was some discussion on slack (#photon-structure) with Aaron and Stephen about the desired sizing behaviour. I... am still not sure what they want, though it seems they might not want what these patches do.

(In reply to :Gijs from comment #40) > Technically the width change here seems unrelated? Maybe it's supposed to be > in a different patch? Hm, should probably add this to the commit message. This code was likely a leftover from some previous change, because we were clearing properties we did't set anywhere. (In reply to :Gijs from comment #41) > Note that there was some discussion on slack (#photon-structure) with Aaron > and Stephen about the desired sizing behaviour. I... am still not sure what > they want, though it seems they might not want what these patches do. Do you think we can still land this version and make any changes when they get back to us?

(In reply to :Paolo Amadini from comment #42) > (In reply to :Gijs from comment #41) > > Note that there was some discussion on slack (#photon-structure) with Aaron > > and Stephen about the desired sizing behaviour. I... am still not sure what > > they want, though it seems they might not want what these patches do. > > Do you think we can still land this version and make any changes when they > get back to us? I guess that works. Let's ping them here too, for good measure... Aaron/Stephen: it's still unclear to me (and I think Paolo) how these menus are supposed to size, height-wise, both in the cases where there are only 2 views (the first thing you see, and then a subview) and when there are more (like 3 or 4) and if/when shrinking the height is (ever) acceptable. Right now, we take the height of the first thing you see, and never shrink below that, even on the third/fourth/... time you go through to another view. So if a view is really small, it'll never have "just" the height needed for it, it'll have the height needed for the very first view that opened. This can be odd if, going through views A->B->C, view A is "medium" size, B is tall, and C is tiny, because we do shrink from B but not to C's size, only to A's size. In practice, this happens (for instance) with the bookmarks tools subview in the bookmarks subview of the library. With Paolo's patch, the behaviour is much simpler in that, when not moving backwards, we only ever increase the size of the view, never decrease it. So if B is really tall, then C will simply be just as tall. Please let us know what behaviour you actually want.

Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/a11ab6e2c8c8 Part 1 - Always update min-width and max-width before showing views. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4769682570 Part 2 - Move state related to view sizing to the PanelView class. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/15e5dcaa8257 Part 3 - Use the minimum height from the older transitioning view. r=Gijs

https://hg.mozilla.org/mozilla-central/rev/a11ab6e2c8c8 https://hg.mozilla.org/mozilla-central/rev/0d4769682570 https://hg.mozilla.org/mozilla-central/rev/15e5dcaa8257

Build ID: 20180209005628 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 Verified as fixed on Firefox Nightly 60.0a1 on Windows 10 x 64, Windows 7 x32 and Mac OS X 10.12. I observed that the behavior is a bit different on Ubuntu 16.04 comparing it with Windows or Mac, as you can see in the screenshot attached, the height is not the same. Should we consider this an issue?

(In reply to Hani Yacoub from comment #46) > I observed that the behavior is a bit different on Ubuntu 16.04 comparing it > with Windows or Mac, as you can see in the screenshot attached, the height > is not the same. > Should we consider this an issue? Is the height shrinking on Ubuntu, or are there just less items in the previous view?

The height is shrinking on Ubuntu, the numbers of items are the same.

Ok, then it's definitely a bug. Thanks for finding it!

No problem, I'll log a bug for this then.

Logged bug 1437033.