Closed Bug 1349552 Opened 7 years ago Closed 7 years ago

Add Setting for More Window Drag Space

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- verified

People

(Reporter: dao, Assigned: johannh)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Whiteboard: [photon-visual][p1])

Attachments

(4 files)

Needinfo Stephen for UX spec. In particular:

- Where should this option be?
- How exactly should windows with additional drag space look on Windows/mac/Linux?
- How does this relate to the Title bar option which basically already provides additional drag space?
Flags: needinfo?(shorlander)
Whiteboard: [photon]
No longer blocks: photon-ui-refresh
Depends on: 1355395
Flags: needinfo?(shorlander)
Whiteboard: [photon] → [photon][57]
Whiteboard: [photon][57] → [photon-visual][57]
Depends on: 1355764
Priority: -- → P3
Blocks: photon-tabs
No longer blocks: photon-visual
Flags: qe-verify+
Priority: P3 → P2
QA Contact: ovidiu.boca
Whiteboard: [photon-visual][57] → [photon-visual][p3][57]
QA Contact: ovidiu.boca → brindusa.tot
Stephen, since bug 1355764 landed, we've mostly been getting complaints about /excessive/ drag space, due to bug 1369786. I haven't seen any demand for restoring the old behavior among the nightly community, and I'm not sure this will be much different across the general user base. Do we really need this setting? It's not trivial to support this in the theme; we'll pay an ongoing maintenance cost for this.
Flags: needinfo?(shorlander)
We've received the first complaint in bug 1371769. In bug 1371769 comment 5, bz says he's using the titlebar pref basically as a substitute for this nonexisting setting. Is that good enough?
Blocks: 1371769
No longer depends on: 1355764
Since we're at complains, I'll add just my 2 cents. I find the drag space on the left very uncomfortable since I have to cross the whole screen to reach it. Being right-handed I'd prefer having it on the right, indeed very often I just use the space before the system buttons and ignore the provided drag space. I suppose left-handed people may prefer it on the left.
I've seen others willing to completely remove it too.
I was just wondering maybe it could be implemented as flexible space, if flexible space would allow to drag the window (it currently doesn't). Then one could move it or enlarge it as preferred (currently the flexible space is not even flexible, but I assume it's a bug).
(In reply to Marco Bonardo [::mak] from comment #3)
> Since we're at complains, I'll add just my 2 cents. I find the drag space on
> the left very uncomfortable since I have to cross the whole screen to reach
> it. Being right-handed I'd prefer having it on the right, indeed very often
> I just use the space before the system buttons and ignore the provided drag
> space. I suppose left-handed people may prefer it on the left.

There should be drag space on both sides.
(In reply to Dão Gottwald [::dao] from comment #4)
> There should be drag space on both sides.

Ah, I thought it was there by "chance" since it looks smaller, it's probably just an optical effect due to the different widgets in the area. Thank you for the clarification, I retire my complain if the space on the right is there to stay :)
Attached image lack-of-drag-space.png
I don't have any drag space on my right side.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Created attachment 8888341 [details]
> lack-of-drag-space.png
> 
> I don't have any drag space on my right side.

We'll need to add it then. Windows already has it although I expect we'll make it wider for photon: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/themes/windows/browser.css#357-359
(In reply to Marco Bonardo [::mak] from comment #3)
> I was just wondering maybe it could be implemented as flexible space, if
> flexible space would allow to drag the window (it currently doesn't). Then
> one could move it or enlarge it as preferred (currently the flexible space
> is not even flexible, but I assume it's a bug).

The new Flexible Spacers should also allow dragging:

https://cl.ly/1A2o1t0A3D0I

So the finished implementation would have more symmetrical persistent drag spaces:

https://cl.ly/0q012g123L3v
(In reply to Stephen Horlander [:shorlander] from comment #8)
> The new Flexible Spacers should also allow dragging:
> 
> https://cl.ly/1A2o1t0A3D0I

Is there a bug filed on implementing this?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Dão Gottwald [::dao] from comment #9)
> (In reply to Stephen Horlander [:shorlander] from comment #8)
> > The new Flexible Spacers should also allow dragging:
> > 
> > https://cl.ly/1A2o1t0A3D0I
> 
> Is there a bug filed on implementing this?

I filed bug 1383009 purely for adding the flexible spaces. As for making empty space drag stuff cross-platform, I've heard people talk about doing that. It already works on OS X, but I expect it does not on Windows if you haven't applied a lightweight theme. I don't know if there's a separate bug on file for that. I guess we could morph this bug if that's the desired solution here.
Flags: needinfo?(gijskruitbosch+bugs)
We decided that we'll do this.
Flags: needinfo?(shorlander)
Keywords: uiwanted
Whiteboard: [photon-visual][p3][57] → [photon-visual][p1]
No longer blocks: 1371769
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Priority: P2 → P1
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
QA Contact: brindusa.tot → ovidiu.boca
Assignee: dao+bmo → jhofmann
Comment on attachment 8906536 [details]
Bug 1349552 - Part 3 - Add a test for drag space in customize mode.

https://reviewboard.mozilla.org/r/178288/#review183634

Tests look good, thanks!

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:16
(Diff revision 2)
> +});
> +
> +add_task(async function test_dragspace_checkbox() {
> +  let win = document.getElementById("main-window");
> +  let checkbox = document.getElementById("customization-extra-drag-space-checkbox");
> +  ok(!checkbox.checked, "Drag space is disabled initially.");

I think this message is more relevant to the assertion on the next line, don't you think? Maybe a better message for the checkbox assertion is something like "Checkbox state correctly reflects drag space pref"?

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:22
(Diff revision 2)
> +  is(Services.prefs.getBoolPref(PREF_DRAG_SPACE), false);
> +
> +  let dragSpaceEnabled = BrowserTestUtils.waitForAttribute("extradragspace", win, "true");
> +  EventUtils.synthesizeMouseAtCenter(checkbox, {});
> +  await dragSpaceEnabled;
> +  is(Services.prefs.getBoolPref(PREF_DRAG_SPACE), true);

Please add a message to this assertion like "Drag space should be enabled after checkbox is clicked"

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:27
(Diff revision 2)
> +  is(Services.prefs.getBoolPref(PREF_DRAG_SPACE), true);
> +
> +  let dragSpaceDisabled = BrowserTestUtils.waitForCondition(() => !win.hasAttribute("extradragspace"));
> +  EventUtils.synthesizeMouseAtCenter(checkbox, {});
> +  await dragSpaceDisabled;
> +  is(Services.prefs.getBoolPref(PREF_DRAG_SPACE), false);

Please add a message to this assertion

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:39
(Diff revision 2)
> +  ok(!titleBarCheckbox.checked, "Title bar is disabled initially.");
> +  ok(!dragSpaceCheckbox.hasAttribute("disabled"), "Drag space checkbox is enabled initially.");
> +
> +  let checkboxDisabled = BrowserTestUtils.waitForAttribute("disabled", dragSpaceCheckbox, "true");
> +  EventUtils.synthesizeMouseAtCenter(titleBarCheckbox, {});
> +  await checkboxDisabled;

Hmm, what do you think about showing a message here? We're showing a message that the checkbox is enabled initially so it would be nice to also have a message saying it was disabled.

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:43
(Diff revision 2)
> +  EventUtils.synthesizeMouseAtCenter(titleBarCheckbox, {});
> +  await checkboxDisabled;
> +
> +  let checkBoxEnabled = BrowserTestUtils.waitForCondition(() => !dragSpaceCheckbox.hasAttribute("disabled"));
> +  EventUtils.synthesizeMouseAtCenter(titleBarCheckbox, {});
> +  await checkBoxEnabled;

And a message here?

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:49
(Diff revision 2)
> +});
> +
> +add_task(async function test_dragspace_reset() {
> +  let win = document.getElementById("main-window");
> +  let checkbox = document.getElementById("customization-extra-drag-space-checkbox");
> +  ok(!checkbox.checked, "Drag space is disabled initially.");

See my earlier comment about the message being relevant to the next line.

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:56
(Diff revision 2)
> +
> +  // Enable dragspace manually.
> +  let dragSpaceEnabled = BrowserTestUtils.waitForAttribute("extradragspace", win, "true");
> +  EventUtils.synthesizeMouseAtCenter(checkbox, {});
> +  await dragSpaceEnabled;
> +  is(Services.prefs.getBoolPref(PREF_DRAG_SPACE), true);

Message

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:59
(Diff revision 2)
> +  EventUtils.synthesizeMouseAtCenter(checkbox, {});
> +  await dragSpaceEnabled;
> +  is(Services.prefs.getBoolPref(PREF_DRAG_SPACE), true);
> +
> +  // Disable dragspace through reset.
> +  let dragSpaceDisabled = BrowserTestUtils.waitForCondition(() => !win.hasAttribute("extradragspace"));

We don't need to store the waitForCondition promise, we can directly await on it after gCustomizeMode.reset(). This will result in fewer polls, and will avoid crazy (though admittedly highly unlikely) intermittent failures if waitForCondition() for some reason times out before reset() completes, too.

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:68
(Diff revision 2)
> +
> +  // Undo reset and check that dragspace is enabled again.
> +  dragSpaceEnabled = BrowserTestUtils.waitForAttribute("extradragspace", win, "true");
> +  await gCustomizeMode.undoReset();
> +  await dragSpaceEnabled;
> +  is(Services.prefs.getBoolPref(PREF_DRAG_SPACE), true);

Message

::: browser/components/customizableui/test/browser_customizemode_dragspace.js:70
(Diff revision 2)
> +  dragSpaceEnabled = BrowserTestUtils.waitForAttribute("extradragspace", win, "true");
> +  await gCustomizeMode.undoReset();
> +  await dragSpaceEnabled;
> +  is(Services.prefs.getBoolPref(PREF_DRAG_SPACE), true);
> +
> +  Services.prefs.clearUserPref(PREF_DRAG_SPACE);

Do we need to do this here as well as in cleanup()? I'm tempted to say we should just keep both for thoroughness, but I'm not sure. I don't feel strongly about this, feel free to drop this issue.
Attachment #8906536 - Flags: review?(nhnt11)
Comment on attachment 8906535 [details]
Bug 1349552 - Part 2 - Add an extra drag space setting to customize mode.

https://reviewboard.mozilla.org/r/178286/#review183560

Thanks for the patch, tested it quickly and it seems to work well. Just a couple of minor comments.

::: browser/components/customizableui/CustomizableUI.jsm:2686
(Diff revision 1)
>      let uiCustomizationState = gUIStateBeforeReset.uiCustomizationState;
>      let drawInTitlebar = gUIStateBeforeReset.drawInTitlebar;
> +    let extraDragSpace = gUIStateBeforeReset.extraDragSpace;
>      let currentTheme = gUIStateBeforeReset.currentTheme;
>      let uiDensity = gUIStateBeforeReset.uiDensity;
>      let autoTouchMode = gUIStateBeforeReset.autoTouchMode;

This block of assignments is getting quite large now, what do you think about destructuring? I.e.:

let {
  uiCustomizationState,
  drawInTitlebar,
  ...,
} = gUIStateBeforeReset;

::: browser/components/customizableui/CustomizeMode.jsm:1560
(Diff revision 1)
> +      checkbox.setAttribute("checked", "true");
> +    } else {
> +      checkbox.removeAttribute("checked");
> +    }
> +
> +    if (!drawInTitlebar || menuBarEnabled) {

Shouldn't we set the disabled attribute first and return early if it's true? I'd say the checkbox should not be checked even if the pref is set, if we're disabling it - that would reflect the actual state of whether we're showing the drag space in UI.

I don't know if there's precedent for setting the checked state of a disabled checkbox. If there is, we should follow it, I suppose.

I don't think this blocks r+ though.
Attachment #8906535 - Flags: review?(nhnt11) → review+
Comment on attachment 8906535 [details]
Bug 1349552 - Part 2 - Add an extra drag space setting to customize mode.

https://reviewboard.mozilla.org/r/178286/#review183560

> This block of assignments is getting quite large now, what do you think about destructuring? I.e.:
> 
> let {
>   uiCustomizationState,
>   drawInTitlebar,
>   ...,
> } = gUIStateBeforeReset;

Wouldn't that even add more lines? It's also touching blame needlessly, so I'm not sure...

> Shouldn't we set the disabled attribute first and return early if it's true? I'd say the checkbox should not be checked even if the pref is set, if we're disabling it - that would reflect the actual state of whether we're showing the drag space in UI.
> 
> I don't know if there's precedent for setting the checked state of a disabled checkbox. If there is, we should follow it, I suppose.
> 
> I don't think this blocks r+ though.

> Shouldn't we set the disabled attribute first and return early if it's true?

I wouldn't say that that's a good idea in this use case. That block is pretty self-contained in it's logic and it's easy to add more code after it.

> I'd say the checkbox should not be checked even if the pref is set, if we're disabling it - that would reflect the actual state of whether we're showing the drag space in UI.
> 
> I don't know if there's precedent for setting the checked state of a disabled checkbox. If there is, we should follow it, I suppose.

I've had this discussion a couple of times already, I believe. I usually point to about:preferences where it's inconsistent with a slight advantage for my solution (last time I checked). I just personally prefer the variant that I implemented, so I'll go with it if you don't have a strong opinion :)
Comment on attachment 8906535 [details]
Bug 1349552 - Part 2 - Add an extra drag space setting to customize mode.

https://reviewboard.mozilla.org/r/178286/#review183560

> Wouldn't that even add more lines? It's also touching blame needlessly, so I'm not sure...

No worries, let's leave it as it is. I just like the syntactic sugar ;)

> > Shouldn't we set the disabled attribute first and return early if it's true?
> 
> I wouldn't say that that's a good idea in this use case. That block is pretty self-contained in it's logic and it's easy to add more code after it.
> 
> > I'd say the checkbox should not be checked even if the pref is set, if we're disabling it - that would reflect the actual state of whether we're showing the drag space in UI.
> > 
> > I don't know if there's precedent for setting the checked state of a disabled checkbox. If there is, we should follow it, I suppose.
> 
> I've had this discussion a couple of times already, I believe. I usually point to about:preferences where it's inconsistent with a slight advantage for my solution (last time I checked). I just personally prefer the variant that I implemented, so I'll go with it if you don't have a strong opinion :)

Alright, WFM! Thanks.
Comment on attachment 8906536 [details]
Bug 1349552 - Part 3 - Add a test for drag space in customize mode.

https://reviewboard.mozilla.org/r/178288/#review183872

Thanks!
Attachment #8906536 - Flags: review?(nhnt11) → review+
Comment on attachment 8906534 [details]
Bug 1349552 - Part 1 - Add optional drag space on top of the tabstrip.

https://reviewboard.mozilla.org/r/178284/#review184344

::: browser/themes/osx/browser.css:86
(Diff revision 1)
>  /**
>   * For tabs in titlebar on OS X, we stretch the titlebar down so that the
>   * tabstrip can overlap it.
>   */
>  #main-window[tabsintitlebar] > #titlebar {
> -  min-height: calc(var(--tab-min-height) - var(--tab-toolbar-navbar-overlap));
> +  -moz-appearance: -moz-mac-vibrancy-dark;

This looks like it makes http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/browser/themes/osx/browser.css#905 redundant, except that you're missing the fallback background-color for OS X 10.9, and your selector looks less correct.
Attachment #8906534 - Flags: review?(dao+bmo) → review+
Blocks: 1397401
Comment on attachment 8906534 [details]
Bug 1349552 - Part 1 - Add optional drag space on top of the tabstrip.

https://reviewboard.mozilla.org/r/178284/#review184344

> This looks like it makes http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/browser/themes/osx/browser.css#905 redundant, except that you're missing the fallback background-color for OS X 10.9, and your selector looks less correct.

Yes, it's not redundant, but I should add the titlebar to that rule so that it includes the fallback color.
(In reply to Johann Hofmann [:johannh] from comment #30)
> Comment on attachment 8906534 [details]
> Bug 1349552 - Part 1 - Add optional drag space on top of the tabstrip.
> 
> https://reviewboard.mozilla.org/r/178284/#review184344
> 
> > This looks like it makes http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/browser/themes/osx/browser.css#905 redundant, except that you're missing the fallback background-color for OS X 10.9, and your selector looks less correct.
> 
> Yes, it's not redundant, but I should add the titlebar to that rule so that
> it includes the fallback color.

Ah, I think I was wrong! I'll update the patch.
(In reply to Johann Hofmann [:johannh] from comment #31)
> (In reply to Johann Hofmann [:johannh] from comment #30)
> > Comment on attachment 8906534 [details]
> > Bug 1349552 - Part 1 - Add optional drag space on top of the tabstrip.
> > 
> > https://reviewboard.mozilla.org/r/178284/#review184344
> > 
> > > This looks like it makes http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/browser/themes/osx/browser.css#905 redundant, except that you're missing the fallback background-color for OS X 10.9, and your selector looks less correct.
> > 
> > Yes, it's not redundant, but I should add the titlebar to that rule so that
> > it includes the fallback color.
> 
> Ah, I think I was wrong! I'll update the patch.

Meh, this doesn't work as nicely due to the titlebar being hidden in fullScreen mode. I'll just consolidate the rules and leave it at that.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc5821333286
Part 1 - Add optional drag space on top of the tabstrip. r=dao
https://hg.mozilla.org/integration/autoland/rev/9debb84b682c
Part 2 - Add an extra drag space setting to customize mode. r=nhnt11
https://hg.mozilla.org/integration/autoland/rev/81d48f464468
Part 3 - Add a test for drag space in customize mode. r=nhnt11
Depends on: 1399930
Depends on: 1401195
I verified this issue on Mac OS X 10.10, Windows 10 with FF Nightly 57.0a1(2017-09-18) and I can confirm the fix, there are settings for adding more drag space. I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
I do not see the option on Linux (Fedora 26, Gnome 3.24). Is it because Firefox does not support client side decorations bug 1283299 (there is always a titlebar on Gnome)?
Flags: needinfo?(jhofmann)
(In reply to Michal Stanke (Mozilla.cz) [:MikkCZ][:mstanke] (use needinfo) from comment #39)
> I do not see the option on Linux (Fedora 26, Gnome 3.24). Is it because
> Firefox does not support client side decorations bug 1283299 (there is
> always a titlebar on Gnome)?

Yes, on the other platforms drag space is also disabled if the native titlebar is showing.
Flags: needinfo?(jhofmann)
No longer depends on: 1401195
The option has no effects on maximized windows. IMHO this is not very logic since we added it as an option, so the user expects that it works everywhere and not only in resized ones.

Tested on Nightly 58 x64 on Win10
(In reply to Emanuele from comment #42)
> The option has no effects on maximized windows. IMHO this is not very logic
> since we added it as an option, so the user expects that it works everywhere
> and not only in resized ones.
> 
> Tested on Nightly 58 x64 on Win10

I agree that the experience isn't optimal in maximized Windows (we should probably just add a note in customize mode), can you make a bug about that?
Depends on: 1405282
We should also implement that for linux when Bug 1399611 (or 1283299) lands.
(In reply to Martin Stránský from comment #44)
> We should also implement that for linux when Bug 1399611 (or 1283299) lands.

I don't think this bug blocks it then. Please make a new bug for tracking enabling drag space for Linux and make it depend on either of these bugs.
No longer blocks: gtktitlebar
Depends on: 1406136
You need to log in before you can comment on or make changes to this bug.