1369339 - Identity popup subview is cut off

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Johann Hofmann [:johannh]]

See mozscreenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=39d5cc0fda5e16c49a59d29d4ca186a5534cc88b&newProject=mozilla-central&newRev=692e277e2b9f3bb36d6d67e1166d135228f2d851&filter=controlCenter

This is the push log for the screenshots:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=39d5cc0fda5e16c49a59d29d4ca186a5534cc88b&tochange=692e277e2b9f3bb36d6d67e1166d135228f2d851

Mike, could this be from bug 1364738?

Comment 1

[Mike de Boer [:mikedeboer]]

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

Yes. I'll look into it.

Comment 2

[Johann Hofmann [:johannh]]

Thanks!

Comment 3

[Loic]

Attachment: cut-screenshot.jpg

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1369339 - make sure to flush layout when necessary for the description height workaround in panelviews.

Because before this event, `dwu.getBoundsWithoutFlushing()` reports a width and
height of '0' and the description element will be inadevertently skipped.

Review commit: https://reviewboard.mozilla.org/r/144824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144824/

Comment 5

[Mike de Boer [:mikedeboer]]

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 6

[:Paolo Amadini]

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.

Comment 7

[Loic]

(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

Comment 8

[Mike de Boer [:mikedeboer]]

(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.

Comment 9

[:Paolo Amadini]

(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.

Comment 10

[Mike de Boer [:mikedeboer]]

(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.

Comment 11

[:Paolo Amadini]

(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.)

Comment 13

[:Paolo Amadini]

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?

Comment 14

[Mike de Boer [:mikedeboer]]

(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.

Comment 15

[:Paolo Amadini]

(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 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8873397 [details]
Bug 1369339 - make sure to flush layout when necessary for the description height workaround in panelviews.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144824/diff/1-2/

Comment 17

[:Paolo Amadini]

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?

Comment 18

[Mike de Boer [:mikedeboer]]

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 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8873397 [details]
Bug 1369339 - make sure to flush layout when necessary for the description height workaround in panelviews.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144824/diff/2-3/

Comment 20

[:Paolo Amadini]

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...

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8873397 [details]
Bug 1369339 - make sure to flush layout when necessary for the description height workaround in panelviews.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144824/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f14337def2d
make sure to flush layout when necessary for the description height workaround in panelviews. r=Paolo

Comment 23

[:Paolo Amadini]

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.)

Comment 24

[Mike Conley (: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.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f14337def2d

Comment 27

[u554753]

Verified on Windows, Mac, and Ubuntu.

Comment 28

[Johann Hofmann [:johannh]]

Mozscreenshots says success!:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=7efda263a842e60cd0cc00b3c4a7058c65590702&newProject=mozilla-central&newRev=f223e1fd2044a026c740434df95f37a7f7accf48&filter=subview

Thanks everyone!