1420480 - regression: CSS Menus layering under other page elements.

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Jesse Roland]

Attachment: website.7z

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171123161455

Steps to reproduce:

1. Download Zipped website.7z
2. Open the main WHMCS.html file.
3. Open Firefox window to half screen or 960px approximately. (Enough for the nested menu to open over the buttons: "Today", "This Month", "This Year")
4. Go to menu tab "Support"
5. Navigate to "Support tickets"
6. If menu hovers over buttons or support right side DIV content it layers underneath instead of on top.


Actual results:

When menu hovers down the screen further the menu improperly layers under other div and button elements.
This happens in Firefox Nightly 59, but not in Firefox Beta 58b6


Expected results:

Menu should be the top layer and not display underneath other content.

Comment 1

[Jesse Roland]

Attachment: menu_bug_example.png

Comment 2

[Darkspirit]

Thank you!

(extracted website.7z, deleted scripts.js because I don't use a VM (inline scripts look ok): it still looks wrong like on the screenshot (attachment 8931752 [details]))

(top nav) Support > Support Tickets

mozregression --good 2017-09-15 --bad 2017-11-24 --pref "layers.acceleration.force-enabled:true" "gfx.webrender.enabled:true" "gfx.webrendest.enabled:true" "gfx.webrender.layers-free:true" "gfx.webrender.blob-images:true"
> [...]
> 10:55.65 INFO: Last good revision: 7fb001f811d183095e9cfdd958c9215484377396
> 10:55.65 INFO: First bad revision: d5e1ed773fefe2eb9d52402dd74b5fea19b8b4a4
> 10:55.65 INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7fb001f811d183095e9cfdd958c9215484377396&tochange=d5e1ed773fefe2eb9d52402dd74b5fea19b8b4a4

> d5e1ed773fef	Matt Woodrow — Bug 1416055 - Enable retained display lists for Nightly builds. r=miko
> 2ec68ec7ff01	Matt Woodrow — Bug 1416448 - Don't update the ASR during merging for an empty container item, since we can't compute the ASR of the contents. r=miko
> 559f4487dc65	Matt Woodrow — Bug 1413073 - Fix rebase issue that caused us to only update the container asr for reused items.

bad: mozregression --repo autoland --launch d5e1ed773fef
bad: mozregression --repo autoland --launch 2ec68ec7ff01
bad: mozregression --repo autoland --launch 559f4487dc65

This is not a WebRender bug.

-----

Confirmed in
Nightly 59 x64 20171124220317 de_DE e8ce4865e42125850e6371eba3c5e71cf5ab6d29 @ Debian Testing (KDE, Radeon RX480).
Disabling layout.display-list.retain makes this problem go away. I'm unsure whether I should block bug 1413073 or bug 1416055.

Comment 3

[Jesse Roland]

Giving this P2 priority is fine as long as layout.display-list.retain isn't going to be enabled by default in 58 Stable or 59 Beta.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1420480_RDL_checks.txt

Some analysis, but no solution.


Attaching the output of the RDL checker when opening the menu.

It shows 4 display lists: NR (Non-Retained, displays correctly), BM (Before Merge, retained list from previous paint), TM (To-be Merged, modified items), AM (After Merge, combining BM and TM, shows issue).

The problem is that the new WrapList containing the just-opened menu (TM#39) is merged near the middle (AM#159), below other elements, when it should instead appear at the end (NR#378), above all elements.

Looking at the top-level items by sections:
> BM#1-148   == TM#1-34  AM#1-148    NR#1-148
> 
>                  /---> AM#159 :-(
> BM#159-373 != TM#39    AM#200-414  NR#159-373
>                  \~~~~~~~~~~~~~~~> NR#378 :-)
> 
> BM#378     == TM#80    AM#419      NR#419

The first and last sections match, items from BM get replaced by modified items from AM, all is well.
But in the middle, BM#159-373 and TM#39 don't match, and the MergeDisplayLists algorithm picks items from the new list (TM) first, which is why it ends up before other elements and gets covered.


I'm afraid I don't know enough to propose a solution here, but hopefully the above information will help others to figure something out -- Good luck!
(I doubt the solution would be to "just" modify the algorithm to reverse the placement of new vs old items, as there could very well be other situations where this would be the wrong choice.)

Comment 5

[Matt Woodrow (:mattwoodrow)]

The merging code can't figure out the right ordering between two arbitrary items (it requires know which frame would be reached first during a depth first search, which I haven't figured out how to do cheaply).

If the ordering between an item in the new list, and an item in the old list matters (i.e they intersect), then we need to have that item in the new list too.

By the sounds of your analysis, the item that obscured the menu wasn't in the new list, so that suggests that bug is in ComputeRebuildRegion (the code that decides what needs to be in the new list).

Comment 6

[Matt Woodrow (:mattwoodrow)]

Opening the first menu inserts a new stacking context for the menu items, which gets sorted correctly wrt things that it intersects (like the System Overview panel), but not things that it doesn't (like the Support panel).

We don't worry about the ordering between non intersecting things (since it doesn't matter), and in this case the Support panel is sorted in front of the menu.

When we open the submenu, it adds new items to the stacking context, and we assume that we only need to worry about sorting within that stacking context.

Unfortunately this isn't true, we've made the stacking context bigger, it now intersects the Support panel, and we don't re-evaluate the ordering decision.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add testcase

The first update adds new content that gets into a valid spot, but not the same spot that a full build would place it.

The second update changes the size of the new content such that the different position matters.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Build items infront/behind stacking contexts if the size might change

As discussed on IRC, this catches the case where the change increases the size of the stacking context, and builds items infront and behind so that we update the sorting correctly.

Comment 9

[Miko Mynttinen]

Comment on attachment 8949268 [details] [diff] [review]
Part 2: Build items infront/behind stacking contexts if the size might change

Review of attachment 8949268 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: layout/painting/RetainedDisplayListBuilder.cpp
@@ +811,5 @@
>        CRR_LOG("Frame belongs to stacking context frame %p\n", currentFrame);
>        // If we found an intermediate stacking context with an existing display item
>        // then we can store the dirty rect there and stop. If we couldn't find one then
>        // we need to keep bubbling up to the next stacking context.
> +      nsIFrame::DisplayItemArray* items = currentFrame->GetProperty(nsIFrame::DisplayItems());

It would be nice to have this whole logic in a separate function that returns nullptr if there is no wrapper item or display items, and otherwise the wrapper item.

@@ +822,5 @@
> +          wrapperItem = i;
> +          break;
> +        }
> +      }
> +      if (!wrapperItem) {

Here the frame is a stacking context, has items, but none of them do not have children. Is this case possible? If not, assertion would be nice.

Comment 10

[Miko Mynttinen]

Comment on attachment 8949267 [details] [diff] [review]
Part 1: Add testcase

Review of attachment 8949267 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d105096bbd
Add testcase for modifying the size of a stacking context after a partial build. r=miko
https://hg.mozilla.org/integration/mozilla-inbound/rev/4497aaf5a3c1
Build display items infront and behind a stacking context if the size might have changed. r=miko

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b2d105096bbd
https://hg.mozilla.org/mozilla-central/rev/4497aaf5a3c1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

layout.display-list.retain is set to false by default on Beta, so I don't believe this affects 59 at this point.