Closed Bug 1392340 Opened 4 years ago Closed 4 years ago

photonpanelmultiview height jumps if you open subviews of different lengths.

Categories

(Firefox :: Menus, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: bwinton, Assigned: Paolo)

References

Details

Attachments

(1 file)

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.
Is this fixed with the new photonpanelmultiview animation?
Flags: needinfo?(mdeboer)
Priority: -- → P4
I think so... Blake?
Flags: needinfo?(mdeboer) → needinfo?(bwinton)
I'm not sure… it was Paolo who first mentioned it, so let's let him weigh in (when he gets back). ;)
Flags: needinfo?(bwinton) → needinfo?(paolo.mozmail)
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'.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(paolo.mozmail)
Resolution: --- → FIXED
I'd have expected this to be fixed, but it is still reproducible with the steps from comment 0.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: nobody → paolo.mozmail
Status: REOPENED → ASSIGNED
Priority: P4 → P1
Blocks: 1409301
Blocks: 1416231
Depends on: 1416498
No longer blocks: 1416231
Depends on: 1416231
No longer blocks: 1409301
Attachment #8927310 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
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.
Attachment #8927310 - Flags: review?(gijskruitbosch+bugs)
(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.
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...
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. :-\
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.
Blocks: 1420939
No longer depends on: 1416498
Duplicate of this bug: 1416231
Depends on: 1420945
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.
(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?
Flags: needinfo?(paolo.mozmail)
This isn't true anymore as of comment 14.
Flags: needinfo?(paolo.mozmail)
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.
Attachment #8927310 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Duplicate of this bug: 1400669
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
https://hg.mozilla.org/mozilla-central/rev/35425d2a0d90
Status: ASSIGNED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Depends on: 1424264
Depends on: 1424823
Depends on: 1433143
This got backed out of 59 for regressions.
You need to log in before you can comment on or make changes to this bug.