1252495 - Global Toolbar should be always visible

Status: VERIFIED FIXED

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

Description

[Matteo Ferretti [:zer0] [:matteo]]

With the current implementation of global toolbar – see bug 1239464 – with small values in window's height; the toolbar won't be visible.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: MozReview Request: Bug 1252495 - Global Toolbar should be always visible; r=jryans

Review commit: https://reviewboard.mozilla.org/r/44161/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44161/

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8737866 [details]
MozReview Request: Bug 1252495 - Global Toolbar should be always visible; r=jryans

Helen, what do you think? Probably we could reduce a bit the margin of the global toolbar – at least on top, I left what we had previously –; but it's up to you!

Comment 3

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

Comment on attachment 8737866 [details]
MozReview Request: Bug 1252495 - Global Toolbar should be always visible; r=jryans

https://reviewboard.mozilla.org/r/44161/#review40979

::: devtools/client/responsive.html/index.css:34
(Diff revision 1)
>  html, body {
>    margin: 0;
>    height: 100%;
>  }
>  
> -body {
> +#root {

What is the purpose of these 3 styles?  They didn't appear to make a difference when I disabled them...

::: devtools/client/responsive.html/index.css:48
(Diff revision 1)
>    justify-content: center;
>    align-items: center;
>    flex-direction: column;
> +
> +  /* Snap to the top of the app when there isn't enough vertical space anymore
> +     to center the viewports (so we don't loose the global toolbar) */

Nit: lose

::: devtools/client/responsive.html/index.css:90
(Diff revision 1)
>    padding: 4px 5px;
>    display: inline-flex;
>    -moz-user-select: none;
> +  /* Make sure the global toolbar is visible when there's horizontal overflow
> +     and the document is scrolled over the original toolbar's left position */
> +  position: sticky;

Since we're allowing the toolbar to scroll out of view vertically, I am not sure we want to make it sticky horizontally?

Comment 4

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

Comment on attachment 8737866 [details]
MozReview Request: Bug 1252495 - Global Toolbar should be always visible; r=jryans

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #2)
> Comment on attachment 8737866 [details]
> MozReview Request: Bug 1252495 - Global Toolbar should be always visible;
> r=jryans
> 
> Helen, what do you think? Probably we could reduce a bit the margin of the
> global toolbar – at least on top, I left what we had previously –; but it's
> up to you!

I actually really like what you've done—it scales well, and looks pretty good on smaller screens (which I think is important).

Going to ui-r+ this.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/44161/#review40979

> What is the purpose of these 3 styles?  They didn't appear to make a difference when I disabled them...

As discussed on IRC, they're needed basically to center the whole toolbar + viewports; it might not be obvious if the window's height is not big enough.

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8737866 [details]
MozReview Request: Bug 1252495 - Global Toolbar should be always visible; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44161/diff/1-2/

Comment 7

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

Comment on attachment 8737866 [details]
MozReview Request: Bug 1252495 - Global Toolbar should be always visible; r=jryans

https://reviewboard.mozilla.org/r/44161/#review41039

Great, thanks for working on this!

Comment 8

[Carsten Book [:Tomcat]]

failed to apply:

applying 94ddbdf08314
patching file devtools/client/responsive.html/index.css
Hunk #2 FAILED at 102
1 out of 2 hunks FAILED -- saving rejects to file devtools/client/responsive.html/index.css.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

can you take a look, thanks!

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Carsten Book [:Tomcat] from comment #8)
> failed to apply:

> can you take a look, thanks!

Immediately, thanks! Probably it clashes with my other bug that was just landed before this.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8737866 [details]
MozReview Request: Bug 1252495 - Global Toolbar should be always visible; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44161/diff/2-3/

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Rebased against the current master after bug 1239461.

Comment 12

[Pulsebot]

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

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/b86104749bea

Comment 14

[Mihai Boldan, Desktop QA [:mboldan]]

While testing this issue, I've encountered 2 possible issues:
- If the FF window is set to it's minimum height, the Global toolbar is not visible - http://imgur.com/MpYJzsb 
- If the viewport is wider than the screen (Eg. 4k Ultra HD Television device is selected) and the window is scrolled to the right, the global toolbar is not visible any more. - My opinion is that the Global toolbar should be visible and centered all the time. 

I know that the first issue is an edge case, but should I log a bug for the issues described above?

Note that the tests were performed on FF 48.0a1 (2016-04-12) and on Windows 10 x64, Ubuntu 14.04 x86 and Mac OS X 10.11.

Comment 15

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

(In reply to Mihai Boldan, QA [:mboldan] from comment #14)
> While testing this issue, I've encountered 2 possible issues:
> - If the FF window is set to it's minimum height, the Global toolbar is not
> visible - http://imgur.com/MpYJzsb 

I think this issue is okay for now.  Helen is planning some tweaks to the global toolbar behavior in bug 1262839, so let's check again once that's done.

> - If the viewport is wider than the screen (Eg. 4k Ultra HD Television
> device is selected) and the window is scrolled to the right, the global
> toolbar is not visible any more. - My opinion is that the Global toolbar
> should be visible and centered all the time. 
> 
> I know that the first issue is an edge case, but should I log a bug for the
> issues described above?

Helen's new behavior may cover this as well, but let's file a bug for this part just in case.

Comment 16

[Mihai Boldan, Desktop QA [:mboldan]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15)
> (In reply to Mihai Boldan, QA [:mboldan] from comment #14)
> > While testing this issue, I've encountered 2 possible issues:
> > - If the FF window is set to it's minimum height, the Global toolbar is not
> > visible - http://imgur.com/MpYJzsb 
> 
> I think this issue is okay for now.  Helen is planning some tweaks to the
> global toolbar behavior in bug 1262839, so let's check again once that's
> done.

I'm leaving this issue Resolved Fixed until Bug 1262839 is resolved and and I will be able to confirm that the issue described above was fixed. 
 
> > - If the viewport is wider than the screen (Eg. 4k Ultra HD Television
> > device is selected) and the window is scrolled to the right, the global
> > toolbar is not visible any more. - My opinion is that the Global toolbar
> > should be visible and centered all the time. 
> > 
> > I know that the first issue is an edge case, but should I log a bug for the
> > issues described above?
> 
> Helen's new behavior may cover this as well, but let's file a bug for this
> part just in case.

I've logged Bug 1265323 for this issue.

Comment 17

[Marco Mucci [:MarcoM]]

Hi Mihai, should this bug be marked as verified?  Thanks.

Comment 18

[Mihai Boldan, Desktop QA [:mboldan]]

(In reply to Marco Mucci [:MarcoM] from comment #17)
> Hi Mihai, should this bug be marked as verified?  Thanks.

Hi Marco,
I think we should leave this issue Resolved Fixed until the Bug 1262839 is fixed and see if the first issue described in Comment 14 is resolved along with it.

Thanks.

Comment 19

[Mihai Boldan, Desktop QA [:mboldan]]

Since the first issue from Comment 14 was resolved by the fix from Bug 1262839, I am marking this issue Verified Fixed.
The tests were performed on Firefox 49.0a1 (2016-05-03) and on Mac OS X 10.10.5, Ubuntu 14.04 x86 and on Windows 10 x64.