Remove toolbar-drag (browser / customizable UI version)

RESOLVED FIXED in Firefox 65

Status

()

P3
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
This is linux-only: https://searchfox.org/mozilla-central/rev/c56977420df7a1b692ce0f7e499ddb364d9fd7b2/browser/themes/linux/browser.css#489-498. Note that `moz-menubar-drag` is used in GTK, so for those elements we could maybe use matchMedia.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1473311#c0:

> Move the window dragging code to the extant windowdragging jsm, and invoke it from browser.js and https://dxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js (possibly later than we do now, which should be fine and a minor perf win).

An alternative would be to set up a Custom Element for `toolbar` that checks attrs / media queries and sets this up if necessary. If we did that we could tackle the toolkit version at the same time and not explicitly set up drag for the callers in browser/. The problem there is that we'd have to wait until we removed the XBL binding for CUI toolbar, since we can't have both CE and XBL attached.
(Assignee)

Comment 1

3 months ago
Two other thoughts:

1) We could rewrite consumers from [type="menubar"] to [is="toolbar-draggable"] or similar, and then register a customized built-in CE (as in https://searchfox.org/mozilla-central/rev/9cb3e241502a2d47e2d5057ca771324a446b6695/toolkit/components/printing/content/printPreviewToolbar.js#10). I think the browser consumers don't even set `type="menubar"` at the moment, so we could add `[is]` to those as well.
2) We could change the window dragging util to bind handlers to the document and use event delegation to look for the right type of toolbar, rather than wiring it up to each element individually.
Even better would be to get -moz-window-dragging to work on GTK -- then we could get rid of not only the binding but the whole JSM.
See Also: → bug 1241885, bug 1442961
Setting as P3 for now, please feel free to update
Priority: -- → P3
(Assignee)

Comment 4

2 months ago
Created attachment 9025709 [details]
Bug 1500424 - Remove customizable ui toolbar-drag binding;r=Gijs

It was almost identical to the toolkit version, only missing a [customizing=true]
check to prevent drag. Since Customization only happens in browser/ we are able
to replace the toolkit version with the CUI version, and then remove the CUI version.

The `#toolbar-menubar:not([autohide="true"])` selector will fall back to the
`toolbar[type="menubar"]` selector in global.css to apply the toolkit one, so
that is removed from browser.css.
(Assignee)

Updated

2 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 months ago
Jorg, this patch slightly changes the linux-only "toolbar-drag" binding (it removes this line checking `this.getAttribute("customizing") != "true"`): https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/toolkit/content/widgets/toolbar.xml#25. As far as we can tell, that isn't important to Firefox (since we always overrode this binding with a browser version that didn't have the check), so worth double checking if that's relevant to TB.
Flags: needinfo?(jorgk)
(Assignee)

Updated

2 months ago
Blocks: 1507863

Comment 6

2 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae7984aa4803
Remove customizable ui toolbar-drag binding;r=Gijs

Comment 7

2 months ago
Thanks Brian. I'll forward the NI to some people who know better than me.
Flags: needinfo?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(jorgk)
Flags: needinfo?(arshdkhn1)
I built TB with the patch and a quick check showed customizing still works as before. Dragging with the menubar TB during the customizing doesn't work as before.
Flags: needinfo?(richard.marti)
(Assignee)

Comment 9

2 months ago
(In reply to Richard Marti (:Paenglab) from comment #8)
> I built TB with the patch and a quick check showed customizing still works
> as before. Dragging with the menubar TB during the customizing doesn't work
> as before.

To confirm: it used to not drag, but now it does? That's what I would expect given that we are removing a conditional check in `mouseDownCheck`.
Flags: needinfo?(richard.marti)
It doesn't drag before and now.
Flags: needinfo?(richard.marti)

Comment 11

2 months ago
So is there some work needed in TB? If so, Richard, please file a bug.
(In reply to Jorg K (GMT+1) from comment #11)
> So is there some work needed in TB? If so, Richard, please file a bug.

Not needed as far as I see it.

Comment 13

2 months ago
(In reply to Richard Marti (:Paenglab) from comment #10)
> It doesn't drag before and now.

Makes sense, TB's customization mode is a modal dialog, which prevents the other window from being dragged. Firefox doesn't use a modal, so the behaviour only matters for TB.

Comment 14

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae7984aa4803
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Flags: needinfo?(arshdkhn1)
Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.