1266414 - Implement the design spec for the device modal

Status: VERIFIED FIXED

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

Description

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

Extracted from https://bugzilla.mozilla.org/show_bug.cgi?id=1241720#c13:
- Original design spec showed the global toolbar and a bit of the viewport peaking above the modal to ground the user. Can we get that back in with a margin-top of some sort? 
- I think it'd be nice to have the modal 'appear' in some way. I'm asking Fang for some more info but if we could reverse-engineer the "slide-in (bottom)" effect from here I think it would add a lot: http://tympanus.net/Development/ModalWindowEffects/

Extracted from https://bugzilla.mozilla.org/show_bug.cgi?id=1241720#c15:
We also chatted about the potential for the viewport being larger than the modal and what should happen in that situation. I think if the viewport is (for whatever reason) very large, the modal should hug the top, the viewport should blur out, and pointer events for the viewport should freeze until the device modal is closed: http://cl.ly/1w43071f0R1u

Summary:
- Add a blur effect when the modal is open for the content behind the modal
- Reposition the modal
- Add a transition effect when the modal is opening
- Turn off pointer events for the viewports

Comment 2

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

For completeness, here is Helen's UI feedback from the device modal bug 1241720 comment 15:

Helen says:

"We also chatted about the potential for the viewport being larger than the modal and what should happen in that situation. I think if the viewport is (for whatever reason) very large, the modal should hug the top, the viewport should blur out, and pointer events for the viewport should freeze until the device modal is closed: http://cl.ly/1w43071f0R1u"

Comment 3

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

Attachment: modal.patch

Like I said in standup, this doesn't cover all of the items listed in the bug—I wasn't sure how to handle the overlay on top of the iframe (sure it's possible, just wasn't sure how to myself). (Thus, pointer events are still on.)

Comment 4

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

Comment on attachment 8758319 [details] [diff] [review]
modal.patch

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

Should we also implement click outside modal and / or press escape key to exit the modal?

::: devtools/client/responsive.html/index.css
@@ +44,1 @@
>    height: 100vh;

While we're here, I think it makes sense to remove the following styles from #app:

  height: 100vh;
  position: sticky;
  top: 0;

It seems like they no longer have any effect to me...?  I tried testing with a viewport size that taller than the browser window, and these styles seemed to make no difference.  Do you see a purpose to keeping them?  (Also they may impede the overlay sizing.)

@@ +148,4 @@
>    vertical-align: top;
>  }
>  
> +.viewport-overlay {

To implement the overlay, I'd suggest wrapping the device modal in another container div that can be used as the overlay.  So something like:

1. Change device-modal to return:

return dom.div(
  {
    id: "modal-overlay",
  },
  <current return value here>
);

2. Style the overlay with something like:

#modal-overlay {
  position: absolute;
  top: 0;
  left: 0;
  min-height: 100vh;
  height: 100%;
  width: 100%;
  background-color: <your favorite transparent color>
}

3. Set position: relative on the overlay container:

#app {
  position: relative;
}

I think that should give the right effect.  Make sure to test both cases (a) viewport smaller than window and (b) viewport larger than window.

I don't think we can use a literal filter: blur effect, since it seems to make the page content disappear.  Hopefully a transparent color is close enough?

Comment 5

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

Attachment: modal.patch

Added an overlay to the viewport, and got escape/click outside of the modal working.

Comment 6

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

Comment on attachment 8765042 [details] [diff] [review]
modal.patch

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

Move the r=jryans up to commit summary (first line).

Clicking out and escape do work, but it appears the X and Done buttons no longer work.

As we discussed on Vidyo, I think it would be better to try to "balance" what is covered by the overlay visually vs. the area you can click to exit the modal.  At the moment, only the viewport is covered visually, but you can click anywhere in the entire document to exit the modal.  This is somewhat confusing for things like the global toolbar, since they look like they are still interactive visually, but trying to use them just exits the modal.  

It should become easier to bind the click handler in a more natural React way (in the render() method) if we make the overlay cover the same area that should be clickable.  We might be able to do the same with the keydown handler too, but I am less sure of that one.

::: devtools/client/responsive.html/components/device-selector.js
@@ +92,4 @@
>        }),
>        dom.option({
>          value: OPEN_DEVICE_MODAL_VALUE,
> +        id: "openDeviceModal",

Use "my-cool-name" style for IDs and classes.

Comment 7

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

Attachment: modal.patch

Switched this to have a modal that covers the entire screen.

I still have `componentDidMount` and `componentWillUnmount` in `device-modal.js`; I ran into an issue where I couldn't use onKeyPress/onKeyDown events unless they were in a textarea or an input box. Couldn't tell from the React documentation if that was purposeful design or a bug.

Also in `device-modal.js`, I have a wrapped element with an empty class string... wasn't sure if that could be left undefined or not.

In `index.css` I removed the height declaration from #app, although in the future we might want to bring it back for viewport fitting? Didn't seem to matter for this patch/at the moment, so it's gone.

Comment 8

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

Comment on attachment 8766435 [details] [diff] [review]
modal.patch

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

Overall I think the approach looks good!  A few cleanup items left, but we're getting close.

::: devtools/client/responsive.html/components/device-modal.js
@@ +25,5 @@
>      return {};
>    },
>  
> +  componentDidMount() {
> +    window.addEventListener("keydown", this.listenForEsc, true);

The issue you found with keydown only working on inputs, etc. is just how key events work, I think.  They are only sent to the "focused" element and only some elements like inputs can be focused (you can also add @tabindex to more elements to make them focusable).

Given all that, I think this window listener is okay the way you have it.

@@ +79,4 @@
>      onUpdateDeviceModalOpen(false);
>    },
>  
> +  listenForEsc(event) {

Since this is called for any keydown, let's call it "onKeyDown".

@@ +79,5 @@
>      onUpdateDeviceModalOpen(false);
>    },
>  
> +  listenForEsc(event) {
> +    if (event.keyCode === 27) {

Add a comment above this line to note that this is escape.

Also, since the event listener is added in componentWillMount, it is active all the time, even when the modal is hidden (since the component is always present on the page).  Please add something like:

if (!this.props.devices.isModalOpen) {
  return;
}

to the top of this listener.

@@ +93,5 @@
>        devices,
>        onUpdateDeviceModalOpen,
>      } = this.props;
>  
> +    let modalClass = "device-modal container fade-in";

Let's leave "device-modal container" as a common path, and then append "fade-in" or "fade-out" as needed.  So, use an if / else below.

@@ +98,3 @@
>  
>      if (!devices.isModalOpen) {
> +      modalClass = "device-modal container fade-out";

As for the exact class names, let's use class names that describe the end state instead of the animation type.  (The keyframe names seem fine as they are though, since they are about the animation itself.)

So, something like "opened" and "closed".

@@ +108,4 @@
>  
>      return dom.div(
>        {
> +        className: ""

I would like this element to have some kind of id / class.  Seems like an ID is okay here since there is only one device modal.

The best I thought of was something like "device-modal-wrapper" which is kind of sad...  Anyway, it would be nice to have some name.

I think we actually want to be setting the opened vs. closed class name on this root element.  Then we can use that as a selector for everything underneath without having to attach opened vs. closed classes everywhere.

@@ +166,2 @@
>          {
> +          className: this.props.devices.isModalOpen ? "modal-overlay" : "",

I would like this to have _some_ class / ID that is always set and "modal-overlay" is a good name for that.

Assuming opened vs. closed is moved up to the wrapper, you can use something like "#device-modal-wrapper.opened #modal-overlay" as the selector to style this.

::: devtools/client/responsive.html/components/device-selector.js
@@ +92,4 @@
>        }),
>        dom.option({
>          value: OPEN_DEVICE_MODAL_VALUE,
> +        id: "openDeviceModal",

It seems like this is no longer used in the new version, so let's remove it.

::: devtools/client/responsive.html/index.css
@@ +151,4 @@
>    vertical-align: top;
>  }
>  
> +.modal-overlay {

Move this down to the device modal section.

Comment 9

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> @@ +108,4 @@
> >  
> >      return dom.div(
> >        {
> > +        className: ""
> 
> I would like this element to have some kind of id / class.  Seems like an ID
> is okay here since there is only one device modal.
> 
> The best I thought of was something like "device-modal-wrapper" which is
> kind of sad...  Anyway, it would be nice to have some name.
> 
> I think we actually want to be setting the opened vs. closed class name on
> this root element.  Then we can use that as a selector for everything
> underneath without having to attach opened vs. closed classes everywhere.
Currently the .opened/.closed classes just to handle the animation specifically for the device modal box and include a `translateY` that would look odd on the `.modal-overlay`. Should I still move those classes up to the top and apply that transform to the device modal box in some other way?

Comment 10

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

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #9)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> > @@ +108,4 @@
> > >  
> > >      return dom.div(
> > >        {
> > > +        className: ""
> > 
> > I would like this element to have some kind of id / class.  Seems like an ID
> > is okay here since there is only one device modal.
> > 
> > The best I thought of was something like "device-modal-wrapper" which is
> > kind of sad...  Anyway, it would be nice to have some name.
> > 
> > I think we actually want to be setting the opened vs. closed class name on
> > this root element.  Then we can use that as a selector for everything
> > underneath without having to attach opened vs. closed classes everywhere.
> Currently the .opened/.closed classes just to handle the animation
> specifically for the device modal box and include a `translateY` that would
> look odd on the `.modal-overlay`. Should I still move those classes up to
> the top and apply that transform to the device modal box in some other way?

I was imagining opened / closed would move up to the wrapper element purely so that the opened vs. closed state is "accessible" via selectors to all of its children without repeating the information as class names on the children as well.

So, I did not mean that that current _styles_ of opened / closed should actually apply to the wrapper element.  That's why you'd want to adjust the selectors like I suggested later in the review so that they still match the same elements as they do now.

Here's how I would imagine you would rewrite them:

.modal-overlay -> #device-modal-wrapper.opened #modal-overlay
.device-modal.fade-in -> #device-modal-wrapper.opened .device-modal
.device-modal.fade-out -> #device-modal-wrapper.closed .device-modal

Does this make sense?  There might be an edge case I'm missing...

Comment 11

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

Attachment: modal.patch

I think I understood the comments about the device modal and the open/close classes. This version of the patch removes some of the React class swapping, changes the class names, and adds the open/close classes to the wrapper.

Comment 12

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

Comment on attachment 8768037 [details] [diff] [review]
modal.patch

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

Great, this version looks good overall to me!  I'd like to double check the next version just to be sure all rebasing issues and such are resolved, so f+ for now.

::: devtools/client/responsive.html/components/device-modal.js
@@ +90,5 @@
> +    if (!this.props.devices.isModalOpen) {
> +      return;
> +    }
> +    // Escape keycode
> +    else if (event.keyCode === 27) {

Use just regular `if` on this line (no `else`).

::: devtools/client/responsive.html/components/device-selector.js
@@ +117,4 @@
>          onChange: this.onSelectChange,
>          disabled: (state !== Types.deviceListState.LOADED),
>        },
> +      dom.option({

What is the reason for this change?  It seems like a bad rebase.  I don't think any changes are needed in this file?

Comment 13

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

Attachment: modal.patch

Removes the changes made in device-selector.js, but everything else is the same.

Comment 14

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

Comment on attachment 8768777 [details] [diff] [review]
modal.patch

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

There are still a few sizing issues to work out, but I think we could improve them in follow up work.  It's pretty hard to think about all the sizing requirements at the same time!

::: devtools/client/responsive.html/index.css
@@ +39,4 @@
>  
>  #root,
>  html, body {
> +  height: 100%;

Do we still need this height: 100%?  Removing it did not seem to change things.  If it is still needed, add a comment about its purpose.

@@ +50,5 @@
>    flex-direction: column;
> +  padding-top: 15px;
> +  padding-bottom: 1%;
> +  position: relative;
> +  height: 100%;

I did notice there's a sizing problem with this version:

STR:

1. Make the viewport tall enough so that scrollbars are needed in the browser window
2. Make the browser window smaller than the viewport (opening the toolbox is one fast way)
3. Show the overlay

ER:

The overlay should cover the entire UI, even while scrolling

AR:

The overlay currently only covers a region the size of the browser window.  Once you scroll, you can see "through" the overlay.

Possible fix:

* Remove height: 100% from #app
* Add min-height: 100vh to #app

This seems to help the "too tall" case.  Even with that, there are issues when the viewport is too wide.  We can probably leave "too wide" case to a separate bug, it seems we have other problems with it, like the global toolbar takes on a mind of its own.

Comment 15

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> Possible fix:
> 
> * Remove height: 100% from #app
> * Add min-height: 100vh to #app
> 
> This seems to help the "too tall" case.  Even with that, there are issues
> when the viewport is too wide.  We can probably leave "too wide" case to a
> separate bug, it seems we have other problems with it, like the global
> toolbar takes on a mind of its own.

Oh no, I tried to fix this. Alas!

Without height declarations: https://cl.ly/1q2w3e0P0d0F

With percentage height declarations: (large viewport: https://cl.ly/1S05160u1l0r) (small viewport: https://cl.ly/1o0A0j2S201C)

100vh, small viewport: https://cl.ly/3v0h3T3A0n3m

Do you want me to check this in and file a follow-up bug? At least you don't notice it until the viewport is kinda awkwardly sized, although this is pretty annoying...

Comment 16

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

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #15)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> > Possible fix:
> > 
> > * Remove height: 100% from #app
> > * Add min-height: 100vh to #app
> > 
> > This seems to help the "too tall" case.  Even with that, there are issues
> > when the viewport is too wide.  We can probably leave "too wide" case to a
> > separate bug, it seems we have other problems with it, like the global
> > toolbar takes on a mind of its own.
> 
> Oh no, I tried to fix this. Alas!
> 
> Without height declarations: https://cl.ly/1q2w3e0P0d0F
> 
> With percentage height declarations: (large viewport:
> https://cl.ly/1S05160u1l0r) (small viewport: https://cl.ly/1o0A0j2S201C)
> 
> 100vh, small viewport: https://cl.ly/3v0h3T3A0n3m
> 
> Do you want me to check this in and file a follow-up bug? At least you don't
> notice it until the viewport is kinda awkwardly sized, although this is
> pretty annoying...

Yeah, let's land what you have and file a follow up to reach sizing nirvana.  It's not obvious what the right solution is, so I expect it will take a lot of experimentation.  (Maybe we should even write tests to check all the cases...?  It's really quite hard to test them all manually each time.)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/d192030869ef
device modal fades in/out, r=jryans

Comment 18

[Carsten Book [:Tomcat]]

sorry had to back this out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10446931&repo=fx-team#L7800

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/829e9870eb1d
Backed out changeset d192030869ef for eslint failures

Comment 20

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

Carsten, if it's all right, I'm going to clear my ni? based on the conversation in the #devtools IRC:

> jdescottes | Tomcat|sheriffduty: confirmed, I just imported the console commit and browser_dbg_WorkerActor.attachThread.js started failing locally

Comment 21

[Carsten Book [:Tomcat]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #20)
> Carsten, if it's all right, I'm going to clear my ni? based on the
> conversation in the #devtools IRC:
> 
> > jdescottes | Tomcat|sheriffduty: confirmed, I just imported the console commit and browser_dbg_WorkerActor.attachThread.js started failing locally

hey Helen,
yeah this conversation was for crashes caused by a other cset in that push. But i guess the error from comment #19 is still related to your push or ? :)

Comment 22

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

Attachment: device-modal.patch

Looks like it was just an eslint error from not having /* eslint-env browser */ in device-modal.js, so I'm carrying over the r+ from jryans.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b9cd9d6a4abe
Device modal fades in/out. r=jryans

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/9fb3a5f8b892 for test failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=10475520&repo=fx-team

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/45c7c3af7a20
Fix dt6 browser_modal_device_exit|submit.js failure. r=bustage

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/8c882ca5b996
Reland with fixup Bug 1266414 - Device modal fades in/out. r=jryans

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Pulsebot from comment #26)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/fx-team/rev/8c882ca5b996
> Reland with fixup Bug 1266414 - Device modal fades in/out. r=jryans

So this is a backout of 45c7c3af7a20 folded into a relanding of b9cd9d6a4abe folded into a relanding of 45c7c3af7a20. I guess that's a bit more complicated than it needed to be, but hopefully everything starts passing now.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hmm, this is still failing for some reason on my relanding.

Backed it (so everything, I think) out in https://hg.mozilla.org/integration/fx-team/rev/1fe5b3ad867b

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/da676c4491c1
Back everything out from bug 1266414 to fix bustage. r=backout
https://hg.mozilla.org/integration/fx-team/rev/0845f945173f
Device modal fades in/out. r=jryans

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out everything and relanded the patch and the followup (the backout seemed to be missing the followup):

https://hg.mozilla.org/integration/fx-team/rev/0845f945173f613cabea846124a6bd443a048aa7
https://hg.mozilla.org/integration/fx-team/rev/da676c4491c1917e4ebaa9ef31961718b60689d5

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out because it's still failing the test: https://hg.mozilla.org/integration/fx-team/rev/a76528042c03

Comment 32

[Tim Nguyen :ntim]

So one part of the failure was caused by the class names not being updated in tests (including head.js).
I also had forgotten to change the selector to target the modal in the tests (it's now #device-modal-wrapper instead of .device-modal).
But even after these changes, there remains one browser_device_modal_submit.js failure that I can't seem to understand.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45c7c3af7a20
https://hg.mozilla.org/mozilla-central/rev/da676c4491c1

Comment 34

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

I made an attempt at fixing up the tests, but I'd like to check with try first.  At the moment, try is closed again, so I'll wait until it opens.  Setting ni? so I don't forget.

Comment 35

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0eb49d5c3409

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ece89d2f73e
device modal fades in/out, r=jryans

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ece89d2f73e

Comment 38

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to test this issue on Firefox 51.0a1 (2016-08-01), across platforms [1] and beside Bug 1291263 and Bug 1286001, no other issues were found.
I am marking this issue Verified Fixed, since the found issues were logged separately,

[1] Windows 10 x64, Mac OS X 10.11.1, Ubuntu 16.04 x64