1545360 - {inc} Drag and drop item from toolbar to overflow menu cause bug

Status: VERIFIED FIXED

(Core :: Layout: Flexbox, defect, P3)

Description

[Stark Eames]

Attachment: screencast of bug

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

1. Customize Toolbar
2. Drag and drop something from "Toolbar" to "Overflow Menu"(Already some item there"
3. When holding upon items there
4. Drop item

Actual results:

The item cannot step aside correctly but overlapped.

Expected results:

The item should give a space when between two items.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

This doesn't appear to affect macOS, but I can reproduce this on Windows.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6ef8cd81d40542b7319127fdb8be840ab16c4cd4&tochange=7845d51b6f3cd691ec3ec97061021f18cda74c0c

Regressed by: 7845d51b6f3c Daniel Holbert — Bug 1490890: Make flex item cached-measurement invalidation more targeted. r=emilio

Comment 3

[Daniel Holbert [:dholbert]]

I can trigger this directly in Browser Toolbox (without needing to drag-n-drop) by adding...
border-block-start-width: 25px;
...to one of the elements in the Overflow Menu. (That's what gets added under the hood when you drag something in.)

Then if I add and remove display:none (to force the frame to be destroyed and recreated), the layout ends up fixed.

Comment 4

[Daniel Holbert [:dholbert]]

And I can reproduce this in

Comment 5

[Daniel Holbert [:dholbert]]

Seems like we need to be including the flex item's BorderPadding as part of the CachedMeasuringReflowResult "key".

The CSS that causes this in the Overflow Menu seems to be pretty trivial, but so far I haven't been able to create a simple HTML testcase that triggers this (using similar CSS)...

Anyway, I'll take a closer look in a debugger this afternoon and will hopefully come up with a simple testcase to sanity-check that the diagnosis + potential fix makes sense.

Comment 6

[Daniel Holbert [:dholbert]]

Ah, so in a standard HTML+CSS testcase, this border change triggers a call to MarkIntrinsicISizesDirty on the flex item, which causes us to clear the cached result:
https://searchfox.org/mozilla-central/rev/966590c35c19d3b7850c5444a31bf0fd68a7d0c4/layout/generic/nsFrame.cpp#5249-5253

So that's why we don't need to bother tracking the borderpadding as part of the "key".

I'm betting that there's a different (non-nsFrame.cpp) MarkIntrinsicISizesDirty implementation that's being invoked in this case. Perhaps the one on nsBoxFrame.cpp (which does not call its superclass method).

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (requires XUL)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: screenshot of buggy rendering of testcase 1 with Nightly (with remote XUL enabled)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 9059615 [details]
screenshot of buggy rendering of testcase 1 with Nightly (with remote XUL enabled)

EXPECTED RESULTS: top half of testcase should look like bottom half.

ACTUAL RESULTS: top half of testcase has superimposed text.

The only differ between the top half and the bottom half is that the pink border-top is added dynamically in the top half.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 1545360: Make nsBoxFrame and nsLeafBoxFrame invoke (or inherit) superclass MarkIntrinsicISizesDirty() method. r?emilio

nsBoxFrame::MarkIntrinsicISizesDirty() is special in that it doesn't
call its superclass method, so it needs this extra invalidation logic
to be duplicated. Other frame classes do invoke their superclass
method, so they don't need this.

The only other place we might need this is nsLeafBoxFrame. Similar to
nsBoxFrame, nsLeafBoxFrame's MarkIntrinsicISizesDirty implementation
doesn't call the superclass method, and is entirely empty in fact!
The nsLeafBoxFrame frame class only seems to be used for XUL 'spring'
and 'spacer' elements, which seem unlikely to need this flex-item
invalidation for now, since they're unlikely to be used as flex items
in a modern CSS flexbox. Also, their intrinsic sizes are probably not
likely to dynamically change often. We can always fix them up later if
we need to.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79b7450eff2c
Make nsBoxFrame and nsLeafBoxFrame invoke (or inherit) superclass MarkIntrinsicISizesDirty() method. r=emilio

Comment 12

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/79b7450eff2c

Comment 13

[Daniel Holbert [:dholbert]]

I verified that I can't reproduce in latest nightly 68.0a1 (2019-04-22) (64-bit), and I double-checked that I can reproduce in Nightly from before the fix landed (2019-04-20).

I think it'd be worth considering an uplift the fix to Firefox 67 beta -- unsetting 'wontfix' there (from comment 2).

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 9059619 [details]
Bug 1545360: Make nsBoxFrame and nsLeafBoxFrame invoke (or inherit) superclass MarkIntrinsicISizesDirty() method. r?emilio

Beta/Release Uplift Approval Request

• User impact if declined: Broken layout (overlapping content) when dragging and dropping items into Overflow Menu in "customize mode". See screencast in https://bugzilla.mozilla.org/attachment.cgi?id=9059205
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0 / attached screencast.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch should just make us invalidate a bit more layout measurements than we currently do, in response to certain types of changes. (And then we have existing code to recompute that layout data from scratch when we need it, which is appropriate in this case because it's stale).

This also only touches XUL layout code, so there shouldn't be any way for it to affect web content (where XUL is disabled). This is effectively frontend-only.

• String changes made/needed: None.

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9059619 [details]
Bug 1545360: Make nsBoxFrame and nsLeafBoxFrame invoke (or inherit) superclass MarkIntrinsicISizesDirty() method. r?emilio

Patch is minimal has tests and was verified manually on Nightly, uplift approved for 67 beta 14, thanks.

Comment 16

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

https://hg.mozilla.org/releases/mozilla-beta/rev/9c937da2511d

Comment 17

[Rares Doghi, Desktop QA]

Hi, I have retested this issue on All Operating systems using Firefox Beta 67.0b14 and I can confirm that this issue is Verified as Fixed. I will mark it accordingly.