Closed Bug 1438131 Opened 6 years ago Closed 6 years ago

[Wayland] Drop does not work

Categories

(Core :: Widget: Gtk, defect, P3)

Product:
Core
Component:
Widget: Gtk
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1438131 - Implement Drop on Wayland,
59 bytes, text/x-review-board-request
jhorak
: review+
Details 
Dragging tabs does not have any effect:

- tab reorder
- pull tab to open a new window
It's safe to say that any drag and drop event on Firefox UI doesn't work at all, not only tabs.

I've been trying since ever to customize the Firefox toolbar to remove the "Flexible Space" but I couldn't because the drag events doesn't drop anywhere!
Assignee: nobody → stransky
Summary: [Wayland] Tab drag&drop does not work → [Wayland] Drop does not work
Depends on: 1417892
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review239214

::: widget/gtk/nsClipboardWayland.cpp:811
(Diff revision 2)
> +}
> +#endif
> +
>  nsRetrievalContextWayland::~nsRetrievalContextWayland(void)
>  {
> +#ifdef DEBUG

You should free all elements in the hash_table. They should not contain any, but anyway [1]
[1] https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-destroy

Alternatively use g_hash_table_new_full to create hash table with the destroy function or at least add assert for g_hash_table_size() == 0 there that we can check we're not leaking memory there.
Attachment #8958393 - Flags: review?(jhorak)
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review241378

::: commit-message-a6a32:1
(Diff revision 3)
> +Bug 1438131 - Implement Drag and Drop on Wayland, r?jhorak

Could you briefly explain how d&d in wayland works there and in the source?
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review241396

::: widget/gtk/nsClipboardWayland.cpp:355
(Diff revision 3)
> +nsWaylandDragContext::DropBegin(wl_surface *aSurface, uint32_t aTime,
> +                                GtkWidget* aGtkWidget, nscoord aX, nscoord aY)
> +{
> +    mWaylandSurface = aSurface;
> +    mTime = aTime;
> +    mGtkWidget = aGtkWidget;

Can't you get GtkWidget from mWaylandSurface?

::: widget/gtk/nsClipboardWayland.cpp:489
(Diff revision 3)
> +        static_cast<WaylandDataOffer*>(g_hash_table_lookup(mActiveOffers,
> +                                                           aDropDataOffer));
> +    NS_ASSERTION(dataOffer, "We're missing drag and drop dat offer!");
> +    if (dataOffer) {
> +        g_hash_table_remove(mActiveOffers, aDropDataOffer);
> +        mDragAndDropOffer = new nsWaylandDragContext(dataOffer, mDisplay);

The name for mDragAndDropOffer does not match class name nsWaylandDragContext. Offer is part of it. Reconsider to rename it to make it more clear.

::: widget/gtk/nsClipboardWayland.cpp:494
(Diff revision 3)
> +        mDragAndDropOffer = new nsWaylandDragContext(dataOffer, mDisplay);
> +    }
> +}
> +
> +nsWaylandDragContext*
> +nsRetrievalContextWayland::GetDragAndDropDataOffer(void)

It returns pointer to nsWaylandDragContext while named as GetDragAndDropDataOffer.

::: widget/gtk/nsClipboardWayland.cpp:539
(Diff revision 3)
>      context->SetClipboardDataOffer(offer);
>  }
>  
>  // The new fresh wayland data content is drag and drop.
>  static void
>  data_device_enter (void                  *data,

Almost all of the data_device_* functions gets some data from nsWaylandDragContext and then do some action. Consider if the nsWaylandDragContext or even nsRetrievalContextWayland (since it owns nsWaylandDragContext instance) could do the action instead.

For exampledata_device_leave(...) {
  ...
  context->DragDataDeviceLeave();
}

nsRetrievalContextWayland::DragDataDeviceLeave() {
  WindowDragLeaveHandler(mDragAndDropDataOffer->GetWidget());
  mDragAndDropOffer = nullptr;
}

You could get rid of some methods like     ClearDragAndDropDataOffer or GetLastDropInfo, GetWidget (if the action implements methods in nsRetrievalContextWayland).

::: widget/gtk/nsClipboardWayland.cpp:543
(Diff revision 3)
>  static void
>  data_device_enter (void                  *data,
>                     struct wl_data_device *data_device,
>                     uint32_t               time,
>                     struct wl_surface     *surface,
> -                   int32_t                x,
> +                   int32_t                x_fixed,

Why renaming there?

::: widget/gtk/nsClipboardWayland.cpp:549
(Diff revision 3)
> +    context->SetDragAndDropDataOffer(offer);
> +
> +    nsWaylandDragContext* dragContext = context->GetDragAndDropDataOffer();

This is quite confusing for me when reading this code
you call:
context->SetDragAndDropDataOffer(offer)
and on next line:
context->GetDragAndDropDataOffer();

Renaming GetDragAndDropDataOffer should fix this.

::: widget/gtk/nsClipboardWayland.cpp:562
(Diff revision 3)
> +    }
> +
> +    LOGDRAG(("nsWindow data_device_enter for GtkWidget %p\n",
> +             (void*)gtkWidget));
> +
> +    dragContext->DropBegin(surface, time, gtkWidget,

Can you stick to the Enter naming there by renaming DropBegin - DropDataEnter?

Just to be more specific that data entered and not that some data has been just dropped as BeginDrop suggests.

::: widget/gtk/nsClipboardWayland.cpp:588
(Diff revision 3)
>                      struct wl_data_device *data_device,
>                      uint32_t               time,
> -                    int32_t                x,
> -                    int32_t                y)
> +                    int32_t                x_fixed,
> +                    int32_t                y_fixed)
>  {
> +    // We lost focus so our clipboard data are outdated

Seems like copy&paste leftover comment to me.

::: widget/gtk/nsClipboardWayland.cpp:606
(Diff revision 3)
>  
>  static void
>  data_device_drop (void                  *data,
>                    struct wl_data_device *data_device)
>  {
> +    // We lost focus so our clipboard data are outdated

Seems like copy&paste leftover comment to me.
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review241396

> Can't you get GtkWidget from mWaylandSurface?

Yes, that's handled at data_device_enter(). When we fail to get the GtkWidget we even don't perform the drop.
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review241396

> Why renaming there?

because those are in wl_fixed format, see wl_fixed_to_int() or wl_fixed_to_double() for reference.
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review241396

> Almost all of the data_device_* functions gets some data from nsWaylandDragContext and then do some action. Consider if the nsWaylandDragContext or even nsRetrievalContextWayland (since it owns nsWaylandDragContext instance) could do the action instead.
> 
> For exampledata_device_leave(...) {
>   ...
>   context->DragDataDeviceLeave();
> }
> 
> nsRetrievalContextWayland::DragDataDeviceLeave() {
>   WindowDragLeaveHandler(mDragAndDropDataOffer->GetWidget());
>   mDragAndDropOffer = nullptr;
> }
> 
> You could get rid of some methods like     ClearDragAndDropDataOffer or GetLastDropInfo, GetWidget (if the action implements methods in nsRetrievalContextWayland).

It's possible but it also means to implement all D&D actions at nsRetrievalContextWayland().

So we'd need to implement data_device_enter/data_device_leave/data_device_motion/data_device_drop drop at nsRetrievalContextWayland() which looks suboptimal to me.
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review241396

> Can you stick to the Enter naming there by renaming DropBegin - DropDataEnter?
> 
> Just to be more specific that data entered and not that some data has been just dropped as BeginDrop suggests.

DropBegin is a Gtk+ terminology used by Gtk+ handlers and nsDragService and enter/leave is a wayland description. So I'd stick with the Gtk+/nsDragService names here.
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review249554

::: widget/gtk/nsClipboardWayland.h:28
(Diff revision 4)
>      void AddMIMEType(const char *aMimeType);
>  
>      GdkAtom* GetTargets(int* aTargetNum);
>      bool  HasTarget(const char *aMimeType);
>  
> +    virtual void SetAvailableDragActions(uint32_t aWaylandActions) = 0;

You could probably make the method empty instead of make it virtual and override in WaylandDataOffer. That would not require to add it to PrimaryDataOffer.

::: widget/gtk/nsClipboardWayland.h:48
(Diff revision 4)
> +    GdkDragAction GetAvailableDragActions();
> +
> +    void SetAvailableDragActions(uint32_t aWaylandActions);

Nit: please don't divide setter and getter by new line

::: widget/gtk/nsClipboardWayland.h:89
(Diff revision 4)
> +
> +    GtkWidget* GetWidget() { return mGtkWidget; }
> +    GList* GetTargets();
> +    char* GetData(const char* aMimeType, uint32_t* aContentLength);
> +
> +    nsWaylandDragContext(WaylandDataOffer* aWaylandDataOffer,

Nit: put constructor rather on the beginning of the class declaration.

::: widget/gtk/nsClipboardWayland.h:96
(Diff revision 4)
> +private:
> +    virtual ~nsWaylandDragContext() {};
> +
> +    nsAutoPtr<WaylandDataOffer> mDataOffer;
> +    wl_display* mDisplay;
> +    wl_surface* mWaylandSurface;

Is the mWaylandSurface used actually somewhere?

::: widget/gtk/nsClipboardWayland.cpp:208
(Diff revision 4)
> +{
> +    wl_data_offer_accept(mWaylandDataOffer, aTime, aMimeType);
> +}
> +
> +void
> +WaylandDataOffer::SetDragAction(GdkDragAction aAction, uint32_t aTime)

IMO the method is not named correctly and the name colide with nsDragServervice::SetDragAction which do something else (set the current drag action from client into gecko).

The doc for the wl_data_offer_set_actions says [1]: 
Sets the actions that the destination side client supports for this operation.

[1] https://people.freedesktop.org/~whot/wayland-doxygen/wayland/Client/group__iface__wl__data__offer.html#gab3eb99e16ec1eef1a4f76d6b1241ff8c

I would suggests name it SendSupportedActionsToDragSource or SetSupportedActionsForDragSource or similar to be clear that it does communicate back and not into gecko.

::: widget/gtk/nsClipboardWayland.cpp:212
(Diff revision 4)
> +void
> +WaylandDataOffer::SetDragAction(GdkDragAction aAction, uint32_t aTime)
> +{
> +    uint32_t wl_action = gdk_to_wl_actions(aAction);
> +    wl_data_offer_set_actions(mWaylandDataOffer,
> +                              mAvailableDragActions,

use: 
WL_DATA_DEVICE_MANAGER_DND_ACTION_COPY |	      WL_DATA_DEVICE_MANAGER_DND_ACTION_MOVE 
instead of mAvailableDragAction, we don't support WL_DATA_DEVICE_MANAGER_DND_ACTION_ASK anyway.

::: widget/gtk/nsClipboardWayland.cpp:214
(Diff revision 4)
> +    /* Workaround Wayland D&D architecture here. To get the data_device_drop()
> +       signal we need to accept any mime type before data_device_leave().
> +    */

Looking at https://github.com/GNOME/gtk/blob/master/gdk/wayland/gdkdnd-wayland.c#L171 it does not seems to me as a workaround but as correct approach.

::: widget/gtk/nsClipboardWayland.cpp:218
(Diff revision 4)
> +                              wl_action);
> +    /* Workaround Wayland D&D architecture here. To get the data_device_drop()
> +       signal we need to accept any mime type before data_device_leave().
> +    */
> +    if (mTargetMIMETypes[0]) {
> +        wl_data_offer_accept(mWaylandDataOffer, aTime,

Please add a TODO there, that we plan to send all mimetypes we accept to the source, like there's happening: https://github.com/GNOME/gtk/blob/master/gdk/wayland/gdkdnd-wayland.c#L171

Currently there's not enough reference implementation for this.

::: widget/gtk/nsClipboardWayland.cpp:257
(Diff revision 4)
> +    offer->SetAvailableDragActions(source_actions);
>  }
>  
> +/* Advertise recently selected drag and drop action.
> +   We don't need that as we set the drag action explicitly
> +   by nsDataOffer::SetDragAction().

typo nsDataOffer/(Wayland?)DataOffer

::: widget/gtk/nsClipboardWayland.cpp:350
(Diff revision 4)
> +  , mY(0)
> +{
> +}
> +
> +void
> +nsWaylandDragContext::DropBegin(wl_surface *aSurface, uint32_t aTime,

I still think this is misleading, there's no drop-begin in the Gtk, please stick to DropDataEnter or similar.

::: widget/gtk/nsClipboardWayland.cpp:478
(Diff revision 4)
>          }
>      }
>  }
>  
>  void
> -nsRetrievalContextWayland::ClearDataOffers(void)
> +nsRetrievalContextWayland::SetDragAndDropDataOffer(wl_data_offer *aDropDataOffer)

I must say I don't like the name of the method, because you're creating a new mDragContext there which differs from the SetClipboardDataOffer and SetPrimaryDataOffer, maybe SetWaylandDragContext or CreateWaylandDragContext. Please consider it.

::: widget/gtk/nsClipboardWayland.cpp:486
(Diff revision 4)
> +    mDragContext = nullptr;
> +
> +    WaylandDataOffer* dataOffer =
> +        static_cast<WaylandDataOffer*>(g_hash_table_lookup(mActiveOffers,
> +                                                           aDropDataOffer));
> +    NS_ASSERTION(dataOffer, "We're missing drag and drop dat offer!");

typo dat/data

::: widget/gtk/nsDragService.cpp:2127
(Diff revision 4)
>  
>      // default is to do nothing
>      int action = nsIDragService::DRAGDROP_ACTION_NONE;
> -    GdkDragAction gdkAction = gdk_drag_context_get_actions(mTargetDragContext);
> +    GdkDragAction gdkAction = GDK_ACTION_DEFAULT;
> +    if (mTargetDragContext) {
> +        gdkAction = gdk_drag_context_get_actions(mTargetDragContext);

That's not your code, but documentation says the gdk_drag_context_get_actions it for different purpose:
https://developer.gnome.org/gdk3/stable/gdk3-Drag-and-Drop.html#gdk-drag-context-get-actions

::: widget/gtk/nsDragService.cpp:2131
(Diff revision 4)
> +    if (mTargetDragContext) {
> +        gdkAction = gdk_drag_context_get_actions(mTargetDragContext);
> +    }
> +#ifdef MOZ_WAYLAND
> +    else {
> +        gdkAction = mTargetWaylandDragContext->GetAvailableDragActions();

You can probably use gdkAction = mTargetWaylandDragContext->GetDragAction() (not yet existing) there and get rid of GetAvailableDragActions and wl_to_gdk_actions completely.
Attachment #8958393 - Flags: review?(jhorak)
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review250632

::: widget/gtk/nsClipboardWayland.h:96
(Diff revision 4)
> +private:
> +    virtual ~nsWaylandDragContext() {};
> +
> +    nsAutoPtr<WaylandDataOffer> mDataOffer;
> +    wl_display* mDisplay;
> +    wl_surface* mWaylandSurface;

Removed, thanks.
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review250634

::: widget/gtk/nsClipboardWayland.cpp:212
(Diff revision 4)
> +void
> +WaylandDataOffer::SetDragAction(GdkDragAction aAction, uint32_t aTime)
> +{
> +    uint32_t wl_action = gdk_to_wl_actions(aAction);
> +    wl_data_offer_set_actions(mWaylandDataOffer,
> +                              mAvailableDragActions,

I removed the WL_DATA_DEVICE_MANAGER_DND_ACTION_ASK from available source actions.
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review249554

> Looking at https://github.com/GNOME/gtk/blob/master/gdk/wayland/gdkdnd-wayland.c#L171 it does not seems to me as a workaround but as correct approach.

I updated the comment for better explanation. It's a workaround from Gtk+/nsDragService point of view, because Gtk+ let you decide d&d result *after* mouse button release, but Wayland requests that *before* mouse button release.

When we don't explicitly request any data type before mouse button up on Wayland, we get only _leave() event without _drop() so nsDragService::GetData() won't be called and we fail the drop operation (unlike on Gtk+ which asks you what to do after mouse button up).
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review249554

> Please add a TODO there, that we plan to send all mimetypes we accept to the source, like there's happening: https://github.com/GNOME/gtk/blob/master/gdk/wayland/gdkdnd-wayland.c#L171
> 
> Currently there's not enough reference implementation for this.

We don't need that as we request the actual memi type at nsWaylandDragContext::GetData().
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review251010

::: widget/gtk/nsClipboardWayland.h:50
(Diff revision 5)
> +    void SetDragStatus(GdkDragAction aAction, uint32_t aTime);
> +
> +    GdkDragAction GetSelectedDragAction();
> +    void SetSelectedDragAction(uint32_t aWaylandAction);
> +
> +    GdkDragAction GetSourceDragActions();

Seems no longer be used.

::: widget/gtk/nsClipboardWayland.cpp:265
(Diff revision 5)
>  data_offer_source_actions(void *data,
>                            struct wl_data_offer *wl_data_offer,
>                            uint32_t source_actions)
>  {
> +    auto *offer = static_cast<WaylandDataOffer*>(data);
> +    offer->SetSourceDragActions(source_actions);

I still don't get why you need to store source drag actions despite we know what action we support on our side. 

Look at https://github.com/GNOME/gtk/blob/34d1ebc562c83136cd778749d86abfee58e28627/gdk/wayland/gdkdnd-wayland.c#L202
how wl_data_offer_set_actions is called there.

Could you please clarify that in some comment if that's required?

::: widget/gtk/nsDragService.cpp:2130
(Diff revision 5)
> +    if (mTargetDragContext) {
> +        gdkAction = gdk_drag_context_get_actions(mTargetDragContext);
> +    }
> +#ifdef MOZ_WAYLAND
> +    else {
> +        // We get the selected D&D action from compositor on Wayland.

nit: "We got", as long as we already have it set.
Attachment #8958393 - Flags: review?(jhorak)
Comment on attachment 8958393 [details]
Bug 1438131 - Implement Drop on Wayland,

https://reviewboard.mozilla.org/r/227358/#review251712
Attachment #8958393 - Flags: review?(jhorak) → review+
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/7e4166e13b3e
Implement Drop on Wayland, r=jhorak
https://hg.mozilla.org/mozilla-central/rev/7e4166e13b3e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1463632
mozregression bring me here, because drag & drop on Gentoo Linux - Firefox 62.0a1 (2018-05-23) (64-bit) not working.

I can grab a bookmark, but when I try to move and drop nothing happened.

No Wayland here.
Depends on: 1463753
Depends on: 1463661
Depends on: 1463804
Depends on: 1463817
No longer depends on: 1463817
That commit just broke drag-and-drop for me, which was working before. I'm running Wayland but xeyes seems to indicate that Firefox is running under XWayland. I can't read C++ well, but perhaps the commit broke X support altogether?

And how are people even running Firefox on native Wayland?
According to mozregression this is also what broke drag and drop for me on X running Xfce (also Gentoo, Wayland not installed).
Depends on: 1464124
I don't know if this patch has been reverted or not due to the X11 regression, but today, I tested the Nightly Flatpak under Wayland and finally Drag'n'Drop works.

However, when trying to move a tab (re-order) in the same window, it duplicates the tab instead and reload its page. Also, moving the tab outside the window works, but returning it back duplicates it too.

Is this the desired behavior?!
Flags: needinfo?(stransky)
(In reply to Anass Ahmed from comment #28)
> I don't know if this patch has been reverted or not due to the X11
> regression, but today, I tested the Nightly Flatpak under Wayland and
> finally Drag'n'Drop works.

The regression was fixed by Bug 1463753.

> However, when trying to move a tab (re-order) in the same window, it
> duplicates the tab instead and reload its page. Also, moving the tab outside
> the window works, but returning it back duplicates it too.
> 
> Is this the desired behavior?!

I don't it's correct. Can you try latest beta for instance? from https://www.mozilla.org/en-US/firefox/channel/desktop/

If it's a nightly regression please file a new bug for it and cc me there, Thanks.
Flags: needinfo?(stransky)
Looks like someone already opened it: Bug 1464808

And, the beta version doesn't open wayland version (only Xwayland, even with GDK_BACKEND=wayland).
(In reply to Anass Ahmed from comment #30)
> Looks like someone already opened it: Bug 1464808
> 
> And, the beta version doesn't open wayland version (only Xwayland, even with
> GDK_BACKEND=wayland).

Mozilla does not build Wayland target, you need to do your own builds for that with  --enable-default-toolkit=cairo-gtk3-wayland.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: