Closed Bug 1387415 Opened 7 years ago Closed 7 years ago

Use flexible spaces (and non-active space in the toolbar in general) as window drag areas on Windows

Categories

(Firefox :: Toolbars and Customization, defect, P1)

Product:
Firefox
Component:
Toolbars and Customization
Platform:
x86
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: phlsa, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 files, 1 obsolete file)

Bug 1387415 - Make empty toolbar spaces be drag handles for the window.
59 bytes, text/x-review-board-request
dao
: review+
Details
bug-1387415-1404294.patch
2.52 KB, patch
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review 
IIRC we already have this behavior on Windows. The »dead« space in the toolbar, particularly the new flexible spaces should be dragable so that users can move the Window. Photon has minimized the drag space in the title bar, especially in maximized mode, so this becomes more valuable.
Whiteboard: [photon-structure] [triage]
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-structure] [triage] → [photon-structure]
Whiteboard: [photon-structure] → [reserve-photon-structure]
Priority: P3 → P4
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
(In reply to :Gijs from comment #1)
> I think this is as simple as adding 'toolbarspring' to this list:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/toolkit/content/xul.css#263-265

That does indeed seem to work. Looking for test coverage, testing etc before pushing for review
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Priority: P4 → P1
Attachment #8910504 - Flags: review?(dao+bmo)
It seems we have no existing test coverage to speak of for window-dragging behavior, due to the need for deeper OS integration in the test framework to get the events necessary? I have tested this on windows, linux (Ubuntu) and OSX though and it seems to work as expected. I have some try results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=68d6fe6b71908127329056801afb8feecf5124a4 which seem encouraging also. So, its a 1 line patch.
Comment on attachment 8910504 [details]
Bug 1387415 - Allow dragging of windows from toolbarspring elements

https://reviewboard.mozilla.org/r/181922/#review187450

::: toolkit/content/xul.css:265
(Diff revision 1)
>  toolbar:not([nowindowdrag="true"]):not([customizing="true"]),
>  statusbar:not([nowindowdrag="true"]),
>  %endif
> -windowdragbox {
> +windowdragbox,
> +toolbarspring {
>    -moz-window-dragging: drag;

This doesn't seem to be implemented on Linux. We'll either need a followup for that, or maybe we should just use the toolbar-drag binding for the nav-bar?
(In reply to Sam Foster [:sfoster] from comment #4)
> I have tested this on windows, linux (Ubuntu) and OSX though and it seems to work as expected.

Hm. I'm on Ubuntu too and it doesn't seem to work.
(In reply to Dão Gottwald [::dao] from comment #6)
> (In reply to Sam Foster [:sfoster] from comment #4)
> > I have tested this on windows, linux (Ubuntu) and OSX though and it seems to work as expected.
> 
> Hm. I'm on Ubuntu too and it doesn't seem to work.

I'm confused, my notes say it works and I typically test first on linux. But yeah I just tested this again and it definitely doesnt work, so I must have mixed up notes/bugs. The toolbar-drag binding extends toolbar so I suspect that will do more than we want here, but let me take a look to see what that looks like.
Attachment #8910504 - Attachment is obsolete: true
Attachment #8910504 - Flags: review?(dao+bmo)
Comment on attachment 8910504 [details]
Bug 1387415 - Allow dragging of windows from toolbarspring elements

https://reviewboard.mozilla.org/r/181922/#review187626

::: toolkit/content/xul.css:265
(Diff revision 1)
>  toolbar:not([nowindowdrag="true"]):not([customizing="true"]),
>  statusbar:not([nowindowdrag="true"]),
>  %endif
> -windowdragbox {
> +windowdragbox,
> +toolbarspring {
>    -moz-window-dragging: drag;

I couldn't find any documentation yet on why this wouldnt work on Linux. In the updated patch I borrowed the draggable constructor stuff from toolbar-drag to create a toolbardecoration-drag binding for toolbarspring elements, for Linux only. 

I'm not sure of the ramifications of making the whole toolbar draggable? That seems like it has the potential for more unexpected/undefined behavior?
Comment on attachment 8910858 [details]
Bug 1387415 - Make empty toolbar spaces be drag handles for the window.

I'm not wild about this. I'm still curious why the -moz-window-drag thing didn't work and would like to at least understand this better before moving forward. But this at least *works* on Mac/Windows/Linux this time.
Attachment #8910858 - Flags: feedback?(dao+bmo)
Comment on attachment 8910858 [details]
Bug 1387415 - Make empty toolbar spaces be drag handles for the window.

oh, the windowdragbox that has -moz-window-dragging: drag applied at http://searchfox.org/mozilla-central/source/toolkit/content/xul.css#263, uses a windowdragbox binding that just creates a WindowDragUtils instance, just like the toolbar-drag and toolbardecoration-drag binding I posted in  attachment 8910858 [details]. I.e. -moz-window-dragging: drag is not exclusive. So I can pull out the ifdefs from this patch I think. 

Sorry for the bug noise, I'll get a new patch up for review shortly.
Attachment #8910858 - Flags: feedback?(dao+bmo)
I'm leaving attachment 8910858 [details] on here for reference (makes toolbarspring a window drag handle), but as Dao points out, comment #0 calls for *all* inactive space on the toolbar to be draggable, not just the toolbarspring elements. So I'll pursue his suggestion to use the toolbar-drag binding for the #nav-bar.
Comment on attachment 8910858 [details]
Bug 1387415 - Make empty toolbar spaces be drag handles for the window.

https://reviewboard.mozilla.org/r/182340/#review189108

::: browser/themes/linux/browser.css:617
(Diff revision 4)
>    color: -moz-menubartext;
>  }
>  
>  #toolbar-menubar:not([autohide="true"]):not(:-moz-lwtheme):-moz-system-metric(menubar-drag),
> +#nav-bar,
>  #TabsToolbar:not(:-moz-lwtheme):-moz-system-metric(menubar-drag) {

The two selectors using :-moz-system-metric(menubar-drag) belong together. Please move #nav-bar before or after them rather than in the middle.
Attachment #8910858 - Flags: review?(dao+bmo) → review+
Comment on attachment 8910858 [details]
Bug 1387415 - Make empty toolbar spaces be drag handles for the window.

https://reviewboard.mozilla.org/r/182340/#review189108

::: browser/themes/linux/browser.css:617
(Diff revision 4)
>    color: -moz-menubartext;
>  }
>  
>  #toolbar-menubar:not([autohide="true"]):not(:-moz-lwtheme):-moz-system-metric(menubar-drag),
> +#nav-bar,
>  #TabsToolbar:not(:-moz-lwtheme):-moz-system-metric(menubar-drag) {

The two selectors using :-moz-system-metric(menubar-drag) belong together. Please move #nav-bar before or after them rather than in the middle.

::: toolkit/content/xul.css:264
(Diff revision 4)
>  titlebar,
>  toolbar:not([nowindowdrag="true"]):not([customizing="true"]),
>  statusbar:not([nowindowdrag="true"]),
>  %endif
>  windowdragbox {
>    -moz-window-dragging: drag;

Please remove this:

http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/base/content/browser.css#318-323
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1654d3ffca3e
Make empty toolbar spaces be drag handles for the window. r=dao
https://hg.mozilla.org/mozilla-central/rev/1654d3ffca3e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Depends on: 1404294
Sam, can you request uplift to 57 for this to help with photon polish? Might be worth including the fix from bug 1404294 so we uplift in 1 patch.
Flags: needinfo?(sfoster)
This patch combines changesets https://hg.mozilla.org/mozilla-central/rev/1654d3ffca3e from this bug, and the follow-up https://hg.mozilla.org/mozilla-central/rev/df01a92fc23e from bug 1404294

Approval Request Comment
[Feature/Bug causing the regression]: The new toolbar spacers in Photon adds dead space to the toolbar. This patch allows that space to be used as a window drag handle

[User impact if declined]:
Photon has minimized the drag space in the title bar, especially in maximized mode, without this patch, some users may find moving windows more difficult  

[Is this code covered by automated tests?]: Not this patch specifically, but window dragging does have test coverage. 

[Has the fix been verified in Nightly?]: Not yet

[Needs manual test from QE? If yes, steps to reproduce]: 
On Windows, Linux and Windows, it should be possible to move a window by dragging from the space on either side of the location bar. 

[List of other uplifts needed for the feature/fix]: As noted above, this patch combines the changesets from bug 1387415 and bug 1404294.

[Is the change risky?]:Low-ish risk

[Why is the change risky/not risky?]:
Extends existing functionality to the navbar. This behavior is new to OSX. Its a minor css/js only change, but has not seen a lot of use yet on Nightly.  

[String changes made/needed]: None
Flags: needinfo?(sfoster)
Attachment #8914969 - Flags: approval-mozilla-beta?
Comment on attachment 8914969 [details] [diff] [review]
bug-1387415-1404294.patch

recent regression/usability issue caused by Photon work, beta57+
Attachment #8914969 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1406401
Reproduced this issue on Ubuntu 16.04 x64 using an affected Nightly build (20170804193726).

Not reproducible anymore on Beta 57.0b7 (20171009192146) and latest Nightly 58.0a1 (20171010220102) under Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: fixed → verified
Flags: qe-verify+
Depends on: 1406751
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: