Closed Bug 1593060 Opened 5 years ago Closed 4 years ago

WebPage Language Settings and Firefox Language Settings sub dialogs glitch (popup dropdown from the "Select a language to add" menulist is confused about its size vs. position)

Categories

(Firefox :: Settings UI, defect, P1)

71 Branch
Desktop
Windows 10
defect

Tracking

()

RESOLVED FIXED
Firefox 73
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 + verified
firefox72 + fixed
firefox73 --- verified

People

(Reporter: alice0775, Assigned: dholbert)

References

(Regressed 2 open bugs, Regression)

Details

(Keywords: nightly-community, regression)

Attachments

(4 files)

Reproducible : always

Steps to reproduce:

  1. Open Options
  2. Click [Choose...] button of Language section
  3. Click "Select a language to add" so that language list options will pops up.

Actual results:
Language list popup shift up. So, there is gap between select button and optioons popup.
See attached screenshot

Expected results:
And the popup should not shift up. There should not be gap.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=922881dd399a94bda9635be2745af79fa8878238&tochange=aefedc124f9a4ad6598a6757fe0e3c5c688efa06

Regressed by:
aefedc124f9a4ad6598a6757fe0e3c5c688efa06 Tim Nguyen — Bug 1579516 - Remove XUL grid from browser/components/preferences/languages.xul. r=jaws

[Tracking Requested - why for this release]:
Unshipped user-visible regression.

:ntim, can you take a look? If not, please can you ping me back so we look at this before we ship it? Thanks.

Flags: needinfo?(ntim.bugs)

I don't have a Windows machine (I can't reproduce on macOS) and I don't have time to setup a VM unfortunately.

Flags: needinfo?(ntim.bugs) → needinfo?(gijskruitbosch+bugs)

:dholbert, it looks like we're opening a popup here and it gets confused about how much room it has. I don't really know why that is, or how to debug it...

Steps that worked for me (on Windows and Mac):

  1. open prefs
  2. search for "language"
  3. click the "Choose..." button next to "Check your preferred language for displaying pages"
  4. resize the dialog to be bigger
  5. click the dropdown labeled "Select a language to add"

The popup is displayed near the top of the window, with no bottom border and a big gap between its bottom and the anchor (ie the menulist). Not sure why this would have worked before the conversion and no longer does - maybe because of the display: grid ancestor or something?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dholbert)
Summary: WebPage Language Settings sub dialog glitches → WebPage Language Settings sub dialog (popup dropdown from the "Select a language to add" menulist is confused about its size)
Summary: WebPage Language Settings sub dialog (popup dropdown from the "Select a language to add" menulist is confused about its size) → WebPage Language Settings sub dialog glitches (popup dropdown from the "Select a language to add" menulist is confused about its size vs. position)

I can take a look in the next day or so. --> Leaving needinfo toggled.

(In reply to :Gijs (he/him) from comment #3)

  1. click the "Choose..." button next to "Check your preferred language for displaying pages"
  2. resize the dialog to be bigger

(Note: step 4 here ^^^ is important to trigger the bug, on my linux system at least. With the default dialog-size, I don't see any issues.)

So the container here (the <box> with display:grid) has grid-template-rows: 1fr auto.

If I use devtools to change that to [any hardcoded pixel value] auto, then this issue seems to mostly go away[1]. So I'm guessing this may be related to content measurement, and it might be the same issue described in bug 1576946 comment 17... (and might be fixed by bug 1492582)

[1] at least, the popup seems to get the right size. I do sometimes see the scrollbar fly past the bottom of the popup and protrude a bit, which seems odd but might be a separate problem.

Nope, not fixed by bug 1492582. (Just built autoland tip, which includes bug 1492582's patches that just landed, and I can still reproduce this bug here.)

I believe the interesting code here (for positioning the menu-popup) is in nsMenuPopupFrame::ReflowFinished(), which calls SetPopupPosition() in a post-reflow callback, to place the popup based (entirely?) on the position of its "anchor frame" (the nsMenuFrame, which I believe is basically the button that you click to open the popup). It also e.g. makes sure the popup frame doesn't run offscreen, and flips it if it would.
https://searchfox.org/mozilla-central/rev/b2b0077c2e6a516a76bf8077d6f0237c58f5959a/layout/xul/nsMenuPopupFrame.cpp#611

The menu frame (the button) does end up in a a reasonable place, so you'd think the popup would as well. (But perhaps there's some XUL/non-XUL disconnect, with handling of coordinate spaces or offsets, that's introducing trouble somehow...)

Got a bit further -- ReflowFinished() seems to be producing the correct final size in the frame tree, but it doesn't update the nsView for our popup. And at this point, grid layout's ::MeasuringReflow call has left us with a smaller bogus popup-size, and that smaller bogus size was propagated through to our nsView (via nsMenuPopupFrame::LayoutPopup calling nsViewManager::ResizeView). And unfortunately we don't get another LayoutPopup/ResizeView call after that -- so the view is left with the wrong size. The frame gets the correct size via the ReflowFinished callback, but the view never gets the updated size, so it doesn't actually fill the whole area that we'd like to give it.

So: I think we need to be sure we get a call to ResizeView with the correct larger size here, either via:

  • making sure we get a LayoutPopup call after the measuring reflow.
    ...or:
  • making the ReflowFinished() callback update the size of the view (it kinda seems like it should, since it does change the size of the frame...)
Flags: needinfo?(dholbert)

(If this doesn't end up being trivially fixable, we may want to back out regressing bug 1579516, at least on beta71 if not also on Nightly.)

(In reply to Daniel Holbert [:dholbert] from comment #9)

(If this doesn't end up being trivially fixable, we may want to back out regressing bug 1579516, at least on beta71 if not also on Nightly.)

This is a sensible suggestion, definitely for beta at this point. I'll ask sheriffs to do so.

[Tracking Requested - why for this release]:
Regressor backed out of beta, so let's track this for current nightly so we don't forget. :-)

:ntim pointed out that although this was filed for the web content language dialog, the browser language dialog in bug 1581747 has more or less the same structure (once you select "Search for more languages"), and shows similar issues (though they seem less severe for me, perhaps because the list of languages is smaller and that influences the popup size, or something). I'll ask to have that backed out from 71 too...

Any layout fix would apply to both; if we end up working around in frontend CSS then we should apply the fix in both dialogs.

Regressed by: 1581747
Summary: WebPage Language Settings sub dialog glitches (popup dropdown from the "Select a language to add" menulist is confused about its size vs. position) → WebPage Language Settings and Firefox Language Settings sub dialogs glitch (popup dropdown from the "Select a language to add" menulist is confused about its size vs. position)

A platform fix as described in Comment 8 sounds better than individual frontend CSS workarounds. I'm not sure how hard those are though. Daniel, is this something you or someone on your team could help with?

We have only a few more frontend xul:grid consumers (tracked in Bug 1520625) and once we remove all of them we can remove support for the element and the associated layout code (Bug 1525737).

Flags: needinfo?(dholbert)

Thanks for requesting the backouts, Gijs!

bgrins: yeah, I'm tentatively planning on seeing this through and fixing it with a platform-level change. (I'm focusing on a different bug today, but hopefully I can close this one out next week.)

Assignee: nobody → dholbert
Status: NEW → ASSIGNED

This is a pretty nasty visual glitch. Yes, it's not our primary UI, but given how broken this looks, and the fact that it's assigned, I'm marking this P1.

Priority: -- → P1

I'm looking at this again (sorry for the wait).

It turns out that this boils down to this chunk of code (which gets invoked on behalf of the <vbox> element that is the child of the CSS Grid and the parent of the menu frame). Here, child is the nsMenuFrame for the "Select a language..." menu.

Here's the relevant code:

        if (!NS_SUBTREE_DIRTY(child)) layout = false;
[...]
      bool sizeChanged = (childRect.width != oldRect.width ||
                          childRect.height != oldRect.height);
[...]
      // If we already determined that layout was required or if our size has
      // changed, then we make sure to call layout on the child, since its
      // children may need to be shifted around as a result of the size change.
      if (layout || sizeChanged) child->XULLayout(aState);

https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/layout/xul/nsSprocketLayout.cpp#410

As a hack/demonstration: If I comment out the first line quoted above (the NS_SUBTREE_DIRTY check), then I get "expected results" here.

Here's what's going on -- when you click the menu to open it, we hit the above-quoted code twice:
(1) for the grid item "measuring reflow", i.e. inside of a css-grid ::MeasuringReflow call, at which point the child has a dirty subtree.
(2) for the grid item "actual reflow", i.e. inside of a css-grid ::ReflowInFlowChild call, at which point the child no longer has a dirty subtree because we cleared the dirty bits in (1).

During the "actual reflow" (labeled (2) above), we skip the XULLayout() call, but we do really need to make that call because the available space has changed, and the eventual size of one of our descendants (the menu popup) needs to change. The sizeChanged bool is intending to capture that sort of thing to let us still layout the child when needed, but in this case it's not helping because child is the nsMenuFrame for the menu-button, and that button's height hasn't changed, even though the available space has changed and the not-yet-visible popup needs to be resized to reflect that.

So, basically this is a "wart" from XUL-layout/modern-layout mixing together. We need to be sure that we hit the child->XULLayout(aState) call here, one way or another.

Flags: needinfo?(dholbert)

We're running out of time to address this on nightly - should we back bug 1579516 and bug 1581747 out of 72 beta, too? Can we even still do that or did the XUL grid implementation get completely removed already?

Flags: needinfo?(ntim.bugs)
Flags: needinfo?(dholbert)

The XUL grid implementation is still around, see bug 1525737, so this can be backed out from 72 beta if needed.

Leaving ni? for Daniel in case he has time to address this for 72.

Flags: needinfo?(ntim.bugs)

Yeah, I'm still looking at this (and we can safely back out if it gets too late, as we did for 71).

Flags: needinfo?(dholbert)

(Note that the GetPrefSize API might still do a "::BoxReflow" under the hood,
inside its call to RefreshSizeCache.)

The general problem here is that we've got an initial dummy reflow which the XUL Box (as a grid item) mistakenly thinks is official, and then the later official/final reflow gets disregarded by the XUL box because it thinks it hasn't changed when in fact it has (at the very least, its position has changed in the final reflow, which then influences the size of its menu descendant's menu-popup-frame).

We can get around this and perhaps get an optimization as well by changing our ::MeasuringReflow grid function so that it calls the roughly-equivalent XUL "give-me-your-preferred-size" API, in cases where we encounter a XUL frame.

This seems to work, and though it feels kind of terrible to be mixing XUL-specific code in our modern grid layout, it seems not-crazy in an intermediate world where we're intentionally having XUL items inside of CSS grid/flexbox.

I do want to write some automated testcases to ensure that this works as we expect in some corner cases & to ensure it doesn't break (since we'll be relying on it in our UI for the time being which means we should be testing it). I'm not requesting review until I've got some of those testcases, but feel free to post thoughts if you've got any, mats (I'll tag you for review when it's ready).

Flags: needinfo?(mats)

Daniel, thanks for the debugging!

This seems to work, and though it feels kind of terrible to be mixing XUL-specific code in our modern grid layout, it seems not-crazy in an intermediate world where we're intentionally having XUL items inside of CSS grid/flexbox.

Yeah, I agree it sucks, but as long as it's a temporary measure I guess I'm OK with it...

Flags: needinfo?(mats)

I added a reftest, but unfortunately it passes in current mozilla-central & fails in patched mozilla-central, so it indicates that this patch regresses things a bit. (Specifically: it looks like the grid container is discarding margins & perhaps a bit more when computing its own BSize based on the item's BSize.)

Oddly, this fixes itself if I allow the code to proceed with the traditional "measuring reflow" codepath (dropping the }else{ in the patch) and then I simply throw away the result (still returning the measurement that we got from GetXULPrefSize() for the result of the measuring reflow). So it's not simply that GetXULPrefSize() is giving us a wrong answer -- it seems to be giving us the right answer, but there's some other side effect of the measuring reflow that we need in order for the grid to size itself properly.

Flags: needinfo?(dholbert)

I suspect it's the GetLogicalUsedMargin(wm) calls in ContentContribution in nsGridContainerFrame -- I think that "Used" API depends on reflow having been performed before, so they're bogus now that I've removed the reflow that would've been performed before it now.

Specifically this call is becoming bogus:
https://searchfox.org/mozilla-central/rev/4df8821c1b824db5f40f381f48432f219d99ae36/layout/generic/nsGridContainerFrame.cpp#4868

(I can "fix" the reftest by making it perform a dynamic tweak (e.g. by adding .container:hover { border: 2px solid purple; } and then hovering each grid box to trigger a relayout, in a browser that has the reftest loaded (with about:config pref dom.allow_XUL_XBL_for_file = true so that the XUL works). I added some logging to dump the ContentContribution LogicalUsedMargin value, too, and I verified that it's 0,0,0,0 in the first runthrough (with this patch) but nonzero after dynamic tweaks when the correct rendering ends up being produced.)

We probably can add a special case for XUL there, too (maybe changing the patch's logic slightly to avoid two independent "is-this-child-XUL" checks).

I'm AFK for tomorrow (Thanksgiving!) and probably also Friday, but I'll try to set aside some time to make that change & get the test passing before the end of the weekend.

Attachment #9111861 - Attachment description: Bug 1593060: When measuring size of XUL in CSS grid, use the GetXULPrefSize API instead of an explicit reflow. → Bug 1593060: When measuring size of XUL in CSS grid, use the GetXULPrefSize API instead of an explicit reflow. r?mats
Depends on: 1600542
Blocks: 1600544

I updated the patch to fix the margin issue discussed above. The reftest passes now (locally at least).
Try run (no reftests because I messed up the fuzzy syntax):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=851211c68289a653ddecf434b28c914d2c3e7ddb
Try run with just reftests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c73e9794b381217f0bced9a4c087ada297c87b04

[Ab]using needinfo to be sure you notice the re-review request, in case phabmail isn't clear on re-review; it's an uncommon case so I'm not sure how well it notifies.

Flags: needinfo?(dholbert) → needinfo?(mats)
Comment 25 is private: false

I can still reproduce small gap see attached with the try run(Comment 25) in windows10 and Ubuntu18.04.

Thanks, Alice0775 - I can reproduce that locally and I'll take a look...

OK, so I think this is happening because reflowing the popup frame at bogus position "0,0", via the LogicalPoint(childWM) arg here (and only setting the position later on):

  ReflowChild(aChild, pc, childSize, childRI, childWM, LogicalPoint(childWM),
              dummyContainerSize, ReflowChildFlags::Default, aStatus);

https://searchfox.org/mozilla-central/rev/8bc24752246aeac8a9aed566cf1caccf88d97d11/layout/generic/nsGridContainerFrame.cpp#6438

This is supposed to be fine, because we determine the correct position and pass it in via FinishReflowChild down below -- but in fact, it ends up causing trouble because nsMenuPopupFrame determines its size (during reflow) based on its presumed position. So if we reflow it at the wrong position, we end up with a bogus size for the popup and its descendants. We do end up correcting the nsMenuPopupFrame's own size eventually, via the post-reflow callback nsMenuPopupFrame::ReflowFinished(), but that doesn't handle resizing the popup's contents -- it only resizes the popup itself (as part of repositioning it). So the contents still end up being the wrong size, which is why Alice0775's screenshot shows them being a bit too small.

The simplest solution in this case is to "guess" the correct position and pass it in to reflow -- i.e. to "guess" that the child is at the default position, which is the grid area's origin. We can do this as long as the child and the parent have the same writing mode, which is probably always the case in our XUL code. (If they did disagree on writing mode, then we woudln't be able to reliably express the origin in terms of the child's writing mode until after we've reflowed the child and know its size -- so there'd be a chicken-and-egg problem. But anyway, that's not a problem here.)

If we "guess" wrong, then it's generally fine (for the same reason that our current bogus 0,0 point is fine) except in this case of nsMenuPopupFrame which is already broken, so it'd be a low-stakes change.

So I think we should just make that additional change to the current patch.

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d39de2be1ae
When measuring size of XUL in CSS grid, use the GetXULPrefSize API instead of an explicit reflow. r=mats
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73

Daniel, is this something you want to uplift to 72? It's also possible to back out the regressing bugs from 72 if that's preferable.

Flags: needinfo?(mats) → needinfo?(dholbert)

I'd prefer backing out the regressing bugs from 72; that's lower-risk, and there's no rush on having that rewrite make it through the release trains.

Flags: needinfo?(dholbert)

(So in particular, I think we need to backout https://bugzilla.mozilla.org/show_bug.cgi?id=1581747 and https://bugzilla.mozilla.org/show_bug.cgi?id=1579516 from beta72, as we did for beta71)

Regressions: 1601950

Unfortunately this seems to have triggered a non-trivial perf regression for about:preferences itself (which was also recently rewritten to use CSS-grid-with-XUL-children at the top level, and hence could conceivably be impacted by this change, I think). See more on bug 1601950. Unfortunately, I won't be able to investigate for a while, as I'm disappearing for parental leave - sorry!

If we can't come up with a resolution for the performance regression, we might need to backout this changeset and punt on this (i.e. backout the rewrites linked in comment 34) for 73 as well (unless someone else has cycles to investigate/address the regression).

Regressions: 1602939
Blocks: 1603397
Has Regression Range: --- → yes
Regressions: 1762569
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: