The onBoarding overlay should be responsive to difference window sizes

VERIFIED FIXED in Firefox 56

Status

()

Firefox
General
P3
normal
VERIFIED FIXED
4 months ago
16 days ago

People

(Reporter: Fischer, Assigned: timdream)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
The layout and design of the onBoarding overlay should be responsive to difference window sizes
(Reporter)

Updated

4 months ago
Whiteboard: [photon-onboarding]

Updated

4 months ago
Assignee: nobody → rexboy
Summary: [meta] The onBoarding overlay should be responsive to difference window sizes → The onBoarding overlay should be responsive to difference window sizes

Updated

4 months ago
Status: NEW → ASSIGNED

Updated

4 months ago
Target Milestone: --- → Firefox 55

Comment 1

4 months ago
Just made some prototyping based on :gasolin's prototype overlay. See (on the security view page):
http://gasolin.idv.tw/onboarding.js/

and the patch for detail.
https://github.com/gasolin/onboarding.js/commit/c309b26dfd5cee37d2f849d7cd1ff11df73d08d6

Separating points are 
>1200px
1200px~1100px
1100px~1000px
1000px~900px
900px

So far I didn't encounter issues since the RWD part is quite straightforward.

Updated

4 months ago
Flags: qe-verify+
QA Contact: jwilliams

Updated

4 months ago
Priority: -- → P1

Updated

3 months ago
Target Milestone: Firefox 55 → Firefox 56
(Reporter)

Comment 2

3 months ago
Set to INVALID because the RWD is no longer required. See https://bugzilla.mozilla.org/show_bug.cgi?id=1354707#c8.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → INVALID

Comment 3

3 months ago
remove whiteboard tag due to RWD is no longer required.
Whiteboard: [photon-onboarding]
This should have been WONTFIX instead of INVALID since the issue always exists. Anyway, I am reopening this and make it as P3. Let's try to get to this in future iterations.
Status: RESOLVED → REOPENED
Priority: P1 → P3
Resolution: INVALID → ---
Whiteboard: [photon-onboarding]

Updated

2 months ago
Duplicate of this bug: 1375428

Comment 6

2 months ago
Created attachment 8883470 [details]
tour-small-window.png

The window needs to be at least 900px wide to see the dialog's close button. When smaller than this there are no scrollbars and no apparent way to close the dialog.

In comparison the onboarding notification bar is responsive down to about 600px, about:newtab down to 400px and mozilla.org down to 300px. 

about:newtab is quite important so I would expect anything covering it to be similarly responsive or have a clear exit.

Updated

2 months ago
Duplicate of this bug: 1379026

Comment 8

2 months ago
We have a fresh Firefox Desktop started maximized if the screen width is less than 1152 x 936px:
http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1327
Unless users use a screen smaller than the size above (which is a rare case for nowadays PCs and notebooks) they won't see the overlay being clipped. I think this is a nice to have IMO.
A decision was made to hide the onboarding overlay/notification on small window sizes, so at least we don't show user broken things. We can revisit the design for small screen if we have cycle to it.

I will submit a patch for it and dup other bugs to this one.
Assignee: rexboy → timdream
Status: REOPENED → ASSIGNED
Comment hidden (mozreview-request)
Comment on attachment 8885130 [details]
Bug 1358970 - Hide onboarding UI when the window size is smaller than 960px,

As discussed, I put the logic in JS instead of CSS so we have explict access to the visibility state of the UIs. Let me know what you think about this patch and do point me any left overs. I will ask for review after your feedback. Thanks.
Attachment #8885130 - Flags: feedback?(fliu)
Duplicate of this bug: 1378162

Comment 13

a month ago
mozreview-review
Comment on attachment 8885130 [details]
Bug 1358970 - Hide onboarding UI when the window size is smaller than 960px,

https://reviewboard.mozilla.org/r/155966/#review161118

::: browser/extensions/onboarding/content/onboarding.js:244
(Diff revision 1)
> -    this._window.document.body.appendChild(this._overlay);
> -
>      this._loadJS(UITOUR_JS_URI);
>      this._loadJS(TOUR_AGENT_JS_URI);
>  
> -    this._overlayIcon.addEventListener("click", this);
> +    contentWindow.addEventListener("resize", this);

please use `this._window` for consistency

::: browser/extensions/onboarding/content/onboarding.js:252
(Diff revision 1)
>      // No any leak out there.
>      this._window.addEventListener("unload", () => this.destroy());
>  
> +    this.uiInitialized = false;
> +    this._resizeTimerId =
> +      contentWindow.requestIdleCallback(this._resizeUI.bind(this));

same here

::: browser/extensions/onboarding/content/onboarding.js:257
(Diff revision 1)
> +      contentWindow.requestIdleCallback(this._resizeUI.bind(this));
> +  }
> +
> +  _resizeUI() {
> +    // Don't show the overlay UI before we get to a better, responsive design.
> +    if (this._window.document.body.getBoundingClientRect().width < 960) {

Since we only care about the width, we can use mediaquery via win.matchMedia, ex: `this._window.matchMedia("max-width: 960px").matches`

According to comment 1, the breakpoint seems to be `900px` instead of `960px`. Though it's fine to keeping `960px` here.
(Reporter)

Comment 14

a month ago
mozreview-review
Comment on attachment 8885130 [details]
Bug 1358970 - Hide onboarding UI when the window size is smaller than 960px,

https://reviewboard.mozilla.org/r/155966/#review161126

Thanks for the works. Overall looks good.

::: browser/extensions/onboarding/content/onboarding.js:244
(Diff revision 1)
> -    this._window.document.body.appendChild(this._overlay);
> -
>      this._loadJS(UITOUR_JS_URI);
>      this._loadJS(TOUR_AGENT_JS_URI);
>  
> -    this._overlayIcon.addEventListener("click", this);
> +    contentWindow.addEventListener("resize", this);

Nit: Please use `this._window`. Very trivial just for the consistency, thanks.

::: browser/extensions/onboarding/content/onboarding.js:252
(Diff revision 1)
>      // No any leak out there.
>      this._window.addEventListener("unload", () => this.destroy());
>  
> +    this.uiInitialized = false;
> +    this._resizeTimerId =
> +      contentWindow.requestIdleCallback(this._resizeUI.bind(this));

Nit: Please use `this._window`.
Nit: Maybe kind of minor but please use `() => this._resizeUI()` because the perfomance of arrow function is better (see Bug 1355056). Thanks.

::: browser/extensions/onboarding/content/onboarding.js:257
(Diff revision 1)
> +      contentWindow.requestIdleCallback(this._resizeUI.bind(this));
> +  }
> +
> +  _resizeUI() {
> +    // Don't show the overlay UI before we get to a better, responsive design.
> +    if (this._window.document.body.getBoundingClientRect().width < 960) {

Guess the reason of using `getBoundingClientRect` is for the scroll bar and the border case. And since we are doing `_resizeUI` inside `requestIdleCallback`, this should be fine.

::: browser/extensions/onboarding/content/onboarding.js:345
(Diff revision 1)
>  
>    handleEvent(evt) {
> +    if (evt.type === "resize") {
> +      this._window.cancelIdleCallback(this._resizeTimerId);
> +      this._resizeTimerId =
> +        this._window.requestIdleCallback(this._resizeUI.bind(this));

Nit: Please use `() => this._resizeUI()`.
(Reporter)

Updated

a month ago
Attachment #8885130 - Flags: feedback?(fliu) → feedback+
Comment hidden (mozreview-request)

Comment 16

a month ago
mozreview-review
Comment on attachment 8885130 [details]
Bug 1358970 - Hide onboarding UI when the window size is smaller than 960px,

https://reviewboard.mozilla.org/r/155966/#review161362
Attachment #8885130 - Flags: review?(dtownsend) → review+
Keywords: checkin-needed
Let me get a Try run first? I wonder the window size we run tests on ...
Keywords: checkin-needed
... and yes tests are all busted.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23a7d705883cae14db1b3607047dba433f004a9f&selectedJob=113495079
Comment hidden (mozreview-request)
(Reporter)

Updated

a month ago
Blocks: 1372067
This is a lot better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c4ada51cbff05a9adbc4c8dd1e895003d0e65b2
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Comment hidden (mozreview-request)

Comment 22

a month ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da08d4b7545e
Hide onboarding UI when the window size is smaller than 960px, r=mossop
Keywords: checkin-needed

Comment 23

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/da08d4b7545e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months agoa month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
(Reporter)

Updated

a month ago
Depends on: 1381010

Comment 24

16 days ago
Verified as fixed.
I have tested this on Ubuntu 16.04, Windows 10 and OSX10.12 and it is fixed.

Updated

16 days ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.