1602939 - "Delete Profile" dialog glitch

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Alice0775 White]

Attachment: image.png

Reproducible: always

Steps To Reproduce:

1. Start firefox.exe -p
2. Try to delete an existing profile
   Click [Delete Profile] button

optionally
3. Click one of 3 button

Actual results:
Description and buttons overlap each other.
And the description covers button's clickable area.

Expected Results:
The dialog should be working properly.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=dfa7714e0543412adf6ee9e065069f9fe5d1b025&tochange=2d39de2be1ae9fa8063a19b459d8712fa90bc851

Comment 2

[Tim Nguyen :ntim]

Attachment: [alternate approach] Bug 1602939 - Only apply XUL in CSS grid layout fix on certain items.

Comment 3

[Tim Nguyen :ntim]

Attachment: Bug 1602939 - Backed out changeset 2d39de2be1ae for regressions (bug 1602939, bug 1601950). r=emilio

Comment 4

[Tim Nguyen :ntim]

Attachment: Bug 1602939 - Use a HTML <div> instead of a XUL <vbox> around preference menupopups in CSS grids. r=Gijs,bgrins

This avoids hitting both the XUL sizing code path and the CSS grid reflow code path which don't play well together

Depends on D56654

Comment 5

[Tim Nguyen :ntim]

Alice0775, can you please check if this build fixes this bug and bug 1593060? Thanks!

I've checked locally on macOS, and both bugs seem fixed on my side, but I'm not sure about Windows or Linux.

Comment 6

[Alice0775 White]

(In reply to Tim Nguyen :ntim from comment #5)

Alice0775, can you please check if this build fixes this bug and bug 1593060? Thanks!

I've checked locally on macOS, and both bugs seem fixed on my side, but I'm not sure about Windows or Linux.

The try build Windows10 and Ubuntu18.04:
this bug is fixed. However, bug 1593060 is reproduced again.

Comment 7

[Tim Nguyen :ntim]

Thanks Alice0775!

Emilio, Mats, the only solution I can think of at this point would be the alternate approach. Alternatively, we could back out all 3 patches (both XUL grid removals and dholbert’s patch), but we’d need to fix this anyway, so this would just save a bit of time for an extra cycle.

I unfortunately don’t know enough about the layout code to debug this further. Would you be OK with something like the alternative patch or do you have a bit of time to look into this?

Comment 8

[Mats Palmgren (inactive)]

So the frame tree for the grid item looks something like this:

Box(vbox) (0,0,0,0) [state=0002000080801602] <
  Block(description) (0,0,81624,3420) [state=0002000000d00000] <
    line (0,0,36243,1140) <
      Text(0)"Deleting a profile will remove the profile from th" (0,0,36243,1140)
    >
    line (0,1140,81624,1140) <
      Text(0)"You may also choose to delete the profile data fil" (0,1140,81624,1140)
    >
    line (0,2280,17066,1140) <
      Text(0)"Would you like to delete the profile data files?" (0,2280,17066,1140)
    >
  >
>

Is there some way we could get rid of the Box frame there? Setting display:contents on it perhaps?
Or could we de-XULify the prompt content in general so that it uses a regular display:block instead?
Alternatively, this seems like a pretty insignificant dialog for the product, could we just hardcode a min height on the relevant element for now?

FTR, at the time the Grid container asks for the intrinsic block-size for the above item, GetXULPrefSize returns 24px (with an availISize=600.00px) which is clearly wrong. The reason is that nsBoxFrame::GetXULPrefSize:
https://searchfox.org/mozilla-central/rev/690e903ef689a4eca335b96bd903580394864a1c/layout/xul/nsBoxFrame.cpp#547
has a width, but no height, so it ends up using the Min size instead (enforced by the BoundsCheck call).
(Also, the XUL sizes are cached so we should probably invalidate that cache before/after each GetXULPrefSize call here for good measure. That doesn't make a difference in this case though.)

Comment 9

[Tim Nguyen :ntim]

Attachment: Bug 1602939 - Make #infoContainer and #iconContainer HTML elements in commonDialog. r=dao

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(I don't think I can provide more input, I agree with Mats)

Comment 11

[Tim Nguyen :ntim]

Hi Mats, thanks for looking into it!
I've changed the grid item to use display: block, which does fix this specific issue. But is it possible to know what other cases were regressed by bug 1593060 other than testing ? There are quite a few XUL in CSS grid items now and I wonder if there are any others that are affected by a similar issue.

Comment 12

[Mats Palmgren (inactive)]

We could add an assertion perhaps, but it might be hard to detect which ones will give the "wrong" result.
We should probably just warn about all of them (bug 1600544) and then try to remove them...
Easier said than done, I know ;-)

Comment 13

[Tim Nguyen :ntim]

This landed on autoland an hour ago:
https://hg.mozilla.org/integration/autoland/rev/234701139a2a61d1262e609c9d8ac42384ecafda

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/234701139a2a

Comment 16

[Cristian Baica [:cbaica], Release Desktop QA]

Managed to reproduce the issue using Fx73.0a1 buildID: 20191210212905.
The issue is verified fixed on the latest Fx73.0RC build as well as the latest Fx74.0a1 build on windows 10 and double checked on ubuntu and macOS to make sure nothing broke there.