1241326 - Resize handles to change viewport size on drag

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Grippers to allow for custom vertical and horizontal sizing of the viewport

Comment 1

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: resizing-viewport-ux.png

UX Notes:

For resizing a viewport I think we should use the same grippers as 
<textarea> grippers, with a slight modification. Since viewports can be
various colors (to the left here, they are the same color as the background
of the viewport) we should have a dropshadow on them:

text-shadow: 0 1px 2px 0  rgba(255, 255, 255, 0.78);

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

As discussed on IRC, we should also allow resizing in one dimension when grabbing the right or bottom edges of the viewport.

There won't be any extra gripper graphics, but the mouse cursor should change to the appropriate resize arrow when hovering the edge.  This is just like edge of an OS application window (at least on Mac).

Comment 3

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: resizing-viewport-ux.png

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1241326.patch

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1241326.patch

Comment 6

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: grippers.svg

SVG for handling resizing the viewport.

Comment 7

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8717013 [details] [diff] [review]
1241326.patch

Do you think you can get in the new svg before I review this?

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8717013 [details] [diff] [review]
1241326.patch

Review of attachment 8717013 [details] [diff] [review]:
-----------------------------------------------------------------

It feels quite nice overall!  A few tweaks to make, but I think we're getting close.

::: devtools/client/responsive.html/actions/viewports.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const { ADD_VIEWPORT, RESIZE_VIEWPORT, ROTATE_VIEWPORT } = require("./index");

Nit: probably more than 80 char

@@ +16,5 @@
>        type: ADD_VIEWPORT,
>      };
>    },
>  
> +   /**

Nit: indentation

::: devtools/client/responsive.html/components/browser.js
@@ +30,5 @@
>  
>      return dom.iframe(
>        {
> +        style: {
> +          pointerEvents: isResizing ? "none" : "auto",

Set a "resizing" class as appropriate, and then control this from CSS based on the class.  (Generally we're decided in our React DevTools component cabal to avoid the "inline styles" approach some React users go for.  Eventually we'll create per-component CSS files with an easy way to load them.)

::: devtools/client/responsive.html/components/viewport.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +/* global window */

Nit: I've been putting these ESLint modifier comments after the "use strict" block in responsive.html.  There appears to be no consistent choice in DevTools so far.

@@ +23,5 @@
> +    onViewportResize: PropTypes.func.isRequired,
> +  },
> +
> +  componentDidMount() {
> +    this.onResizeDrag = this.onResizeDrag.bind(this);

React logs a warning in the console that these binds are not needed, does it work without them?

@@ +43,5 @@
> +      ignoreY: false,
> +    };
> +  },
> +
> +  onResizeStart({target, clientX, clientY}) {

Nit: I think we've been using "({ " so far in this folder.  Should consider an ESLint rule if there's not one...

@@ +47,5 @@
> +  onResizeStart({target, clientX, clientY}) {
> +    window.addEventListener("mousemove", this.onResizeDrag, true);
> +    window.addEventListener("mouseup", this.onResizeStop, true);
> +
> +    this.setState({

Using `setState` like this to manage "component local" state is usually not what we want when using Redux, at least for state that applies to the whole application that you could imagine wants save / restore / undo.  (At least, that's how I think about it, I'm still learning too!)

There may still be cases where `setState` is appropriate with Redux when you have something that's purely a presentation concern of a single component and would never reasonably want to be preserved.  For example, recording every time step of an animation or transition in the Redux store could be overkill and `setState` might be okay for such a case.

This particular case kind of blurs the lines, as the width and height are part of our app state, but there's also other details while dragging.  Additionally future UI may wish to know about the width / height even while dragging, but it would be hidden from them if we only track it as component local state.

I think we should call `onViewportResize` for *all* changes to width / height, even while dragging.  The other pieces of this `setState` call are not app state, but just "how dragging works" so they seem fine to remain as component local in `setState`.

(A bit long winded, but hopefully it makes sense.  First time I've had to think through this issue myself.)

::: devtools/client/responsive.html/index.css
@@ -7,5 @@
>  }
>  
> -body {
> -  /* Only allow horizontal scrolling when more viewports are added */
> -  overflow-y: hidden;

Why is this being removed? (Please reply, don't just put it back.) :)

@@ +97,5 @@
>    border: 0;
>  }
> +
> +/**
> + * Viewport Resize Handlebars

Nit: Maybe "Handles"?

@@ +106,5 @@
> +  width: 20px;
> +  height: 20px;
> +  bottom: 0;
> +  right: 0;
> +  background: url('data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiBzdGFuZGFsb25lPSJubyI/Pg08IS0tIEdlbmVyYXRvcjogQWRvYmUgRmlyZXdvcmtzIENTNiwgRXhwb3J0IFNWRyBFeHRlbnNpb24gYnkgQWFyb24gQmVhbGwgKGh0dHA6Ly9maXJld29ya3MuYWJlYWxsLmNvbSkgLiBWZXJzaW9uOiAwLjYuMSAgLS0+DTwhRE9DVFlQRSBzdmcgUFVCTElDICItLy9XM0MvL0RURCBTVkcgMS4xLy9FTiIgImh0dHA6Ly93d3cudzMub3JnL0dyYXBoaWNzL1NWRy8xLjEvRFREL3N2ZzExLmR0ZCI+DTxzdmcgaWQ9IlVudGl0bGVkLVBhZ2UlMjAxIiB2aWV3Qm94PSIwIDAgNiA2IiBzdHlsZT0iYmFja2dyb3VuZC1jb2xvcjojZmZmZmZmMDAiIHZlcnNpb249IjEuMSINCXhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgeG1sbnM6eGxpbms9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGxpbmsiIHhtbDpzcGFjZT0icHJlc2VydmUiDQl4PSIwcHgiIHk9IjBweCIgd2lkdGg9IjZweCIgaGVpZ2h0PSI2cHgiDT4NCTxnIG9wYWNpdHk9IjAuMzAyIj4NCQk8cGF0aCBkPSJNIDYgNiBMIDAgNiBMIDAgNC4yIEwgNCA0LjIgTCA0LjIgNC4yIEwgNC4yIDAgTCA2IDAgTCA2IDYgTCA2IDYgWiIgZmlsbD0iIzAwMDAwMCIvPg0JPC9nPg08L3N2Zz4=');

This "L shape" is kind of neat actually...  Anyhow, I'll defer to Helen's shapes.

Either way, let's move it to a standalone file.

@@ +115,5 @@
> +  box-sizing: border-box;
> +  cursor: se-resize;
> +}
> +
> +.viewport-horizontal-resize-handle {

Okay, I now see what you mean about them overlapping the scrollbars.

However, I was imagining these edge grippers could actually be *outside* the viewport, which seems like it would resolve the overlap problem.

So, for this one, you could nudge it outside with something like `right: -9px`, so it still covers the frame border.

@@ +117,5 @@
> +}
> +
> +.viewport-horizontal-resize-handle {
> +  position: absolute;
> +  width: 10px;

Maybe it's okay to be a little thinner, since we're just trying to make a pseudo-edge you can grab?

@@ +118,5 @@
> +
> +.viewport-horizontal-resize-handle {
> +  position: absolute;
> +  width: 10px;
> +  height: calc(100% - 40px);

I don't think we want this height to have to account for the height of the toolbar.  Let's add a div around the browser and grippers, "viewport-body" maybe.  (I think it can stay in the viewport component, probably does not need it's own component.)

::: devtools/client/responsive.html/reducers/viewports.js
@@ +16,5 @@
>  const INITIAL_VIEWPORT = {
>    id: nextViewportId++,
>    width: 320,
>    height: 480,
> +  minWidth: 320,

I mentioned the existence of 280px width device in the current device DB to Helen, and I believe she was okay using that as the min width.

Also, these numbers don't feel like app state to me since they aren't meant to change.  I believe they are only accessed in the viewport component, so they can be consts inside that component.

::: devtools/client/responsive.html/types.js
@@ +22,5 @@
>  
>    // The height of the viewport
>    height: PropTypes.number,
>  
> +  // The min width of the viewport

As mentioned in the reducer, these would go away since they aren't app state.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> @@ +47,5 @@
> > +  onResizeStart({target, clientX, clientY}) {
> > +    window.addEventListener("mousemove", this.onResizeDrag, true);
> > +    window.addEventListener("mouseup", this.onResizeStop, true);
> > +
> > +    this.setState({
> 
> Using `setState` like this to manage "component local" state is usually not
> what we want when using Redux, at least for state that applies to the whole
> application that you could imagine wants save / restore / undo.  (At least,
> that's how I think about it, I'm still learning too!)
> 
> There may still be cases where `setState` is appropriate with Redux when you
> have something that's purely a presentation concern of a single component
> and would never reasonably want to be preserved.  For example, recording
> every time step of an animation or transition in the Redux store could be
> overkill and `setState` might be okay for such a case.
> 
> This particular case kind of blurs the lines, as the width and height are
> part of our app state, but there's also other details while dragging. 
> Additionally future UI may wish to know about the width / height even while
> dragging, but it would be hidden from them if we only track it as component
> local state.
> 
> I think we should call `onViewportResize` for *all* changes to width /
> height, even while dragging.  The other pieces of this `setState` call are
> not app state, but just "how dragging works" so they seem fine to remain as
> component local in `setState`.

For future reference, here's a good guide on this topic:

https://facebook.github.io/react/docs/interactivity-and-dynamic-uis.html#what-should-go-in-state

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8717013 [details] [diff] [review]
1241326.patch

Review of attachment 8717013 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/responsive.html/actions/viewports.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const { ADD_VIEWPORT, RESIZE_VIEWPORT, ROTATE_VIEWPORT } = require("./index");

This was actually < 80 char, but I moved it to separate lines anyways.

@@ +16,5 @@
>        type: ADD_VIEWPORT,
>      };
>    },
>  
> +   /**

Fixed.

::: devtools/client/responsive.html/components/browser.js
@@ +30,5 @@
>  
>      return dom.iframe(
>        {
> +        style: {
> +          pointerEvents: isResizing ? "none" : "auto",

Fixed. Added .browser-resizing class

::: devtools/client/responsive.html/components/viewport.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +/* global window */

Fixed. Added a new line between the header comment and ESLint comment. This is consistent with responsive.html/index.js

@@ +23,5 @@
> +    onViewportResize: PropTypes.func.isRequired,
> +  },
> +
> +  componentDidMount() {
> +    this.onResizeDrag = this.onResizeDrag.bind(this);

React actually autobinds. So, removed these bindings.

@@ +43,5 @@
> +      ignoreY: false,
> +    };
> +  },
> +
> +  onResizeStart({target, clientX, clientY}) {

Fixed. Added the spacing between args and { }

::: devtools/client/responsive.html/index.css
@@ -7,5 @@
>  }
>  
> -body {
> -  /* Only allow horizontal scrolling when more viewports are added */
> -  overflow-y: hidden;

We talked about this. Will file a new bug as necessary.

@@ +97,5 @@
>    border: 0;
>  }
> +
> +/**
> + * Viewport Resize Handlebars

Fixed. Used "Handles"

@@ +106,5 @@
> +  width: 20px;
> +  height: 20px;
> +  bottom: 0;
> +  right: 0;
> +  background: url('data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiBzdGFuZGFsb25lPSJubyI/Pg08IS0tIEdlbmVyYXRvcjogQWRvYmUgRmlyZXdvcmtzIENTNiwgRXhwb3J0IFNWRyBFeHRlbnNpb24gYnkgQWFyb24gQmVhbGwgKGh0dHA6Ly9maXJld29ya3MuYWJlYWxsLmNvbSkgLiBWZXJzaW9uOiAwLjYuMSAgLS0+DTwhRE9DVFlQRSBzdmcgUFVCTElDICItLy9XM0MvL0RURCBTVkcgMS4xLy9FTiIgImh0dHA6Ly93d3cudzMub3JnL0dyYXBoaWNzL1NWRy8xLjEvRFREL3N2ZzExLmR0ZCI+DTxzdmcgaWQ9IlVudGl0bGVkLVBhZ2UlMjAxIiB2aWV3Qm94PSIwIDAgNiA2IiBzdHlsZT0iYmFja2dyb3VuZC1jb2xvcjojZmZmZmZmMDAiIHZlcnNpb249IjEuMSINCXhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgeG1sbnM6eGxpbms9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGxpbmsiIHhtbDpzcGFjZT0icHJlc2VydmUiDQl4PSIwcHgiIHk9IjBweCIgd2lkdGg9IjZweCIgaGVpZ2h0PSI2cHgiDT4NCTxnIG9wYWNpdHk9IjAuMzAyIj4NCQk8cGF0aCBkPSJNIDYgNiBMIDAgNiBMIDAgNC4yIEwgNCA0LjIgTCA0LjIgNC4yIEwgNC4yIDAgTCA2IDAgTCA2IDYgTCA2IDYgWiIgZmlsbD0iIzAwMDAwMCIvPg0JPC9nPg08L3N2Zz4=');

Fixed. Used Helen's gripper

@@ +115,5 @@
> +  box-sizing: border-box;
> +  cursor: se-resize;
> +}
> +
> +.viewport-horizontal-resize-handle {

Fixed. Positioned the handles to the edge of the viewport.

@@ +117,5 @@
> +}
> +
> +.viewport-horizontal-resize-handle {
> +  position: absolute;
> +  width: 10px;

Fixed. Used a width/height of 5px

@@ +118,5 @@
> +
> +.viewport-horizontal-resize-handle {
> +  position: absolute;
> +  width: 10px;
> +  height: calc(100% - 40px);

Did not have a need to wrap everything in viewport-body since we moved everything outside the viewport, and wanted handles to emulate a window.

::: devtools/client/responsive.html/reducers/viewports.js
@@ +16,5 @@
>  const INITIAL_VIEWPORT = {
>    id: nextViewportId++,
>    width: 320,
>    height: 480,
> +  minWidth: 320,

Fixed. Added consts

::: devtools/client/responsive.html/types.js
@@ +22,5 @@
>  
>    // The height of the viewport
>    height: PropTypes.number,
>  
> +  // The min width of the viewport

Fixed. Removed these types

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1241326.patch [w/ Component State]

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1241326.patch [1.0]

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1241326.patch [2.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=225b78b1da7b

Comment 14

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8718179 [details] [diff] [review]
1241326.patch [2.0]

Looking at the grippers, they do seem way too light. We're never going to be able to choose one color that works everywhere, but we should at least choose a color that works on light backgrounds. 

Let's go with the <textarea> gripper color. Looks like it's #A5A5A5. Let's keep the text-shadow on the element, though, for darker colored backgrounds.

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1241326.patch [3.0]

- Applied Helen's feedback
- I moved the resize handle, toolbar and browser of the viewport into a ViewportContainer. This would allow for the ViewportLabel (for editable width / height to be under this container. If we moved the Browser + Resize Handles into a ViewportBody, the height of the horizontal resize handle would not reach the toolbar.

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1241326.patch [4.0]

- Updated the svg to remove the filter after talking to Helen

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718428 [details] [diff] [review]
1241326.patch [4.0]

Review of attachment 8718428 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this looks good overall.  Just a few tweaks left.

::: devtools/client/responsive.html/app.js
@@ +33,5 @@
>      return Viewports({
>        location,
>        viewports,
>        onRotateViewport: id => dispatch(rotateViewport(id)),
> +      onViewportResize: (id, width, height) =>

Name this `onResizeViewport` to fit the previous pattern of `onVerbNoun`

::: devtools/client/responsive.html/components/browser.js
@@ +29,5 @@
>      } = this.props;
>  
> +    let className = "browser";
> +    if (isResizing) {
> +      className += " browser-resizing";

Nit: Let's just call the class `resizing`, and then style it with:

.browser.resizing {}

I think think feels a little better to me when styling visual states like "enabled", "resizing", etc.

::: devtools/client/responsive.html/components/viewport-container.js
@@ +23,5 @@
> +  propTypes: {
> +    location: Types.location.isRequired,
> +    viewport: PropTypes.shape(Types.viewport).isRequired,
> +    onRotateViewport: PropTypes.func.isRequired,
> +    onViewportResize: PropTypes.func.isRequired,

Name this `onResizeViewport` to fit the previous pattern of `onVerbNoun`

@@ +27,5 @@
> +    onViewportResize: PropTypes.func.isRequired,
> +  },
> +
> +  getInitialState() {
> +    return {

Add `isResizing: false`.  Currently there is an error in the console:

Failed propType: Required prop `isResizing` was not specified in `Browser`. Check the render method of `ViewportContainer`.

I think that may fix it.  If not, find out what does. :)

::: devtools/client/responsive.html/components/viewport.js
@@ +25,5 @@
>      let {
>        location,
>        viewport,
>        onRotateViewport,
> +      onViewportResize,

Name this `onResizeViewport` to fit the previous pattern of `onVerbNoun`

@@ +35,2 @@
>        },
> +      ViewportContainer({

Okay, I am on board with having a component like this, esp. since you will soon add the label below.  However, the name ViewportContainer is a bit too generic / meaningless.

Let's try to think of a name that describes it's purpose.  Since it has a lot to do with resizing, maybe "ResizableViewport"?

In the future, we could even make it more of a reusable "Resizable" component (not specific to viewport) where you pass in the children (Browser, etc.) as props and it only thinks about resizing.  Anyway, probably not worth going that far yet, just needs a better name.

::: devtools/client/responsive.html/components/viewports.js
@@ +38,5 @@
>            key: viewport.id,
>            location,
>            viewport,
>            onRotateViewport: () => onRotateViewport(viewport.id),
> +          onViewportResize: (width, height) =>

Name this `onResizeViewport` to fit the previous pattern of `onVerbNoun`

::: devtools/client/responsive.html/images/moz.build
@@ +4,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  DevToolsModules(
> +    'grippers.svg',

Seems to work well on dark and light content!

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1241326.patch [5.0]

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/44b9645e7b4e

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44b9645e7b4e

Comment 21

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: viewport.png

I managed to test the grippers that allow custom vertical and horizontal sizing of the viewport on Firefox 47.0a1 (2016-02-18) across platforms[1] and I noticed a few potential issues:
1. The grippers overlaps the scrolling bar if there is only the vertical or the horizontal scrolling bar available (see the attached screenshot).
2. The grippers are not accessible if first the viewport is enlarged horizontally and than rotated by using the Landscape/Portrait button(see the video below).

Should I file new bugs for the issues described above?   

[1] WIndows 10 x86, Ubuntu 14.04 x86, Mac OS X 10.9.5

Comment 22

[Mihai Boldan, Desktop QA [:mboldan]]

In the attached video it can also be observed that the when the viewport is resized, the mouse cursor doesn't stick to the grippers. This issue is also reproducible on all tested platforms.
Viewport video - http://screencast.com/t/2e7ZrMV8y

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mihai Boldan, QA [:mboldan] from comment #21)
> Created attachment 8721316 [details]
> viewport.png
> 
> I managed to test the grippers that allow custom vertical and horizontal
> sizing of the viewport on Firefox 47.0a1 (2016-02-18) across platforms[1]
> and I noticed a few potential issues:
> 1. The grippers overlaps the scrolling bar if there is only the vertical or
> the horizontal scrolling bar available (see the attached screenshot).

This is expected for now.  In bug 1241587 we will convert to floating scrollbars on all platforms, so it should be less noticeable after that.  However, OS X should already have floating scrollbars by default.  The floating scrollbars could still have some overlay near the corner, but I think that's probably okay.  How did it look on OS X for you?

> 2. The grippers are not accessible if first the viewport is enlarged
> horizontally and than rotated by using the Landscape/Portrait button(see the
> video below).

Hmm...  Let's file this.  I am not sure what we'll do about it, but we can at least track the issue.

(In reply to Mihai Boldan, QA [:mboldan] from comment #22)
> In the attached video it can also be observed that the when the viewport is
> resized, the mouse cursor doesn't stick to the grippers. This issue is also
> reproducible on all tested platforms.
> Viewport video - http://screencast.com/t/2e7ZrMV8y

Yes, let's file this.  We should at least investigate if it can be improved.

Thanks!

Comment 24

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: scrollbar on Mac OS X.png

Here is how the scrollbar is displayed on Mac OS X 10.9.5. 
My opinion is that at least the grippers should be more opaque in order to be more visible. It can be observed in the attachment that the gripper is hardly visible while the scrollbar overlaps it.     
Also I logged Bug 1250133 and Bug 1250138 based on Comment 23.
If the problem described above is an expected result, I will mark this bug Verified-Fixed.

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mihai Boldan, QA [:mboldan] from comment #24)
> Created attachment 8721996 [details]
> scrollbar on Mac OS X.png
> 
> Here is how the scrollbar is displayed on Mac OS X 10.9.5. 
> My opinion is that at least the grippers should be more opaque in order to
> be more visible. It can be observed in the attachment that the gripper is
> hardly visible while the scrollbar overlaps it.

This looks like scrollbars are forced on in OS X system preferences, which makes them larger than the "floating" version of system scrollbars.  Go to System Preferences -> General -> Show scroll bars.  I am guessing it is currently set to "Always".  Choose some option that is not "Always" (I believe "Automatically based on mouse or trackpad" is the default).  You may need to restart Firefox for the change to take effect in RDM.

Then you should see thinner scrollbars in the viewport.  Is that what happens?

Comment 26

[Mihai Boldan, Desktop QA [:mboldan]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #25)

> This looks like scrollbars are forced on in OS X system preferences, which
> makes them larger than the "floating" version of system scrollbars.  Go to
> System Preferences -> General -> Show scroll bars.  I am guessing it is
> currently set to "Always".  Choose some option that is not "Always" (I
> believe "Automatically based on mouse or trackpad" is the default).  You may
> need to restart Firefox for the change to take effect in RDM.
> 
> Then you should see thinner scrollbars in the viewport.  Is that what
> happens?

I've checked on Mac the settings of the scroll bars and it was set as "Automatically based on mouse or trackpad". 
I've also changed to 'Always' and there is no change to the viewport scrollbars.(I've restarted Firefox after changing the 'Show scroll bars' settings).

Is there anything that I should verify?

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Scrollbars Recording [jryans]

(In reply to Mihai Boldan, QA [:mboldan] from comment #26)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #25)
> 
> > This looks like scrollbars are forced on in OS X system preferences, which
> > makes them larger than the "floating" version of system scrollbars.  Go to
> > System Preferences -> General -> Show scroll bars.  I am guessing it is
> > currently set to "Always".  Choose some option that is not "Always" (I
> > believe "Automatically based on mouse or trackpad" is the default).  You may
> > need to restart Firefox for the change to take effect in RDM.
> > 
> > Then you should see thinner scrollbars in the viewport.  Is that what
> > happens?
> 
> I've checked on Mac the settings of the scroll bars and it was set as
> "Automatically based on mouse or trackpad". 
> I've also changed to 'Always' and there is no change to the viewport
> scrollbars.(I've restarted Firefox after changing the 'Show scroll bars'
> settings).
> 
> Is there anything that I should verify?

Hmm, that's a bit surprising.  I've attached a recording of the behavior I am seeing with the default "Show scroll bars: Automatically..." setting.  They start out as floating scrollbars, and only become "larger" overlay scrollbars if you happen to hover over one of them.

Is this what you see, or are they constantly present in the "larger" format?

Comment 28

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: scrollbars on Mac.mov

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27)

> Hmm, that's a bit surprising.  I've attached a recording of the behavior I
> am seeing with the default "Show scroll bars: Automatically..." setting. 
> They start out as floating scrollbars, and only become "larger" overlay
> scrollbars if you happen to hover over one of them.
> 
> Is this what you see, or are they constantly present in the "larger" format?

I've also attached a video to observe the behavior from my side.
I observed 3 differences between this 2 attachments:
- on my side, the scrollbars doesn't get larger, only darker when hoovering over one of them
- the scrollbars doesn't go all the way down(vertical scrollbar) or all the way to the right(horizontal scrollbar) if they are both displayed.
- on your side, if the page is not scrolled or the mouse is not hovering the scrollbars, than the scrollbars are not displayed.(This makes the grippers more visible).

Note that the tests were performed on Latest Nightly build with the default "Show scroll bars: Automatically based on mouse or trackpad" setting.

Is there anything that should be checked?

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mihai Boldan, QA [:mboldan] from comment #28)
> Created attachment 8723542 [details]
> scrollbars on Mac.mov
> 
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27)
> 
> > Hmm, that's a bit surprising.  I've attached a recording of the behavior I
> > am seeing with the default "Show scroll bars: Automatically..." setting. 
> > They start out as floating scrollbars, and only become "larger" overlay
> > scrollbars if you happen to hover over one of them.
> > 
> > Is this what you see, or are they constantly present in the "larger" format?
> 
> I've also attached a video to observe the behavior from my side.
> I observed 3 differences between this 2 attachments:
> - on my side, the scrollbars doesn't get larger, only darker when hoovering
> over one of them
> - the scrollbars doesn't go all the way down(vertical scrollbar) or all the
> way to the right(horizontal scrollbar) if they are both displayed.
> - on your side, if the page is not scrolled or the mouse is not hovering the
> scrollbars, than the scrollbars are not displayed.(This makes the grippers
> more visible).
> 
> Note that the tests were performed on Latest Nightly build with the default
> "Show scroll bars: Automatically based on mouse or trackpad" setting.
> 
> Is there anything that should be checked?

Hmm, well, that is definitely different from my video!  I am not sure why you're seeing different scrollbars at the moment.  Let's file a bug then to track this for further investigation.  The goal is to eventually have floating scrollbars in the viewport on all OSes, however I know we definitely aren't there yet.  In my testing we currently will only have them on OS X, so it's expected for the moment that they are not present on Windows and Linux.  But from your video, you are not seeing floating scrollbars on OS X either.  So, let's file a bug about your case where you are getting "full" scrollbars on OS X instead of floating, and we can try to work out why with more discussion in the new bug.

Comment 30

[Mihai Boldan, Desktop QA [:mboldan]]

I logged Bug 1251591 for the issue from Comment 29.
Since the issue found while testing this bug was logged separately, I'm marking this bug Verified-Fixed.