1264380 - Cannot drag link inside web component

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Adrian Aichner [:anaran]]

Attachment: 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.

Comment 1

[Adrian Aichner [:anaran]]

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

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thank you very much for that testcase.  It was useful both for fixing this and for fixing bug 1261351!

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74108d5e1ef1

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch: Get Composed Document of Shadow DOM Element Properly.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: 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?

Comment 8

[Adrian Aichner [:anaran]]

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.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: WIP: Add Test Coverage

the test file is missing in previous patch in comment 7.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Test File to Compare The Behavior of Normal Anchor and the Anchor inside Shadow DOM.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

I'll address the original problem in this bug and file other bugs for other symptoms instead.

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78341b41f42f

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf8342f8649e

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44373a91d746

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38b14fecd0ee

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

(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

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I tried to warn you not to die on this hill :)  Testing drag and drop in automation is really hard.

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (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

Comment 24

[William Chen [: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

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=886612cb2c1e

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cc1baee1c41

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3cc1baee1c41