1210091 - New Tab Page Drag&Drop Causes Overlapping Tiles

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Tim Spurway [:tspurway]]

Attachment: Screen grab movie of newtab drag&drop

With the introduction of scrollbars in about:newtab, we can easily reproduce a bunch of inconsistent behaviour while dragging and dropping tiles.

The first problem is that the page does not 'hot scroll', which is a typical behaviour when dragging off/near a scrollable boundary.  The expected behaviour would be that scrolling would automatically be activated when a tile is dragged close to the edge of a scrollable region.  That is, a tile dragged to the bottom of the window should cause the window's contents to 'scroll up'.

When a tile is dropped onto a partially visible tile on the bottom of the screen, it causes inconsistent overlapping of tiles, where one tile sits on top of another.  Resizing the window while in this state causes the obscured tile (and the search text entry box) to move incorrectly.  In addition, there is a 'hole' in the tileset where (one would presume) the obscured tile is *supposed* to be.

Also, once the newtab is in this state, drag & drop does not work anymore.

(please see attachment movies)

Comment 1

[Doug Turner (:dougt)]

Hey all,

Back out the regression on beta asap.  Fix on nightly please.

Comment 2

[maxim zhilyaev]

This bug will be fixed by uplift of the fix to 1196437.
I am requesting the uplift for beta/aurora right now.

Comment 3

[maxim zhilyaev]

I meant the fix to Bug 1196437 will fix this issue.

Comment 4

[maxim zhilyaev]

Bug 1196437 beta uplift request is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1196437#c11

Comment 5

[maxim zhilyaev]

After talking to tspurway and watching him reproducing this bug, I managed to repro same behavior on current nightly build.  So, following dougt requested suggest backing out scrollbar from beta until root cause could be identified.

Comment 6

[maxim zhilyaev]

apparently this behavior is not new: https://bugzilla.mozilla.org/show_bug.cgi?id=1000097
The scroll bar surfaces it badly, so we will backout the scrollbar per Bug 1211016.

Comment 7

[maxim zhilyaev]

[Tracking Requested - why for this release]:

Comment 8

[maxim zhilyaev]

Attachment: 1210091.v1.patch

Root cause analysis: 
This failure occurs when the mouse is released outside the viewport of the newtab window.  In which case no drop event is generated, and the dragged tile moves back into its original position.  However, the rest of tiles still handle dragend event and move into new positions. which is why the user sees overlapping tiles and empty spots.

The issue was present since the grid was introduced in 2013. However, addition of the scrollbar made it very easy for a user to slide past the bottom of the window when he drags top tile into the bottom row.

The fix:
Simply check if the drop event was seen inside dragend handler and abort the whole drag&drop operation if the drop never arrived. This assumes that drop event always precedes dragend event, which seems to be the case according to mdn documentation (https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Drag_and_drop) and manual testing

unit testing:
creating a mochitest for the failure is involved. Given that this code will become obsolete with introduction of remote newtab, suggest manual testing only.

Comment 9

[Marcos Caceres [:marcosc]]

Comment on attachment 8672033 [details] [diff] [review]
1210091.v1.patch

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

R+, just minor nit.

::: browser/base/content/newtab/dropTargetShim.js
@@ +141,5 @@
>        this._lastDropTarget = null;
>        this._cellPositions = null;
>      }
>  
> +    this._dropSeen = null;

Can you please set this to false instead?

Comment 10

[Marcos Caceres [:marcosc]]

Comment on attachment 8672033 [details] [diff] [review]
1210091.v1.patch

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

R+, just minor nit.

::: browser/base/content/newtab/dropTargetShim.js
@@ +141,5 @@
>        this._lastDropTarget = null;
>        this._cellPositions = null;
>      }
>  
> +    this._dropSeen = null;

Can you please set this to false instead?

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/630095ca2f1f

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/630095ca2f1f

Comment 13

[maxim zhilyaev]

Attachment: nightly patch v2. - fixed reviewer comment

Comment 14

[maxim zhilyaev]

Attachment: aurora patch

Approval Request Comment
[Feature/regressing bug #]:

Bug 1210091 - New Tab Page Drag&Drop Causes Overlapping Tiles

[User impact if declined]:

Significant user impact. The bug existed in the code since 2013. Introduction of the scrollbar made releasing the drag outside the browser window easy. In which case the drop event would never be generated, and the failure case would surface (for details see https://bugzilla.mozilla.org/show_bug.cgi?id=1210091#c8).

Since 1180387 functionality was requested by community at large (note that it was backed out from beta in 1211016), we recommend the fix to be uplifted to aurora to then become beta.

[Describe test coverage new/current, TreeHerder]:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd03745d1eed
manually tested on linux, macos and windows

[Risks and why]: 

Risks are minimal - fixing per-exsting flaw in code logic. 

[String/UUID change made/needed]:  None

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei can your team verify this fix on Nightly before we uplift to aurora? Thanks.

Comment 16

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Andrei can your team verify this fix on Nightly before we uplift to aurora?
> Thanks.

Assigning Mihai for verification.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

42 is unaffected as we backout the patch causing this issue. See bug 1211016 for more information.

Comment 18

[Mihai Boldan, Desktop QA [:mboldan]]

I was able to reproduce this issue on Windows 10 x86 using Firefox 43.0a2 (20151014004030).
The issue is no longer reproducible on Windows 10 x86, Mac OS X 10.11 and Ubuntu 14.04 x86, using Firefox 44.0a1 (20151014030223).

I'll keep an eye on this bug in order to verify it as soon as it lands in aurora build.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8672827 [details] [diff] [review]
aurora patch

Fix verified in nightly; ok to uplift to aurora.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c252d305abf6

Comment 21

[Mihai Boldan, Desktop QA [:mboldan]]

The issue is no longer reproducible on Windows 10 x86, Mac OS X 10.11 and Ubuntu 14.04 x64, using Firefox 43.0 Beta 1.