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

VERIFIED FIXED in Firefox 67

Status

()

defect
P3
normal
VERIFIED FIXED
a month ago
a month ago

People

(Reporter: eamesstark, Assigned: dholbert)

Tracking

(Regression, {regression})

66 Branch
mozilla68
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 wontfix, firefox67 verified, firefox68 verified)

Details

Attachments

(4 attachments)

Reporter

Description

a month ago
Posted video 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.

Component: Untriaged → Toolbars and Customization

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

Priority: -- → P3

Comment 2

a month ago

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

Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Regressed by: 1490890
Assignee

Comment 3

a month ago

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.

Summary: Drag and drop item from toolbar to overflow menu cause bug → {inc} Drag and drop item from toolbar to overflow menu cause bug
Comment hidden (typo)
Assignee

Updated

a month ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee

Comment 5

a month ago

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.

Assignee

Comment 6

a month ago

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

Assignee

Comment 9

a month ago
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.
Attachment #9059615 - Attachment description: screenshot of buggy rendering of testcase 1 with Nightly (with remote XUL enalbed) → screenshot of buggy rendering of testcase 1 with Nightly (with remote XUL enabled)
Assignee

Comment 10

a month ago

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.

Attachment #9059619 - Attachment description: Bug 1545360: Make nsBoxFrame's "invalidate intrinsic ISizes" codepath also invalidate its cached flex item size, if needed. r?emilio → Bug 1545360: Make nsBoxFrame and nsLeafBoxFrame invoke (or inherit) superclass MarkIntrinsicISizesDirty() method. r?emilio

Comment 11

a month ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79b7450eff2c
Make nsBoxFrame and nsLeafBoxFrame invoke (or inherit) superclass MarkIntrinsicISizesDirty() method. r=emilio

Comment 12

a month ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee

Comment 13

a month ago

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

Status: RESOLVED → VERIFIED
Assignee

Updated

a month ago
Attachment #9059205 - Attachment description: 2019-04-18 15-33-19.mkv → screencast of bug
Assignee

Comment 14

a month ago

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.
Attachment #9059619 - Flags: approval-mozilla-beta?
Assignee

Updated

a month ago
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Attachment #9059619 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 17

a month ago

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.

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.