Closed Bug 1358970 Opened 7 years ago Closed 7 years ago

The onBoarding overlay should be responsive to difference window sizes

Categories

(Firefox :: General, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- verified
firefox58 --- verified

People

(Reporter: Fischer, Assigned: timdream)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 files)

The layout and design of the onBoarding overlay should be responsive to difference window sizes
Whiteboard: [photon-onboarding]
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
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 55
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.
Flags: qe-verify+
QA Contact: jwilliams
Priority: -- → P1
Target Milestone: Firefox 55 → Firefox 56
Set to INVALID because the RWD is no longer required. See https://bugzilla.mozilla.org/show_bug.cgi?id=1354707#c8.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
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]
Attached image 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.
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 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)
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.
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()`.
Attachment #8885130 - Flags: feedback?(fliu) → feedback+
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+
Let me get a Try run first? I wonder the window size we run tests on ...
Keywords: checkin-needed
Blocks: 1372067
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
https://hg.mozilla.org/mozilla-central/rev/da08d4b7545e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 1381010
Verified as fixed.
I have tested this on Ubuntu 16.04, Windows 10 and OSX10.12 and it is fixed.
Status: RESOLVED → VERIFIED
I have verified that the tour is responsive to different window sizes on Win 10 x64, Win 7 x32, Ubuntu 16.04 x32, and Mac 10.12 with Firefox 58.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: