1239441 - Landscape / Portrait button that rotates the viewport

Status: VERIFIED FIXED

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

Comment 1

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

Attachment: rotate-viewport.svg

Hasn't been tested on a non-300dpi machine for fuzz yet.

Comment 2

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

Attachment: rotate-viewport.svg

Comment 3

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

Attachment: rotating-viewport-ux-notes.png

Transcribing the UX notes here (helps to see the image too):

- Attached icon rotates the single viewport. Fill in mockups is #696969, #468EE5 when active. It's roughly 3px from the edge in the mockup, but I'm fine with increasing that so long as right/left padding for viewport stays consistent.
- Controls should stay in place (top) after rotate. (See attached screenshot.)
- I think we should implement the switch without any sort of animation to start and consider an animation for the rotate in a later bug at a later date.

Comment 4

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

Attachment: rotate-viewport.svg

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8712352 [details]
rotate-viewport.svg

Looks nice although it's a bit blurry on Windows !

Comment 6

[Tim Nguyen :ntim]

Attachment: fuzzy screenshot icon.PNG

Screenshot on Windows (1x display)

Comment 7

[Tim Nguyen :ntim]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #3)
> Created attachment 8712269 [details]
> rotating-viewport-ux-notes.png
> 
> Transcribing the UX notes here (helps to see the image too):
Nice work.

> - Controls should stay in place (top) after rotate. (See attached
> screenshot.)
> - I think we should implement the switch without any sort of animation to
> start and consider an animation for the rotate in a later bug at a later
> date.
Makes sense, there's a lot to consider if we add an animation, for example how the titlebar would interact with the animation, or if other viewports would move along during the animation (that would be jarring)

Comment 8

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

Attachment: mac-rotate-1x.png

Also kind of fuzzy on Mac at 1x, but probably hard to fix that with such a small icon...?

Comment 9

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

Don't want to override Gabe's "Assigned To" field, but I'll work on getting an updated svg asset attached today.

Comment 10

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

Attachment: rotate-viewport.svg

Ended up having to redesign the icon to avoid the fuzz :( oh well.

This icon is more like a play/pause button. If you have an upright viewport, it will display a sideways phone, which you can press which will cause the viewport to turn sideways. If the phone is sideways, it will show an upright phone, and pressing that will cause the viewport to turn upright.

Here's a codepen showing the two states/Chris Coyier's way of using <symbol> with <svg>, if that's helpful: http://codepen.io/helenvholmes/pen/OMwNKg

Gabe, let me know if you need me to update the UX notes or if you feel you've got it based on this description.

Comment 11

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

Attachment: 1239441.patch

Comment 12

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

Attachment: rotate.svg

Comment 13

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

Attachment: rotate-sideways.svg

Since <symbol> was giving us woe, I think that it's just easiest to have the two svgs separated out.

Comment 14

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

Comment on attachment 8714666 [details] [diff] [review]
1239441.patch

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

Good work overall, glad you found out how to do event handlers.  Just some small tweaks to make mainly.

::: devtools/client/responsive.html/components/viewport.js
@@ +24,2 @@
>      } = this.props;
>      // Additional elements will soon appear here around the Browser, like drag

This comment can be removed once you create and add the toolbar component.

@@ +30,5 @@
>        },
>        dom.div({
>          className: "viewport-header",
> +      },
> +      dom.button({

Create a new "ViewportToolbar" component and move the header plus the button into there.

@@ +32,5 @@
>          className: "viewport-header",
> +      },
> +      dom.button({
> +        className: "viewport-rotate-button",
> +        onClick: () => onRotateViewport()

This can just be `onClick: onRotateViewport`

::: devtools/client/responsive.html/index.css
@@ +24,5 @@
>    border-bottom: 1px solid var(--theme-splitter-color);
>    color: var(--theme-body-color-alt);
>    height: 16px;
>  }
> +.viewport-rotate-button {

Like you said in the meeting, we should work out how this works for dark theme too.

@@ +33,5 @@
> +  width: 16px;
> +  height: 16px;
> +  opacity: 0.8;
> +  float: right;
> +  background-image: url("chrome://devtools/skin/images/rotate-viewport.svg");

I know this differs from most existing tools, but I'd like to keep the images within the tool, and also avoid chrome://, more like web site would.

Create an images directory inside the tool dir, add the image there, add moz.build listing in with DevToolsModules.  Then use "./images/X" here (CSS is already loaded as resource://).

::: devtools/client/responsive.html/reducers/viewports.js
@@ +8,3 @@
>  const INITIAL_VIEWPORTS = [];
>  const INITIAL_VIEWPORT = {
> +  id: nextViewportId++,

Add `id` to `viewport` in types.js so it's validated for people using that type.

Also, mark it `isRequired` in types.js, since we should always have one in every case.  (Other properties are only isRequired at the component level when they are needed.)

@@ +18,5 @@
>        return viewports;
>      }
>      return [...viewports, INITIAL_VIEWPORT];
>    },
> +  [ROTATE_VIEWPORT](viewports, action) {

Nit: { id } feels a little more obvious, so you can know what action contains.

@@ +31,5 @@
> +        height: viewport.width
> +      };
> +    });
> +  }
> +

Splinter has a very confused view of the whitespace in this file... anyway, it seems fine in the actual patch.

::: devtools/client/responsive.html/test/unit/test_rotate_viewport.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test rotating the viewport.

Great!  As we get further, I'd like to also have browser integration tests that actually click things as well as the unit tests like this.

But, we need frame script support to be able to check the size the real way, so it's fine to defer that until my platform work is done.

At least with Redux, we're still about to write unit tests like these!

::: devtools/client/themes/images/rotate-viewport.svg
@@ +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/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">

I hope we find a way to use an icon more like this one, I really like the arrows!

Comment 15

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

Comment on attachment 8712252 [details]
rotate-viewport.svg

Marking this one as non-obsolete since we'd like to use this icon (it's more descriptive) for high-dpi devices.

Comment 16

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

Attachment: 1239441.patch

Comment 17

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

Attachment: 1239441.patch

Comment 18

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

Attachment: 1239441.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47f8e6920899

Comment 19

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

Attachment: 1239441.patch

Comment 20

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

Attachment: 1239441.patch

Comment 21

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

Comment on attachment 8717003 [details] [diff] [review]
1239441.patch

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

::: devtools/client/responsive.html/components/viewport-toolbar.js
@@ +23,5 @@
> +      viewport,
> +      onRotateViewport,
> +    } = this.props;
> +
> +    let buttonId = viewport.isRotated ?

Assuming we do decide to keep the changing image at 1x, let's give the button a stable class of `viewport-rotate-button` (in addition to the general toolbar button class).  We shouldn't use an ID since there would be more than one with multiple viewports.

Also, let's handle the change in images as an extra `sideways` or `rotated` class that's added based on larger of height vs. width.

::: devtools/client/responsive.html/index.css
@@ +33,5 @@
>    white-space: nowrap;
>  }
>  
>  /**
> + * Viewport Toolbar

Move this toolbar section below the .viewport rule set, since it is inside that component.

@@ +43,5 @@
> +  color: var(--theme-body-color-alt);
> +  height: 18px;
> +}
> +
> +.viewport-toolbar-button {

I'd prefer to use flexbox inside the toolbar if we can, especially as we add more things like sizes which will be center etc.  Can we switch to that now?

@@ +56,5 @@
> +  background: transparent;
> +}
> +
> +.viewport-toolbar-button:hover {
> +  opacity: 1;

I am not seeing the opacity change on hover mentioned in Helen's notes, but maybe I missed it?  We should get her input on this.

@@ +60,5 @@
> +  opacity: 1;
> +}
> +
> +#viewport-rotate-sideways-button {
> +  mask-image: url("./images/rotate-sideways.svg");

I really find these 1x icons uninituitive.  On the one hand, people may just try them and find out what they do...  But the initial impression makes it feel like they will open a sidebar or some other strange thing.

At the moment, I am tempted to say we should just use the 2x icon even if it's fuzzy.  Helen, what do you think?

@@ +69,5 @@
> +}
> +
> +#viewport-rotate-sideways-button,
> +#viewport-rotate-button {
> +  background-color: var(--theme-body-color-alt);

Shouldn't this be #468EE5?

::: devtools/client/responsive.html/reducers/viewports.js
@@ +34,5 @@
> +
> +      return Object.assign({}, viewport, {
> +        width: viewport.height,
> +        height: viewport.width,
> +        isRotated: !viewport.isRotated,

I don't think we want this additional "is rotated" state.  The current images we toggle at 1x are based on whether we're currently portrait or landsacpe, so we can show the opposite view.

However, you could easily resize (once that's possible) in a way that changes the orientation without using the rotate button that would make this state incorrect.

So, I think we should remove the isRotated, and determine the image in the component based on whether width or height is larger.

Comment 22

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

Comment on attachment 8717003 [details] [diff] [review]
1239441.patch

This looks awesome from a ui standpoint. I have one small nit that I think isn't work feedback+-ing on this patch but I'll put here anyway:

In dark mode, let's change the text-shadow from (155, 155, 155, 0.26) to (105, 105, 105, 0.26).

Comment 23

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

Attachment: 1239441.patch

- Removed the isRotated state since we will just use the original icon
- Got the okay from Helen regarding the opacity
- #468EE5 is the active color, and I am using var(--theme-body-color-alt) since this is the color we currently use in the our devtools toolbar. Helen, can you check if var(--theme-body-color-alt) matches your expectations? I assumed this variable will be updated to match your design when ntim does the theme refresh bug.

Comment 24

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

Helen, see the last bullet in comment 23.

Comment 25

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

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #23)
> Created attachment 8717632 [details] [diff] [review]
> 1239441.patch
> - #468EE5 is the active color, and I am using var(--theme-body-color-alt)
> since this is the color we currently use in the our devtools toolbar. Helen,
> can you check if var(--theme-body-color-alt) matches your expectations? I
> assumed this variable will be updated to match your design when ntim does
> the theme refresh bug.

For its regular state, please use --theme-body-color. For the :active state, please use --theme-selection-background.

Even after bug 1242709 lands, the blues won't match my designs—seems like that blue's used globally all over Firefox, so Brian's suggested holding off changing it for a different bug.

Comment 26

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

Attachment: 1239441.patch [1.0]

Comment 27

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

Comment on attachment 8717917 [details] [diff] [review]
1239441.patch [1.0]

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

Great, this looks good to me.  Thanks for working on it.

::: devtools/client/responsive.html/components/viewport-toolbar.js
@@ +24,5 @@
> +      {
> +        className: "viewport-toolbar",
> +      },
> +      dom.button({
> +        className: "viewport-rotate-button viewport-toolbar-button ",

Nit: Remove extra space in `...button "`

::: devtools/client/responsive.html/index.css
@@ +80,3 @@
>    height: 16px;
> +  opacity: 0.8;
> +  background: transparent;

I think this has no effect?  Isn't it overridden by the next line?

Comment 28

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

Attachment: 1239441.patch [2.0]

Comment 29

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5411fe99102a

Comment 30

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

Comment on attachment 8718008 [details] [diff] [review]
1239441.patch [2.0]

This looks good! One small nit: can we put a transition on the buttons from here on out for hovers and such? I'd suggest:

<css-property> 0.25s ease;

It'll be really slight, it'll just seem more thought-through.

Comment 31

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

Attachment: 1239441.patch [3.0]

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/86eab7abbd7d

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86eab7abbd7d

Comment 34

[u459114]

Comment on attachment 8718097 [details] [diff] [review]
1239441.patch [3.0]

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

::: devtools/client/responsive.html/index.css
@@ +92,5 @@
> +  opacity: 1;
> +}
> +
> +.viewport-rotate-button {
> +  mask-image: url("./images/rotate-viewport.svg");

I noticed this change while working on bug 1243734
By disable all mask-xxx properties, I will see a TEST-UNEXPECTED-FAIL while running mochitest:

79 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_parsable_css.js | Got error message for jar:file:///builds/slave/test/build/application/firefox/browser/omni.ja!/chrome/devtools/modules/devtools/client/responsive.html/index.css: Unknown property 'mask-image'. Declaration dropped. -

So, before we default enable mask longhands, please still use mask, instead of mask-image
.viewport-rotate-button {
  mask: url("./images/rotate-viewport.svg");

Should I file another issue to change this property?

Comment 35

[Mihai Boldan, Desktop QA [:mboldan]]

I've tested the Landscape / Portrait button that rotates the viewport on Firefox 47.0a1 (2016-02-18) across platforms[1] and I observed 2 potential issues:
- I noticed that the button has the same color in both states, landscape or portrait.
- Also, if I duplicate the node from the inspector and there are 2 viewports displayed, the Landscaoe/Portrait button works only for the first viewport. 

Should I file new bugs for the issues described above, or this is expected behavior?

Comment 36

[Mihai Boldan, Desktop QA [:mboldan]]

Here are the platforms used for testing - [1] WIndows 10 x86, Ubuntu 14.04 x86, Mac OS X 10.9.5

Comment 37

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

(In reply to C.J. Ku[:cjku] from comment #34)
> Comment on attachment 8718097 [details] [diff] [review]
> 1239441.patch [3.0]
> 
> Review of attachment 8718097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/responsive.html/index.css
> @@ +92,5 @@
> > +  opacity: 1;
> > +}
> > +
> > +.viewport-rotate-button {
> > +  mask-image: url("./images/rotate-viewport.svg");
> 
> I noticed this change while working on bug 1243734
> By disable all mask-xxx properties, I will see a TEST-UNEXPECTED-FAIL while
> running mochitest:
> 
> 79 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/browser_parsable_css.js | Got error
> message for
> jar:file:///builds/slave/test/build/application/firefox/browser/omni.ja!/
> chrome/devtools/modules/devtools/client/responsive.html/index.css: Unknown
> property 'mask-image'. Declaration dropped. -
> 
> So, before we default enable mask longhands, please still use mask, instead
> of mask-image
> .viewport-rotate-button {
>   mask: url("./images/rotate-viewport.svg");
> 
> Should I file another issue to change this property?

Yes, please file a new bug.  I am happy to review the change.

Comment 38

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

(In reply to Mihai Boldan, QA [:mboldan] from comment #35)
> - I noticed that the button has the same color in both states, landscape or
> portrait.

That's expected, there is no "enabled" state of the button currently.  There should be an opacity change when hovering over the button, and also a color change while clicking on the button.  After that, it return to the original appearance.  If you see something different, please file.

> - Also, if I duplicate the node from the inspector and there are 2 viewports
> displayed, the Landscaoe/Portrait button works only for the first viewport.

This is expected due to the way React (the web framework we are using) tracks DOM nodes, I believe.  It tracks which node is which based on attributes it adds.  So, duplicate a node in the inspector will probably confuse it.  For now, we aren't focused on multiple viewports, so we can ignore this for the moment.

Thanks!

Comment 39

[Mihai Boldan, Desktop QA [:mboldan]]

I logged Bug 1250124 for the issue found related to the implemented button.
Beside the found issue, the button looks good, so I'm marking this issue Verified-Fixed.