Closed
Bug 1395065
Opened 7 years ago
Closed 7 years ago
[onboarding] change overlay icon position will overlap the newtab "restore all" button
Categories
(Firefox :: New Tab Page, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: gasolin, Assigned: rickychien)
References
Details
(Whiteboard: [photon-onboarding][photon-onboarding-newui])
Attachments
(3 files)
Moving the overlay icon position change discussion from https://bugzilla.mozilla.org/show_bug.cgi?id=1392468#c7 new Icon position (top: 12px, left: 12px) will overlap the `restore all` message button on classic newtab(not activity stream), and cause test fail (the mouse can't click the `restore all` link). The under message is `Thumbnail removed, Undo / Restore All` and take about 20px height. The current position(top 34px, left 34px) does not have that issue. Dao preffer keeping support (newtab and activity stream) without break newtab undo test (In bug 1392468 comment 21)
Reporter | ||
Comment 1•7 years ago
|
||
We'd move the overlay icon position change discussion from https://bugzilla.mozilla.org/show_bug.cgi?id=1392468#c23 Please leave comment here instead of bug 1392468
Flags: needinfo?(tspurway)
Flags: needinfo?(mverdi)
Flags: needinfo?(abenson)
Reporter | ||
Updated•7 years ago
|
Summary: [onboarding] change overlay icon position will overlap the newtab restore all button → [onboarding] change overlay icon position will overlap the newtab "restore all" button
Reporter | ||
Updated•7 years ago
|
Whiteboard: [photon-onboarding][triage]
Comment 2•7 years ago
|
||
Move the notification to the top-center of the window. See attached example...
Flags: needinfo?(abenson)
Comment 3•7 years ago
|
||
I think we have everything we need to proceed with this?
Flags: needinfo?(tspurway)
Comment 4•7 years ago
|
||
(In reply to Aaron Benson from comment #2) > Created attachment 8902702 [details] > tiles-restore-notification-centered.png > > Move the notification to the top-center of the window. See attached > example... +1
Flags: needinfo?(mverdi)
Reporter | ||
Updated•7 years ago
|
Whiteboard: [photon-onboarding][triage] → [photon-onboarding][triage][photon-onboarding-newui]
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: [photon-onboarding][triage][photon-onboarding-newui] → [photon-onboarding][photon-onboarding-newui]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: qe-verify+
Updated•7 years ago
|
QA Contact: jwilliams
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Oriol Brufau, Could you tell me what kind of problems you're trying to solve in bug 1395599? Is your problem still reproducible on latest Nightly? As I can see, the clickable region covered just right around the icon and it also left some unclickable margins around the icon seem like by design. Some suggestions in https://bugzilla.mozilla.org/show_bug.cgi?id=1395599#c0 have been incorrect which might be due to recent changes. Can you check that again? Or give us more details about what you want to achieve? Thanks.
Flags: needinfo?(oriol-bugzilla)
Comment 8•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #7) > Could you tell me what kind of problems you're trying to solve in bug > 1395599? Is your problem still reproducible on latest Nightly? The onboarding icon disappears when you hover it near the top. See the screencast. Recorded in latest 2017-09-06 Nightly.
Flags: needinfo?(oriol-bugzilla)
Comment 9•7 years ago
|
||
I meant the box disappears, not the Firefox icon.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
I see. Patch has updated for moving restore all button and fixing bug 1395599 issue.
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8905446 [details] Bug 1395065 - Move the notification to the top-center of the window https://reviewboard.mozilla.org/r/177262/#review182674 browser/base/content/test/newtab/browser_newtab_undo.js test passed after moveing the `restore all` button. But still some style issue need to be addressed ::: browser/extensions/onboarding/content/onboarding.css:40 (Diff revision 2) > /* make sure the icon stay above the activity-stream searchbar */ > /* We want this always under #onboarding-overlay */ > z-index: 10; > } > > +#onboarding-overlay-button:hover { bug 1395599 is not just related to :hover state, please remove this style and patch #onboarding-overlay-button directly. And we also need to move the firefox icon to the right position, the style will looks like #onboarding-overlay-button { padding: 10px 0 0 0; ... top: 2px; offset-inline-start: 12px; }
Attachment #8905446 -
Flags: review?(gasolin)
Reporter | ||
Comment 13•7 years ago
|
||
Since new firefox icon position moved from left: 34 -> 12, we can move SPEECH_BUBBLE_MIN_WIDTH_PX in onboarding.js from 1150px to 1130px. Please also update the value in test cases accordingly. (the size affect to prevent the speech bubble overlap the activity stream search field in newtab. When resize to this value, the speech bubble will turn into bluedot)
Assignee | ||
Comment 15•7 years ago
|
||
According to latest spec [1], fox icon should be left: 12px, top: 14px. [1] https://mozilla.invisionapp.com/share/D4D6BPMGH#/screens/249240594_New_Tab_-_Onboarding_Measurements
Comment hidden (mozreview-request) |
Reporter | ||
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8905446 [details] Bug 1395065 - Move the notification to the top-center of the window https://reviewboard.mozilla.org/r/177262/#review182720 ::: browser/extensions/onboarding/content/onboarding.css:30 (Diff revision 3) > > #onboarding-overlay-button { > - padding: 0; > + padding: 10px 0 0 0; > position: absolute; > cursor: pointer; > - top: 34px; > + top: 2px; if spec said top: 14px, then we may need to set `top 4px;`
Attachment #8905446 -
Flags: review?(gasolin) → review+
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f4d56d495aa6 Move the notification to the top-center of the window r=gasolin
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f4d56d495aa6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 21•7 years ago
|
||
I have verified that this bug is fixed on today's nightly build.
Status: RESOLVED → VERIFIED
Comment 22•7 years ago
|
||
I can confirm the intended behavior is respected on beta. I verified using Fx 57.0b7 on Windows 10 x64, Ubuntu 14.04 LTS and macOS X 10.12.6.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•