Closed
Bug 1398471
Opened 7 years ago
Closed 7 years ago
Impossible to move more than 1 selected bookmark in Library after landing patches from bug #1199729
Categories
(Core :: DOM: Events, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | verified |
People
(Reporter: Virtual, Assigned: nika)
References
(Blocks 1 open bug)
Details
(Keywords: nightly-community, regression, reproducible)
Attachments
(1 file)
1.82 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
[Tracking Requested - why for this release]: Regression STR: 1. Move more than 1 selected bookmark to other folder in Library and see that it doesn't work Caused by: bug #1199729 Regression commit message: Part 7: Add manual web-platform-tests for Drag Data Store status, r=baku Regression pushlog: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=5a69e957b1986e6c2e8ae073dce7c8db5c2f5d9d&full=1
Flags: needinfo?(michael)
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Has Regression Range: --- → yes
Has STR: --- → yes
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Summary: Impossible to move more than 1 selected bookmark to other folder in Library after landing patches from bug #1199729 → Impossible to move more than 1 selected bookmark in Library after landing patches from bug #1199729
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 1•7 years ago
|
||
Previously the code looped and yielded between each loop. This caused the DataTransfer to be cleared before the read completed. This splits the loop into 2 sections such that we read all important data from the DataTransfer before it is cleared.
Attachment #8906691 -
Flags: review?(amarchesini)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(michael)
Flags: needinfo?(amarchesini)
Comment 2•7 years ago
|
||
Comment on attachment 8906691 [details] [diff] [review] Read data from DataTransfer before yielding Review of attachment 8906691 [details] [diff] [review]: ----------------------------------------------------------------- I cannot review this code.
Attachment #8906691 -
Flags: review?(amarchesini) → review?(gijskruitbosch+bugs)
Comment 3•7 years ago
|
||
Comment on attachment 8906691 [details] [diff] [review] Read data from DataTransfer before yielding Review of attachment 8906691 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but please can you file a followup bug to add a test for this functionality so we don't regress it again? ::: browser/components/places/content/controller.js @@ +1597,5 @@ > let duplicable = new Map(); > duplicable.set(PlacesUtils.TYPE_UNICODE, new Set()); > duplicable.set(PlacesUtils.TYPE_X_MOZ_URL, new Set()); > > + let dtItems = []; Can you add a code comment above this along the lines of bug comment 1, maybe "Extract all the dragged data synchronously. If we do it asynchronously, the dragdata will get cleared once we yield to the event loop" or something along those lines. Hopefully that will avoid someone "optimizing" this later and breaking it again.
Attachment #8906691 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 4•7 years ago
|
||
Also, what are the odds there are other "multiple drag" operations that were regressed?
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to :Gijs from comment #4) > Also, what are the odds there are other "multiple drag" operations that were > regressed? There's a chance that we could have made other mistakes - I'm actually disabling this change on the DnD side for 57 in bug 1398883 to reduce the risk for 57, and we can look into fixing it up later / making additional changes after then.
Updated•7 years ago
|
Priority: -- → P1
Pushed by michael@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/972346f3252c Read data from DataTransfer before yielding, r=gijs
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/972346f3252c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Comment 10•7 years ago
|
||
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-09-14), so I'm marking this bug as VERIFIED. Thank you very much! \o/
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•