1393160 - Detect the visibility of the target element in EventUtils.synthesizeDrop

Status: RESOLVED FIXED

(Testing :: Mochitest, enhancement)

Description

[Tooru Fujisawa [:arai]]

(derived from bug 1393158)

the following test is unexpectedly passed now.
  https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/test/browser/browser_indicatorDrop.js

the testcase emulates drag-and-drop onto download button, but the button is now invisible (changed by bug 1371765),
so, the testcase should fail since that action is not possible manually.

we should detect the visibility of the target element in EventUtils.synthesizeDrop and it should fail if the target element is hidden.

Comment 1

[Tooru Fujisawa [:arai]]

bug 1371765 should be in "see also" field since this bug cannot be fixed unless bug 1371765 gets backed out, (or add some workaround for the change),
otherwise the browser_indicatorDrop.js fails.

Comment 2

[:Gijs (he/him)]

I don't envy the person whose job it would be to green up all the tests this would cause to go intermittent...

Comment 3

[:Gijs (he/him)]

(In reply to Tooru Fujisawa [:arai] from comment #1)
> bug 1371765 should be in "see also" field since this bug cannot be fixed
> unless bug 1371765 gets backed out, (or add some workaround for the change),
> otherwise the browser_indicatorDrop.js fails.

No, we would just have to fix the test. But that's hardly the only test that would need fixing. It's not useful to link those via 'see also'. If this fix had been in place at the time of bug 1371765, I would have just added the workaround, as I did for several of the other downloads test (ie add a download to the downloads panel, wait for the button to be fully visible, then do the dnd).

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to :Gijs from comment #3)
> (ie add a
> download to the downloads panel, wait for the button to be fully visible,
> then do the dnd).

I don't think that flow makes sense, even in a testcase.
why do users need to download something in order to download another thing?

Comment 5

[:Gijs (he/him)]

(In reply to Tooru Fujisawa [:arai] from comment #4)
> (In reply to :Gijs from comment #3)
> > (ie add a
> > download to the downloads panel, wait for the button to be fully visible,
> > then do the dnd).
> 
> I don't think that flow makes sense, even in a testcase.
> why do users need to download something in order to download another thing?

The test case is there to verify that the behaviour works, not to justify the behaviour or to keep it the same forevermore or anything else. The testcase serves the UI, not the other way around. If we think it's not useful anymore, maybe we should remove the drag/drop support altogether. We can make that decision in the other bugs. But either way, we're getting further away from the subject of this bug, which is about modifying the test framework.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Detect visibility of the drag event target and fail if hidden.

Made sendDragEvent to check the visibility of the target element and now it fails if the target element is hidden (actually, if it has zero size)

there I think only dragend can be performed even if the target is hidden, since the target element can become hidden after starting drag session on it.

Comment 7

[Tooru Fujisawa [:arai]]

forgot to explain the reasoning why I chose width and height of bounding box for detecting visibility.
I was about to check hidden attribute or style.display property, but it would require checking ancestor nodes recursively, and also it needs dedicated code for XUL and HTML,
so, instead of checking visibility directly, I use the size of bounding box, that becomes zero if it's hidden or display=none,
and also it covers the case that it's visible but actually has zero size; in all cases drag event cannot be performed manually, since user cannot mousedown or mouseup on that element.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/241116278286
Detect visibility of the drag event target and fail if hidden. r=smaug

Comment 9

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/241116278286e4c38fd7461da5eae52ba409ae1b
Bug 1393160 - Detect visibility of the drag event target and fail if hidden. r=smaug

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/241116278286

Comment 11

[Ionuț Goldan [:igoldan]]

Seems like this change updated the baseline for a Talos test:

== Change summary for alert #9097 (as of August 30 2017 07:08 UTC) ==

Improvements:

  2%  tsvg_static summary windows7-32 opt e10s     61.03 -> 59.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9097

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

highly unlikely.

Comment 13

[:Gijs (he/him)]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #11)
> Seems like this change updated the baseline for a Talos test:
> 
> == Change summary for alert #9097 (as of August 30 2017 07:08 UTC) ==
> 
> Improvements:
> 
>   2%  tsvg_static summary windows7-32 opt e10s     61.03 -> 59.67
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=9097

This was a test-only change, so almost certainly no. Please re-check for the actual cause of this change.

Comment 14

[Ionuț Goldan [:igoldan]]

:Gijs, you are right. I will mark this as invalid.