Closed Bug 1369339 Opened 3 years ago Closed 3 years ago

Identity popup subview is cut off

Categories

(Firefox :: Site Identity, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- verified

People

(Reporter: johannh, Assigned: mikedeboer)

References

Details

(Keywords: regression)

Attachments

(2 files)

(In reply to Johann Hofmann [:johannh] from comment #0)
> Mike, could this be from bug 1364738?

Yes. I'll look into it.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Thanks!
Blocks: 1364738
Attached image cut-screenshot.jpg
Erm, Loic, you might wanna use a different domain as example next time :-D

Perhaps it's worse that I know what it's about...
Comment on attachment 8873397 [details]
Bug 1369339 - make sure to flush layout when necessary for the description height workaround in panelviews.

https://reviewboard.mozilla.org/r/144824/#review148776

::: commit-message-5af91:3
(Diff revision 1)
> +Because before this event, `dwu.getBoundsWithoutFlushing()` reports a width and
> +height of '0' and the description element will be inadevertently skipped.

Deferring the change would probably break the subview animation. Instead we have to force a synchronous layout, which is what we did before the changes landed.

You may want to audit other call sites too to see if that's needed.
Attachment #8873397 - Flags: review?(paolo.mozmail)
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> Erm, Loic, you might wanna use a different domain as example next time :-D
It was just a domain I tested with Nightly for another bug report and I saw this bug. :D
(In reply to :Paolo Amadini from comment #6)
> Deferring the change would probably break the subview animation. Instead we
> have to force a synchronous layout, which is what we did before the changes
> landed.

Why force a flush when it's absolutely unnecessary? I'd rather wait in line properly than flushing all the time, just like the photon-style views do.

> You may want to audit other call sites too to see if that's needed.

I did and it's not necessary for the others.
Flags: needinfo?(paolo.mozmail)
(In reply to Mike de Boer [:mikedeboer] from comment #8)
> (In reply to :Paolo Amadini from comment #6)
> > Deferring the change would probably break the subview animation. Instead we
> > have to force a synchronous layout, which is what we did before the changes
> > landed.
> 
> Why force a flush when it's absolutely unnecessary? I'd rather wait in line
> properly than flushing all the time, just like the photon-style views do.

Unfortunately this is necessary. To see the issue with deferring the operation, try restarting the browser and opening the security subview of the Control Center on mozilla.org. The reason why the subview jumps is that _transitionHeight needs to know the final height of the subview synchronously.

The right fix is obviously bug 1293242.

The second time you open the subview you won't see the issue anymore, because we've already locked the heights of the description elements. For the same reason, even with the right synchronous approach this wouldn't incur in a synchronous layout again if the text doesn't change.
Depends on: 1293242
Flags: needinfo?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #9)
> (In reply to Mike de Boer [:mikedeboer] from comment #8)
> > (In reply to :Paolo Amadini from comment #6)
> > > Deferring the change would probably break the subview animation. Instead we
> > > have to force a synchronous layout, which is what we did before the changes
> > > landed.
> > 
> > Why force a flush when it's absolutely unnecessary? I'd rather wait in line
> > properly than flushing all the time, just like the photon-style views do.
> 
> Unfortunately this is necessary. To see the issue with deferring the
> operation, try restarting the browser and opening the security subview of
> the Control Center on mozilla.org. The reason why the subview jumps is that
> _transitionHeight needs to know the final height of the subview
> synchronously.

That I see as well, but this basically means that we mustn't apply the workaround _after_ the transition, but before it.
(In reply to Mike de Boer [:mikedeboer] from comment #10)
> this basically means that we mustn't apply the
> workaround _after_ the transition, but before it.

...right, which is exactly what _transitionHeight does and is one of the reasons it needs a synchronous layout.

(We might be able to remove this requirement by preparing the subview somewhere else, but I think it's out of scope for this bug. Once we've done that, we've basically unified the Photon and non-Photon paths.)
Duplicate of this bug: 1370266
Blocks: 1369971
Bug 1364738 also caused bug 1369971. In that instance we have to retrieve the height of the description element synchronously, forcing a layout to occur.

I think changing the descriptionHeightWorkaround to try and read sizes without triggering a synchronous layout was a mistake. In cases where we can do that, the caller should wait for MozAfterPaint before calling the function, but in all other cases we should just accept that we should fix the underlying layout bug so the workaround is not needed anymore, and until then use the original version.

I prefer having some extra synchronous layouts than an ineffective workaround that still causes subtle bugs. If we want, in another bug, we can implement a separate and fully asynchronous version that we can use for preparing subviews while they are out of sight. There, we can use MozAfterPaint how many times we want, even inside the function.

Mike?
Flags: needinfo?(mdeboer)
(In reply to :Paolo Amadini from comment #13)
> I prefer having some extra synchronous layouts than an ineffective
> workaround that still causes subtle bugs. If we want, in another bug, we can
> implement a separate and fully asynchronous version that we can use for
> preparing subviews while they are out of sight. There, we can use
> MozAfterPaint how many times we want, even inside the function.

I agree, but I think I'd prefer doing the offscreen version now rather than later, because the platform changes are never going to happen.
I'll try to ping you tomorrow to briefly discuss a plan.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> I agree, but I think I'd prefer doing the offscreen version now rather than
> later, because the platform changes are never going to happen.

Agreed, but we should not block reverting to the synchronous version of the workaround on having implemented the asynchronous version. This would fix the existing regressions before the Beta merge, and we can land the efficient reworking of the subviews at the beginning of next cycle.
Comment on attachment 8873397 [details]
Bug 1369339 - make sure to flush layout when necessary for the description height workaround in panelviews.

https://reviewboard.mozilla.org/r/144824/#review150610

We should replace getBoundsWithoutFlushing with getBoundingClientRect in order to fix bug 1369971. We can move the "height" property removal to a separate loop if we want to optimize further.

I think caching the bounds doesn't hurt, and we can keep that, or is it an issue for other reasons?
Attachment #8873397 - Flags: review?(paolo.mozmail)
It's just that when I removed the caching, I did not see the issues mentioned here anymore. So I believe our mistake was in fact there.
We only use the unflushed-bounds for the 0-check, which is not why the issues exist. As far as I can tell...
Comment on attachment 8873397 [details]
Bug 1369339 - make sure to flush layout when necessary for the description height workaround in panelviews.

https://reviewboard.mozilla.org/r/144824/#review150664

Thanks!

::: commit-message-5af91:1
(Diff revision 3)
> +Bug 1369339 - make sure the flush layout when necessary for the description height workaround in panelviews. r?paolo

the => to

::: browser/components/customizableui/PanelMultiView.jsm:1022
(Diff revision 3)
> +    // Remove the 'height' property, which will cause a layout flush in the next
> +    // loop below if it was set.

Removing the 'height' property will only cause...
Attachment #8873397 - Flags: review?(paolo.mozmail) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f14337def2d
make sure to flush layout when necessary for the description height workaround in panelviews. r=Paolo
Hey mconley, just a heads up as I think you're preparing tests for synchronous reflows. In some cases these reflows are conditional, for example in the descriptionHeightWorkaround here they won't happen if nothing has changed since the last time the main menu panel was opened, or if there is no description element visible. Have you considered this possibility in general, so the tests don't become intermittent based on whether previous tests already interacted with the UI area?

We'll work on an asynchronous version when opening subviews that are out of view, but elements on the main view and DOM changes on visible views all require the synchronous version instead. (This is slightly related to bug 1296442 and us being unable to layout the panel contents asynchronously while the panel is still hidden.)
Flags: needinfo?(mconley)
(In reply to :Paolo Amadini from comment #23)
> Have you considered this possibility in
> general, so the tests don't become intermittent based on whether previous
> tests already interacted with the UI area?

Yes. I attempt to dirty the DOM before every JS event is serviced by using an all-events listener. There are still some edge-cases though: reflows that are triggered within setTimeout, setInterval, requestIdleCallback, and requestAnimationFrame _might_ not trigger a reflow at times. Bug 1368132 was filed for that.
Flags: needinfo?(mconley)
https://hg.mozilla.org/mozilla-central/rev/2f14337def2d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Duplicate of this bug: 1369971
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.