1204944 - Tab drag-rearrangement doesn't appear to work in Nightly w/e10s on OSX

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Jonathan French (:jfrench)]

When I attempt to re-order two tabs in today's Nightly build when running e10s, the page thumbnail that appears, prevents me from re-ordering the tab.

To reproduce:
o run Nightly (43.0a1 (2015-09-15))
o Create a 2nd tab
o LMB-click and try to drag the 2nd tab in front of the first tab

Expected behaviour:
The same as Firefox Release, the selected tab would appear as a semi-transparent thumbnail and slide over to accommodate the dragged tab.

Observed:
The selected tab thumbnail is larger and opaque, and no tab switch occurs.

Workaround:
Disable e10s in Preferences.

Configuration:
OSX 10.10.5 (14F27)
MacBookPro Retina 15-inch
2.6 GHz Intel Core i7
16 GB 1600 MHz DDR3
NVIDIA GeForce GT 750M 2048 MB

Apologies if this is a dupe, but I didn't see any suggested bugs appear during entry, other than a variant bug 1164892.

Comment 1

[Jonathan French (:jfrench)]

Attachment: tabDragNightly_e10s

Per screen grab.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I suspect this is fallout from bug 1202176.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

This is a problem for me but only on the "right half" of the screen.  I'm guessing this is a hidpi only issue?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think Mike is a better person to triage this.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

I can confirm it was bug 1202176.

Comment 6

[Neil Deakin]

Attachment: Patch

This seems to work, but I'm not sure why the CocoaPointsToDevPixels is needed. The other two places within nsChildView.mm where [NSEvent mouseLocation] is used don't seem to need that conversion.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Seems this code could benefit from using the typed units; it's clearly a unit issue (the original one as well), but even fully correct, it isn't obvious from the code that it's correct.  I'm sure a separate bug, just wanted to draw Botond's attention to it.

Comment 8

[Botond Ballo [:botond]]

Looks to me like the following quantities would benefit from being typed:

  (1) nsIDragService.dragMoved() should take its arguments in LayoutDevice pixels
  (2) nsMenuPopupFrame::MoveTo() should take its arguments in CSS pixels
  (3) nsBaseDragService::mImage[X|Y] should be in CSS pixels

We should easily be able to do (2) and (3). We can't do (1) in code because it's IDL and IDL doesn't support typed units, but we can at least document it in a comment.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks, and I see the bug opened for it.  All, sorry for the distraction :)

Comment 10

[Markus Stange [:mstange]]

(In reply to Neil Deakin from comment #6)
> This seems to work, but I'm not sure why the CocoaPointsToDevPixels is
> needed. The other two places within nsChildView.mm where [NSEvent
> mouseLocation] is used don't seem to need that conversion.

That's strange, they should probably have the conversion as well. Can you find out why it works without one, or whether (and if so, why) anything breaks if you add the conversion to the other two places?

Comment 11

[Neil Deakin]

(In reply to Markus Stange [:mstange] from comment #10)
> (In reply to Neil Deakin from comment #6)
> > This seems to work, but I'm not sure why the CocoaPointsToDevPixels is
> > needed. The other two places within nsChildView.mm where [NSEvent
> > mouseLocation] is used don't seem to need that conversion.
> 
> That's strange, they should probably have the conversion as well. Can you
> find out why it works without one, or whether (and if so, why) anything
> breaks if you add the conversion to the other two places?

It looks like the case for SetDragEndPoint is correct as is, or at least, the dragend coordinates are correct if the code is unchanged, and adding a call to CocoaPointsToDevPixels results in incorrect values for event.screenX/Y in the dragend event.
The code here assigns the coordinates from [NSEvent mouseLocation] to event.refPoint.

The Rollup case looks incorrect however. Clicking the searchbox while the popup is open should leave it open, but works ok if I convert using CocoaPointsToDevPixels first.

Comment 12

[Neil Deakin]

Comment on attachment 8661833 [details] [diff] [review]
Patch

Anyway, I think this part is ok.

Comment 14

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7c3ce047ad6fd7b081fccc3c000db1d014820b
Bug 1204944, convert drag position coordinates properly, r=mstange

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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