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)

57 Branch
defect

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)
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)
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
Whiteboard: [photon-onboarding][triage]
Move the notification to the top-center of the window. See attached example...
Flags: needinfo?(abenson)
I think we have everything we need to proceed with this?
Flags: needinfo?(tspurway)
(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)
Whiteboard: [photon-onboarding][triage] → [photon-onboarding][triage][photon-onboarding-newui]
Priority: -- → P1
Whiteboard: [photon-onboarding][triage][photon-onboarding-newui] → [photon-onboarding][photon-onboarding-newui]
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: qe-verify+
QA Contact: jwilliams
Blocks: 1395599
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)
Attached image screencast.gif
(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)
I meant the box disappears, not the Firefox icon.
I see. Patch has updated for moving restore all button and fixing bug 1395599 issue.
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)
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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/f4d56d495aa6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I have verified that this bug is fixed on today's nightly build.
Status: RESOLVED → VERIFIED
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.