Closed
Bug 1392340
Opened 7 years ago
Closed 7 years ago
photonpanelmultiview height jumps if you open subviews of different lengths.
Categories
(Firefox :: Menus, defect, P1)
Firefox
Menus
Tracking
()
RESOLVED
FIXED
Firefox 59
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.
Comment 1•7 years ago
|
||
Is this fixed with the new photonpanelmultiview animation?
Flags: needinfo?(mdeboer)
Priority: -- → P4
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). ;)
Flags: needinfo?(bwinton) → needinfo?(paolo.mozmail)
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'.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(paolo.mozmail)
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → paolo.mozmail
Status: REOPENED → ASSIGNED
Priority: P4 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
Attachment #8927310 -
Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
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.
Attachment #8927310 -
Flags: review?(gijskruitbosch+bugs)
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.
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...
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. :-\
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.
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.
Assignee | ||
Comment 15•7 years ago
|
||
Comment hidden (mozreview-request) |
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?
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 18•7 years ago
|
||
This isn't true anymore as of comment 14.
Flags: needinfo?(paolo.mozmail)
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.
Attachment #8927310 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request) |
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.
Assignee | ||
Comment 23•7 years ago
|
||
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
Comment 25•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in
before you can comment on or make changes to this bug.
Description
•