Closed
Bug 1358970
Opened 8 years ago
Closed 7 years ago
The onBoarding overlay should be responsive to difference window sizes
Categories
(Firefox :: General, defect, P3)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 56
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
Reporter | ||
Updated•8 years ago
|
Whiteboard: [photon-onboarding]
Updated•8 years 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•8 years ago
|
Status: NEW → ASSIGNED
Updated•8 years ago
|
Target Milestone: --- → Firefox 55
Comment 1•8 years 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•8 years ago
|
Flags: qe-verify+
QA Contact: jwilliams
Updated•8 years ago
|
Priority: -- → P1
Updated•7 years ago
|
Target Milestone: Firefox 55 → Firefox 56
Reporter | ||
Comment 2•7 years 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
Closed: 7 years ago
Resolution: --- → INVALID
Comment 3•7 years ago
|
||
remove whiteboard tag due to RWD is no longer required.
Whiteboard: [photon-onboarding]
Assignee | ||
Comment 4•7 years ago
|
||
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]
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.
Comment 8•7 years 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.
Assignee | ||
Comment 9•7 years ago
|
||
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) |
Assignee | ||
Comment 11•7 years ago
|
||
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 13•7 years 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•7 years 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•7 years ago
|
Attachment #8885130 -
Flags: feedback?(fliu) → feedback+
Comment hidden (mozreview-request) |
Comment 16•7 years 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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 17•7 years ago
|
||
Let me get a Try run first? I wonder the window size we run tests on ...
Keywords: checkin-needed
Assignee | ||
Comment 18•7 years ago
|
||
... and yes tests are all busted. https://treeherder.mozilla.org/#/jobs?repo=try&revision=23a7d705883cae14db1b3607047dba433f004a9f&selectedJob=113495079
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
This is a lot better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c4ada51cbff05a9adbc4c8dd1e895003d0e65b2
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment 22•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/da08d4b7545e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Comment 24•7 years ago
|
||
Verified as fixed. I have tested this on Ubuntu 16.04, Windows 10 and OSX10.12 and it is fixed.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 25•7 years ago
|
||
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.
status-firefox58:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•