Cannot drag link inside web component

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: anaran, Assigned: bevis)

Tracking

48 Branch
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8741054 [details]
shady-link.js

test case to reproduce issue described in user story

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160412050029

Steps to reproduce:

Drag link to tab bar as I often do to inspect blob URL content.

See self-documenting scratchpad script attchment for the test case.

Run it against any web page.

A div with a link will be added to page at top left.

Click the x symbol to close that div again.


Actual results:

Link cannot be dragged (no visible drag effect), not dropped in tab bar (no insert arrow).



Expected results:

Dragging blob links works fine in web apps and jetpack add-ons I wrote.

It does not work for a link inside a web component.

It's quite possible I am doing something wrong in my use of web components and would appreciate feedback to that effect.
(Reporter)

Updated

3 years ago
Assignee: nobody → khuey
(Reporter)

Updated

3 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 1

3 years ago
Comment on attachment 8741054 [details]
shady-link.js

test case to reproduce issue described in user story

test case to reproduce issue described in user story
(Reporter)

Updated

3 years ago
Attachment #8741054 - Attachment description: shady-link.js → shady-link.js test case to reproduce issue described in user story
Component: Untriaged → DOM
Product: Firefox → Core
Whiteboard: btpp-active
After further inspection with Kyle side-by-side,
we found that we should invoke GetComposedDoc() instead of GetUncomposedDoc() at [1] to allow the transfer of the element in shadow tree.

Take this bug to follow up the fix.

BTW, I'd like to add a note that
it's very valuable experience to me on inspecting this and then encountering another bug to learn more. :-)

[1] https://hg.mozilla.org/mozilla-central/file/8630367f5e3f/dom/events/DataTransfer.cpp#l930
Assignee: khuey → btseng
Thank you very much for that testcase.  It was useful both for fixing this and for fixing bug 1261351!
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3)
> Thank you very much for that testcase.  It was useful both for fixing this
> and for fixing bug 1261351!

Yes, :anaran's testcase is the key to find out the root cause easily!
Sorry to :anaran for not mentioning about this earlier.
Created attachment 8741650 [details] [diff] [review]
(v1) Patch: Get Composed Document of Shadow DOM Element Properly.
Created attachment 8741651 [details] [diff] [review]
WIP: Add Test Coverage

The fix in comment 6 is workable if we drag & drop the link manually.

However, I haven't found a suitable way for the automation test of this bug yet.

In this WIP patch:
1. 'dragstart' seems not the right event to verify the fix in comment 6, because the 'dragstart' will still be triggered right after the anchor is manually dragged without the fix in comment 6.
2. 'synthesizeMouse' function adopted by this test case didn't help me to trigger the 'dragstart' event on the anchor element. :(

Need some advice from expert regarding 
1. how can we synthesize the mouse down/drag properly on this anchor element inside shadow dom?
2. what's the correct event to be check to verify this fix?
(Reporter)

Comment 8

3 years ago
Another observation: The context menu of a link inside a web component is also lacking (before the fix) link-related entries to open, bookmark, save and copy that link.
Created attachment 8741845 [details] [diff] [review]
WIP: Add Test Coverage

the test file is missing in previous patch in comment 7.
Attachment #8741651 - Attachment is obsolete: true
Created attachment 8741851 [details]
Test File to Compare The Behavior of Normal Anchor and the Anchor inside Shadow DOM.
(In reply to Adrian Aichner [:anaran] from comment #8)
> Another observation: The context menu of a link inside a web component is
> also lacking (before the fix) link-related entries to open, bookmark, save
> and copy that link.

I've simplify the test file in comment 10 for better comparison between normal anchor & broken one inside a shadow DOM in one file.
Unfortunately, I found that the context menu problem you mentioned still exists after applying the fix in comment 6. :(
In addition, even the link is draggable to the tab bar with the fix, there is no cloned text of |Drag me to tab bar (if you can:-)| along with the moving cursor.
Summary: Cannot drag link inside web component → Functions of the Link inside Web Component is Broken: Drag & Drop, Context Menu.
Summary: Functions of the Link inside Web Component is Broken: Drag & Drop, Context Menu. → Functions of the Link inside Web Component like Drag & Drop and Context Menu are Broken
I'll address the original problem in this bug and file other bugs for other symptoms instead.
Summary: Functions of the Link inside Web Component like Drag & Drop and Context Menu are Broken → Cannot drag link inside web component
Depends on: 1265101
See Also: → bug 1265101
See Also: → bug 1265104
No longer depends on: 1265101
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #7)
> Need some advice from expert regarding 
> 1. how can we synthesize the mouse down/drag properly on this anchor element
> inside shadow dom?
Mouse event can be synthesized once the script is run after onload.
> 2. what's the correct event to be check to verify this fix?
We found the difference between normal case and the abnormal case while getting the composed document of the select element in comment 6, but we haven't figure out why this fix makes the drap & drop takes effect eventually. Need some patience to dig into the drag & drop flow to conclude a correct automation test which covers this change.
> We found the difference between normal case and the abnormal case while
> getting the composed document of the select element in comment 6, but we
> haven't figure out why this fix makes the drap & drop takes effect
> eventually. Need some patience to dig into the drag & drop flow to conclude
> a correct automation test which covers this change.

The difference of this fix is that nsIDragService.getCurrentSession() shall be valid after "dragstart" is triggered.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #16)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf8342f8649e
The fix itself on comment 16 is fine.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #17)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=44373a91d746
However, the test case in comment 17:
1. Always failed in non-e10s mode with different reason in differnt platforms.
2. Caused another test case test_bug508479.html failed unexpected in Linux x64 debug build.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #19)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=38b14fecd0ee

 [test_bug1264380.html]
+run-if = e10s

In comment 18, we found that nsDragService behaves differently in different platform implementations which prevents us to check the validity of nsIDragService::getCurrentSession() consistently.

Take OS X as example,
The dragSession will be started and ended immediately inside nsDragService::InvokeDragSessionImpl()[1] (The |mDoingDrag| will be set at nsBaseDragService::StartDragSession() and reset at nsBaseDragService::EndDragSession(), so there is no chance to check the validity of current drag session during the test unless we manually startDragSession/initDragEvent/endDragSession which is not the case of verifying this fix(which ensures the the drag session will be created after 'dragstart' is fired).

Hence, I'd like to narrow down the test scope in e10s mode only.

[1] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsDragService.mm?from=nsDragService.mm#356-374
I tried to warn you not to die on this hill :)  Testing drag and drop in automation is really hard.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #21)
> I tried to warn you not to die on this hill :)  Testing drag and drop in
> automation is really hard.
It's ture. QQ
Seeing is believing. Just realized how bad it is. :p
Created attachment 8745168 [details] [diff] [review]
(v2) Patch: Get Composed Document of Shadow DOM Element Properly. r=wchen

This fix is to ensure not being bailed out earlier due to the invalidity of the transferable from the dragged element inside shadow DOM at EventStateManager::DoDefaultDragStart()[1] without initiating a new drag session at the end of this function.

A test case is included but it can only be run in e10s mode due to the reason explained in comment 20.

Hi William,

May I have your review for this change?

Thanks!

[1] https://hg.mozilla.org/mozilla-central/annotate/fc15477ce628599519cb0055f52cc195d640dc94/dom/events/EventStateManager.cpp#l1960
Attachment #8741650 - Attachment is obsolete: true
Attachment #8745168 - Flags: review?(wchen)
Comment on attachment 8745168 [details] [diff] [review]
(v2) Patch: Get Composed Document of Shadow DOM Element Properly. r=wchen

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

thanks for fixing this
Attachment #8745168 - Flags: review?(wchen) → review+
Attachment #8741845 - Attachment is obsolete: true
Attachment #8745168 - Attachment description: (v2) Patch: Get Composed Document of Shadow DOM Element Properly. → (v2) Patch: Get Composed Document of Shadow DOM Element Properly. r=wchen
Keywords: checkin-needed

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3cc1baee1c41
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1269813
You need to log in before you can comment on or make changes to this bug.