Blake Winton (:bwinton) (:☕️) Reporter
	Description • 7 years ago

From bug 1384692:
> One bug is that the height jumps if you open subviews of different heights.
> You can try this by downloading an uncommon and a malware download and opening
> the two subviews. This should probably be addressed in a separate bug because
> it's likely an issue with the panelmultiview code.

I feel like I've seen a few bugs along these lines, so it might be a dupe, but it sounds like it's worth filing just in case.  :)

Thanks,
Blake.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 1 • 7 years ago

Is this fixed with the new photonpanelmultiview animation?

	Mike de Boer [:mikedeboer]
	Comment 2 • 7 years ago

I think so... Blake?

	Blake Winton (:bwinton) (:☕️) Reporter
	Comment 3 • 7 years ago

I'm not sure… it was Paolo who first mentioned it, so let's let him weigh in (when he gets back). ;)

	Mike de Boer [:mikedeboer]
	Comment 4 • 7 years ago

Well, it was Paolo who reviewed most of the code of the new panel animation, so I pretty much think he's on board when I say 'fixed'.

	:Paolo Amadini Assignee
	Comment 5 • 7 years ago

I'd have expected this to be fixed, but it is still reproducible with the steps from comment 0.

	:Paolo Amadini Assignee
	Comment 7 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d7d1501f7538f2630cf843c0bc0b0dada0e0d73

	:Gijs (he/him)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8927310 [details]
Bug 1392340 - Height jumps when opening subviews of different lengths.

https://reviewboard.mozilla.org/r/198640/#review205676

I haven't tested this, but I have 2 substantial questions so I'll ask these now rather than making you wait until Monday...

::: browser/components/customizableui/PanelMultiView.jsm:734
(Diff revision 1)
>      // shown nodes in a XUL parent node.
>      this._viewStack.style.transition = "transform var(--animation-easing-function)" +
>        " var(--panelui-subview-transition-duration)";
>      this._viewStack.style.willChange = "transform";
> -    deepestNode.style.borderInlineStart = "1px solid var(--panel-separator-color)";
> +    // Use an outline instead of a border so that the size is not affected.
> +    deepestNode.style.outline = "1px solid var(--panel-separator-color)";

This is kind of sad because it also adds the border on the top, bottom and 'end' side of the subview. Can we avoid this? How important is this part of the change? Can we avoid the resize issue here some other way, e.g. by setting a different box-sizing on the view?

::: browser/components/customizableui/PanelMultiView.jsm:1440
(Diff revision 1)
> +    // We now read the computed style to store the height of any element that
> +    // may contain wrapping text.
> +    await BrowserUtils.promiseLayoutFlushed(this.document, "layout", () => {
> +      for (let item of items) {
> +        item.bounds = this._dwu.getBoundsWithoutFlushing(item.element);
> +      }
> +    });

What's the guarantee this actually happens "soon" if the items array is empty? The height removal will not have actually done anything (because there was nothing to remove height from) and as a result, I believe the next layout flush would occur when "something else" next changes layout, which could be significantly longer than a single tick.

In other words, I think we need an early return if items is an empty array after the first layout flush.

	:Paolo Amadini Assignee
	Comment 9 • 7 years ago

(In reply to :Gijs from comment #8)
> > +    deepestNode.style.outline = "1px solid var(--panel-separator-color)";
> 
> This is kind of sad because it also adds the border on the top, bottom and
> 'end' side of the subview. Can we avoid this?

We may be able to use box-shadow, if you think having the outline on the other sides is an issue. At least on Mac, the outline on the other sides isn't noticeable. Even the separation between the views is barely visible, as the animation is quite fast. Maybe we could remove it entirely?

> How important is this part of the change?

Quite important, as one pixel may cause the text to wrap differently.

> Can we avoid the resize issue here some other way, e.g. by
> setting a different box-sizing on the view?

I already looked into this and I don't think box-sizing will help here. We may be able to set different sizes before and after the transition, but then we have to be careful and prevent the contents from shifting by one pixel at the end... overall, it seems to me this is a more complex solution.

> ::: browser/components/customizableui/PanelMultiView.jsm:1440
> (Diff revision 1)
> > +    // We now read the computed style to store the height of any element that
> > +    // may contain wrapping text.
> > +    await BrowserUtils.promiseLayoutFlushed(this.document, "layout", () => {
> > +      for (let item of items) {
> > +        item.bounds = this._dwu.getBoundsWithoutFlushing(item.element);
> > +      }
> > +    });
> 
> What's the guarantee this actually happens "soon" if the items array is
> empty?

As far as I can tell from the implementation, promiseLayoutFlushed checks the state of the document before waiting, so it resolves immediately if there have been no changes that require layout. Otherwise, the function would be pretty dangerous in general.

So, I don't think the early return is needed, though we may consider it as an optimization.

	:Gijs (he/him)
	Comment 10 • 7 years ago

I mean, I don't understand why we can't just switch the box-sizing throughout, but I agree there's no visual impact. However, I now found a different issue...

	:Gijs (he/him)
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8927310 [details]
Bug 1392340 - Height jumps when opening subviews of different lengths.

https://reviewboard.mozilla.org/r/198640/#review206870

::: browser/components/customizableui/PanelMultiView.jsm:678
(Diff revision 1)
> +      // Use the cached size when going back to a previous view, but not when
> +      // reopening a subview, because its contents may have changed.

This (ie not caching the height anymore) is making the height jump when opening e.g. the bookmarks menu view from the overflow menu worse -- that is, the animation is OK right now for every time you open the subview except the first one, but this patch changes things so that now it's just wrong every time. Is there some way we can avoid doing that, or are you fixing this elsewhere already? I mean, it's this or the descriptionheightworkaround becoming async - it's hard to tell. :-\

	:Paolo Amadini Assignee
	Comment 12 • 7 years ago

I noticed this recently while testing other changes. It may be an issue with the panel code or with the subviews being displayed before all the elements are ready. This bug is already blocked on fixing other race conditions in bug 1416498, so I'll look into this issue as well.

	:Paolo Amadini Assignee
	Comment 14 • 7 years ago

This still depends on fixing bug 1420945, but probably doesn't depend on bug 1416498 anymore as I moved the asynchronous description height workaround to bug 1420939.

	:Paolo Amadini Assignee
	Comment 15 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c35a1331fb0381dbecc0ddb66c72c7d350c096f6

	:Gijs (he/him)
	Comment 17 • 7 years ago

(In reply to :Paolo Amadini from comment #12)
> I noticed this recently while testing other changes. It may be an issue with
> the panel code or with the subviews being displayed before all the elements
> are ready. This bug is already blocked on fixing other race conditions in
> bug 1416498, so I'll look into this issue as well.

I don't really understand - you removed that bug from the list of blocking bugs here. Can you clarify?

	:Paolo Amadini Assignee
	Comment 18 • 7 years ago

This isn't true anymore as of comment 14.

	:Gijs (he/him)
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8927310 [details]
Bug 1392340 - Height jumps when opening subviews of different lengths.

https://reviewboard.mozilla.org/r/198640/#review208990

I think this works, though I'm still seeing bug 1400669.

	:Paolo Amadini Assignee
	Comment 21 • 7 years ago

(In reply to :Gijs from comment #19)
> I think this works, though I'm still seeing bug 1400669.

The fix for this was lost in the merge when the related code moved to a different function.

	:Paolo Amadini Assignee
	Comment 23 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5546068400583904838f4d3b4ffa87e94318cb1

	Pulsebot
	Comment 24 • 7 years ago

Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35425d2a0d90
Height jumps when opening subviews of different lengths. r=Gijs

	Andrei Ciure[:aciure]
	Comment 25 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/35425d2a0d90

	:Gijs (he/him)
	Comment 26 • 7 years ago

This got backed out of 59 for regressions.