Closed Bug 1474142 Opened 2 years ago Closed 2 years ago

Toolbar items disappear when drag and dropping them from the palette / to the palette in Customize Mode

Categories

(Firefox :: Toolbars and Customization, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 + verified
firefox63 + verified

People

(Reporter: magicp.jp, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(3 files)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0 ID:20180707100052

Steps to reproduce:
1. Start Nightly
2. Open hamburger menu > Customize...
3. Drag and drop a toolbar item (Please find the attached video)


Actual results:
Toolbarpaletteitem is gone.


Expected results:
Works fine.


Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c5d001049bb7c80b209fc9f41f665c57d9431262&tochange=bdf1c7272fcca0aa81544fc62b9fda8b90e7b0e0
Blocks: 1451256
Has Regression Range: --- → yes
Has STR: --- → yes
Summary: Toolbarpaletteitem is gone when drag and dragging in Customize → Toolbarpaletteitem is gone when drag and dropping in Customize
[Tracking Requested - why for this release]:
Functional regression.

Emilio, can you have a look, given you wrote the patch that regressed this? It looks like some kind of race condition, because it doesn't happen all the time. I also noticed that when this happens, the icon of the dragged thing disappears already before the drag finishes.
Flags: needinfo?(emilio)
Summary: Toolbarpaletteitem is gone when drag and dropping in Customize → Toolbar items disappear when drag and dropping them from the palette / to the palette in Customize Mode
Sure.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Priority: -- → P1
Tracking for 62, as I'd like to try to get the fix into 62 beta once it's ready.
Attached patch Revert on beta.Splinter Review
This should be enough to fix the regression on beta and give me more time to get to this.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1451256 
[User impact if declined]: this bug
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no (this is a beta-only revert)
[Needs manual test from QE? If yes, steps to reproduce]: see STR of this bug
[List of other uplifts needed for the feature/fix]: none 
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: revert the main part of bug 1451256 to avoid uplifting riskier patches to beta.
[String changes made/needed]: none.
Attachment #8991152 - Flags: review?(gijskruitbosch+bugs)
Attachment #8991152 - Flags: approval-mozilla-beta?
Comment on attachment 8991152 [details] [diff] [review]
Revert on beta.

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

rs=me assuming you've tested this and this is sufficient to fix the bug - ie we don't need to also revert the pointer-events CSS change or the layout BOX_FRAME change, from bug 1451256 ?
Attachment #8991152 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs (he/him) from comment #6)
> Comment on attachment 8991152 [details] [diff] [review]
> Revert on beta.
> 
> Review of attachment 8991152 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me assuming you've tested this and this is sufficient to fix the bug - ie
> we don't need to also revert the pointer-events CSS change or the layout
> BOX_FRAME change, from bug 1451256 ?

No, it should not be needed. We would never hit the layout path because of extends="", and the pointer-events change becomes useless but not harmful with this change.

I'll double-check in a beta build as soon as I have the time, but fixes it on trunk for me.
Did this land on trunk?
Flags: needinfo?(emilio)
We don't want to land this on trunk, I plan to fix this properly on top of bug 1451256 instead.
Flags: needinfo?(emilio)
Comment on attachment 8991152 [details] [diff] [review]
Revert on beta.

Fix for drag and drop in the customize ui, please uplift to beta.
Attachment #8991152 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
This is not fixed in trunk.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I verified this issue on Windows 10, Mac OS X 10.12, Ubuntu 16.04 with FF beta 62.0b10 and I can confirm the fix, the issue form description is no longer reproducible.
Attachment #8990631 - Attachment description: Bug 1474142: Don't do stuff when dropping into the same customizable area. r=Gijs → Bug 1474142: Don't skip calling _onDragEnd even if the from and to target is the customizable area. r=Gijs
Comment on attachment 8990631 [details]
Bug 1474142: Don't skip calling _onDragEnd even if the from and to target is the customizable area. r=Gijs

:Gijs (Not available 3-19 Aug; he/him) has approved the revision.

https://phabricator.services.mozilla.com/D2019
Attachment #8990631 - Flags: review+
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/c56155e57e01
Don't skip calling _onDragEnd even if the from and to target is the customizable area. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/c56155e57e01
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
I verified this issue on Windows 10, Mac OS X 10.12, Ubuntu 16.04 with FF Nightly 63.0a1(2018-08-06) and I can confirm the fix, the issue form description is no longer reproducible.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.