[Onboarding] hide speech bubble to blue dot when resize to a smaller screen

VERIFIED FIXED in Firefox 57

Status

()

Firefox
New Tab Page
P1
normal
VERIFIED FIXED
11 months ago
9 months ago

People

(Reporter: gasolin@mozilla.com, Assigned: Fischer)

Tracking

(Blocks: 1 bug)

57 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57+ verified)

Details

(Whiteboard: [photon-onboarding][photon-onboarding-newui])

MozReview Requests

()

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

Attachments

(3 attachments)

(Reporter)

Description

11 months ago
based on https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit

hide speech bubble to blue dot when resize to a smaller screen (~1050px)
(Reporter)

Comment 1

11 months ago
Need address these issues
- in blue dot mode, show speech bubble when hovered
- address a11y change
(Reporter)

Updated

11 months ago
Blocks: 1392474
(Reporter)

Updated

11 months ago
Blocks: 1392475
(Reporter)

Comment 2

11 months ago
as founding in bug 1390222 comment 14, the new UI speech bubble won't overlap the search field, therefore we may not need to solve this issue.

Updated

11 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: jwilliams
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
(Reporter)

Comment 3

11 months ago
Here's the blue dot style for reference

```
#onboarding-overlay-button::after {
  content: ' ';
  border-radius: 50%;
  margin-inline-start: -16px;
  margin-top: -1px;
  border: 2px solid #fff;
  background-color: #0060df;
  padding: 6px 6px;
  position: absolute;
}
```

we might able to add a `noticedot` class and clean the origin classes to showing this state
(Assignee)

Updated

11 months ago
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Comment 4

11 months ago
Correct some style rules based on the specs: https://mozilla.invisionapp.com/share/KFD2GYHWM#/screens/249240594
(In reply to Fred Lin [:gasolin] from comment #3)
> Here's the blue dot style for reference
> ```
> #onboarding-overlay-button::after {
>   content: ' ';
>   border-radius: 50%;
>   margin-inline-start: -16px;
>   margin-top: -1px;
>   border: 2px solid #fff;
    border: 2px solid #f2f2f2;
>   background-color: #0060df;
    background-color: #0A84FF;
>   padding: 6px 6px;
>   position: absolute;
> }
> ```
(Assignee)

Comment 5

11 months ago
Correct the requirement based on the specs : https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit
(In reply to Fred Lin [:gasolin] from comment #0)
> based on
> https://docs.google.com/document/d/
> 1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit
> 
> hide speech bubble to blue dot when resize to a smaller screen (~1050px)
  hide speech bubble to blue dot when resize to a smaller screen (~1150px)
(Assignee)

Comment 6

11 months ago
Created attachment 8901744 [details]
speech_bubble_before_the_1st_notification_mute_session.gif
(Assignee)

Comment 7

11 months ago
Created attachment 8901745 [details]
blue_dot_after_the_1st_notification_mute_session.gif
(Assignee)

Comment 8

11 months ago
This bug should be rebased after the bug 1392468 and bug 1392467
Depends on: 1392468, 1392467
Comment hidden (mozreview-request)
(Assignee)

Comment 10

11 months ago
(In reply to Fischer [:Fischer] from comment #9)
> Created attachment 8901746 [details]
> Bug 1392472 - Implement the blue dot on the overlay icon button,
> 
> The patch
> 
> - fixes Bug 1392471 together: before the 1st notification mute session show
> the speech bubble by default; after the 1st notification mute session show
> the blue dot by default.
> 
> - shows the blue dot by default if the window width is smaller then 1150px
> disregarding the 1st notification mute session
> 
> - shows the speech bubble when hovering on the blue dot
> 
> Review commit: https://reviewboard.mozilla.org/r/173188/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/173188/
Hi Fred,

Please see [1][2] for the UI in action.
Will remember to rebase after the bug 1392468 and bug 1392467

[1] attachment 8901744 [details]: speech_bubble_before_the_1st_notification_mute_session.gif
[2] attachment 8901745 [details]: blue_dot_after_the_1st_notification_mute_session.gif

Thanks
(Assignee)

Comment 11

11 months ago
mozreview-review
Comment on attachment 8901746 [details]
Bug 1392472 - Implement the blue dot on the overlay icon button,

https://reviewboard.mozilla.org/r/173188/#review178512

::: browser/extensions/onboarding/content/onboarding.js:1002
(Diff revision 1)
>      let tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1);
>      button.setAttribute("aria-label", tooltip);
>      button.id = "onboarding-overlay-button";
>      button.setAttribute("aria-haspopup", true);
>      button.setAttribute("aria-controls", `${ONBOARDING_DIALOG_ID}`);
> +    if (this._isFirstSession) {

When doing `_initNotification` we may not start doing notification job immediately[1] so the 1st mute session calculation would go early if we reused `_muteNotificationOnFirstSession` here to judge the 1st session. As a result we use `_isFirstSession` only *checking* the 1st session


[1] http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/browser/extensions/onboarding/content/onboarding.js#394
(Assignee)

Comment 12

11 months ago
Comment on attachment 8901746 [details]
Bug 1392472 - Implement the blue dot on the overlay icon button,

Hi Rex,
Since the watermark mode and the blue dot would affect each other, please have a look at the blue dot implementation.
Attachment #8901746 - Flags: feedback?(rexboy)
(Reporter)

Comment 13

11 months ago
mozreview-review
Comment on attachment 8901746 [details]
Bug 1392472 - Implement the blue dot on the overlay icon button,

https://reviewboard.mozilla.org/r/173188/#review179334

Thanks for the quick patch! The visual and interaction looks right, and the width break point selection also looks good to me.

Since these are very late features, we'd better protect these changes with some tests to not regress the onboarding:
1. for resize window and check the speech-bubble class is removed/added
2. check the speech-bubble class exists during muteDuration
3. check the speech-bubble class is removed when the notification is shown

::: browser/extensions/onboarding/content/onboarding.css:67
(Diff revision 1)
> +  box-sizing: content-box;
> +}
> +
> +#onboarding-overlay-button:hover::after,
> +#onboarding-overlay-button.onboarding-speech-bubble::after,
> +#onboarding-overlay-button.onboarding-speech-bubble:hover::after {

we can remove `#onboarding-overlay-button.onboarding-speech-bubble:hover::after` since
`#onboarding-overlay-button.onboarding-speech-bubble::after` is sufficient

::: browser/extensions/onboarding/content/onboarding.css:87
(Diff revision 1)
>    white-space: pre-line;
>    margin-inline-start: 3px;
>    margin-top: -5px;
>  }
>  
> +@media all and (max-width: 1150px) {

we can put @media at bottom of file so we could find all media-query when we check next time.

::: browser/extensions/onboarding/content/onboarding.css:88
(Diff revision 1)
>    margin-inline-start: 3px;
>    margin-top: -5px;
>  }
>  
> +@media all and (max-width: 1150px) {
> +  #onboarding-overlay-button.onboarding-speech-bubble::after {

This looks like a hack to show the dot instead of speech bubble in smaller screen, the semantic is not correct.

I prefer to do the width checking in `_resizeUI` to add/remove the `onboarding-speech-bubble` class based on `_isFirstSession` and `1150px`. It's harder than this CSS change but it make code flow more clear.

::: browser/extensions/onboarding/content/onboarding.js:756
(Diff revision 1)
> -      // There is a queue. We had prompted before, this must not be the 1st session.
> +      // There is a queue. We had prompted tour notifications before, this must not be the 1st session.
>        return false;
>      }
>  
>      let muteDuration = Services.prefs.getIntPref("browser.onboarding.notification.mute-duration-on-first-session-ms");
>      if (muteDuration == 0) {

nit: ===

::: browser/extensions/onboarding/content/onboarding.js:756
(Diff revision 1)
> -      // There is a queue. We had prompted before, this must not be the 1st session.
> +      // There is a queue. We had prompted tour notifications before, this must not be the 1st session.
>        return false;
>      }
>  
>      let muteDuration = Services.prefs.getIntPref("browser.onboarding.notification.mute-duration-on-first-session-ms");
>      if (muteDuration == 0) {

we can call `getIntPref` directly without declare muteDuration

::: browser/extensions/onboarding/content/onboarding.js:780
(Diff revision 1)
>          value: Math.floor(Date.now() / 1000)
>        }]);
>        return true;
>      }
> +    let muteDuration = Services.prefs.getIntPref("browser.onboarding.notification.mute-duration-on-first-session-ms");
>      return Date.now() - lastTime <= muteDuration;

we can call `getIntPref` directly without declare muteDuration

::: browser/extensions/onboarding/content/onboarding.js:1002
(Diff revision 1)
>      let tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1);
>      button.setAttribute("aria-label", tooltip);
>      button.id = "onboarding-overlay-button";
>      button.setAttribute("aria-haspopup", true);
>      button.setAttribute("aria-controls", `${ONBOARDING_DIALOG_ID}`);
> +    if (this._isFirstSession) {

we also need to check the initial width here if we use `resize` to show/hide the blue dot
Attachment #8901746 - Flags: review?(gasolin)

Comment 14

11 months ago
mozreview-review
Comment on attachment 8901746 [details]
Bug 1392472 - Implement the blue dot on the overlay icon button,

https://reviewboard.mozilla.org/r/173188/#review179422

::: browser/extensions/onboarding/content/onboarding.css:88
(Diff revision 1)
>    margin-inline-start: 3px;
>    margin-top: -5px;
>  }
>  
> +@media all and (max-width: 1150px) {
> +  #onboarding-overlay-button.onboarding-speech-bubble::after {

We can also do `@media all and (min-width: 1150)` with `#onboarding-overlay-button.onboarding-speech-bubble::after` to avoid the confusion but you still need to copy the rule once.. So I don't have strong preference on either way.
Attachment #8901746 - Flags: feedback?(rexboy) → feedback+
(Reporter)

Updated

11 months ago
Whiteboard: [photon-onboarding] → [photon-onboarding][photon-onboarding-newui]
(Assignee)

Comment 15

11 months ago
mozreview-review-reply
Comment on attachment 8901746 [details]
Bug 1392472 - Implement the blue dot on the overlay icon button,

https://reviewboard.mozilla.org/r/173188/#review179334

> This looks like a hack to show the dot instead of speech bubble in smaller screen, the semantic is not correct.
> 
> I prefer to do the width checking in `_resizeUI` to add/remove the `onboarding-speech-bubble` class based on `_isFirstSession` and `1150px`. It's harder than this CSS change but it make code flow more clear.

Updated, thanks

> nit: ===

Updated

> we can call `getIntPref` directly without declare muteDuration

Try to cache "browser.onboarding.notification.mute-duration-on-first-session-ms"
Comment hidden (mozreview-request)
(Assignee)

Comment 17

11 months ago
(In reply to Fischer [:Fischer] from comment #16)
> Comment on attachment 8901746 [details]
> Bug 1392472 - Implement the blue dot on the overlay icon button,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/173188/diff/1-2/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=301f022c834989d083607d2ffc5e254609fc17c4

Comment 18

11 months ago
Tracked for 57, this bug (among others) will track the "Onboarding spec change" work planned for early Beta57
tracking-firefox57: --- → +
(Reporter)

Comment 19

11 months ago
mozreview-review
Comment on attachment 8901746 [details]
Bug 1392472 - Implement the blue dot on the overlay icon button,

https://reviewboard.mozilla.org/r/173188/#review180376

Though local test passed, browser_onboarding_overlay_icon_button.js test fail on treeherder, please take a look on that

::: browser/extensions/onboarding/content/onboarding.css:55
(Diff revision 2)
>  }
>  
>  #onboarding-overlay-button::after {
> +  content: " ";
> +  border-radius: 50%;
> +  margin-inline-start: -10px;

The dot position seems need to twick a little to fit the spec. In spec the dot looks align with the right margin of the icon, and the top is a bit higher than the icon.

https://mozilla.invisionapp.com/share/XYD3ERUKE#/screens/248971014_New_Tab_-_Onboarding

::: browser/extensions/onboarding/content/onboarding.js:24
(Diff revision 2)
>  const BRAND_SHORT_NAME = Services.strings
>                       .createBundle("chrome://branding/locale/brand.properties")
>                       .GetStringFromName("brandShortName");
>  const PROMPT_COUNT_PREF = "browser.onboarding.notification.prompt-count";
>  const ONBOARDING_DIALOG_ID = "onboarding-overlay-dialog";
> +const ONBOARDING_MIN_NEEDED_WIDTH_PX = 960;

`ONBOARDING_MIN_WIDTH_PX` is sufficient

::: browser/extensions/onboarding/content/onboarding.js:25
(Diff revision 2)
>                       .createBundle("chrome://branding/locale/brand.properties")
>                       .GetStringFromName("brandShortName");
>  const PROMPT_COUNT_PREF = "browser.onboarding.notification.prompt-count";
>  const ONBOARDING_DIALOG_ID = "onboarding-overlay-dialog";
> +const ONBOARDING_MIN_NEEDED_WIDTH_PX = 960;
> +const SPEECH_BUBBLE_DISPLAY_THRESHOLD_PX = 1150;

how about use `SPEECH_BUBBLE_MIN_WIDTH_PX` to align with `ONBOARDING_MIN_WIDTH_PX` naming?

::: browser/extensions/onboarding/content/onboarding.js:767
(Diff revision 2)
>  
> -  _muteNotificationOnFirstSession() {
> +  get _isFirstSession() {
> +    // Should only directly return on the "false" case. Consider:
> +    //   1. On the 1st session, `_firstSession` is true
> +    //   2. During the 1st session, user resizes window so that the UI is destroyed
> +    //   3. After the required the 1st mute session, user resizes window so that the UI is re-init

After the 1st mute session,...

::: browser/extensions/onboarding/content/onboarding.js:768
(Diff revision 2)
> -  _muteNotificationOnFirstSession() {
> +  get _isFirstSession() {
> +    // Should only directly return on the "false" case. Consider:
> +    //   1. On the 1st session, `_firstSession` is true
> +    //   2. During the 1st session, user resizes window so that the UI is destroyed
> +    //   3. After the required the 1st mute session, user resizes window so that the UI is re-init
> +    // If we directly retunred on the "true" case, it would mis-judge still the 1st session.

I think this line is not necessary

::: browser/extensions/onboarding/content/onboarding.js:771
(Diff revision 2)
> +    //   2. During the 1st session, user resizes window so that the UI is destroyed
> +    //   3. After the required the 1st mute session, user resizes window so that the UI is re-init
> +    // If we directly retunred on the "true" case, it would mis-judge still the 1st session.
> +    // In fact, it's not.
> +    if (this._firstSession === false) {
> +      return this._firstSession;

can `return false` directly without call the variable again

::: browser/extensions/onboarding/content/onboarding.js:776
(Diff revision 2)
> +      return this._firstSession;
> +    }
> +    this._firstSession = true;
> +
>      if (Services.prefs.prefHasUserValue("browser.onboarding.notification.tour-ids-queue")) {
> -      // There is a queue. We had prompted before, this must not be the 1st session.
> +      // There is a queue. We had prompted tour notifications before, this must not be the 1st session.

`There is a queue, which means we had prompted tour notifications before. Therefore this is not the 1st session.`

And the comment can put before the if(Services...) condition

::: browser/extensions/onboarding/content/onboarding.js:781
(Diff revision 2)
> -      // There is a queue. We had prompted before, this must not be the 1st session.
> -      return false;
> +      // There is a queue. We had prompted tour notifications before, this must not be the 1st session.
> +      this._firstSession = false;
>      }
>  
> -    let muteDuration = Services.prefs.getIntPref("browser.onboarding.notification.mute-duration-on-first-session-ms");
> -    if (muteDuration == 0) {
> +    if (Services.prefs.getIntPref("browser.onboarding.notification.mute-duration-on-first-session-ms") === 0) {
> +      // When this is set to 0 on purpose, always judge as not the 1st session

The comment can put before if(service..) sentence
Attachment #8901746 - Flags: review?(gasolin)
(Reporter)

Updated

11 months ago
Blocks: 1396477
(Assignee)

Comment 20

11 months ago
mozreview-review-reply
Comment on attachment 8901746 [details]
Bug 1392472 - Implement the blue dot on the overlay icon button,

https://reviewboard.mozilla.org/r/173188/#review180376

> The dot position seems need to twick a little to fit the spec. In spec the dot looks align with the right margin of the icon, and the top is a bit higher than the icon.
> 
> https://mozilla.invisionapp.com/share/XYD3ERUKE#/screens/248971014_New_Tab_-_Onboarding

Per the offline talk, let's go `margin-top: -1px; margin-inline-start: -13px;`

> `ONBOARDING_MIN_WIDTH_PX` is sufficient

Updated

> how about use `SPEECH_BUBBLE_MIN_WIDTH_PX` to align with `ONBOARDING_MIN_WIDTH_PX` naming?

Updated

> After the 1st mute session,...

Fixed

> I think this line is not necessary

Removed

> can `return false` directly without call the variable again

OK

> `There is a queue, which means we had prompted tour notifications before. Therefore this is not the 1st session.`
> 
> And the comment can put before the if(Services...) condition

Updated

> The comment can put before if(service..) sentence

Updated
(Assignee)

Comment 21

11 months ago
(In reply to Fred Lin [:gasolin] from comment #19)
> Comment on attachment 8901746 [details]
> Bug 1392472 - Implement the blue dot on the overlay icon button,
> 
> https://reviewboard.mozilla.org/r/173188/#review180376
> 
> Though local test passed, browser_onboarding_overlay_icon_button.js test
> fail on treeherder, please take a look on that
> 
The test on the comment 7 was passed. And the same as you, the test always passes locally. Per the offline talk, I'm afraid this is about intermittent. Since investigation intermittent issue takes many try runs, in order to not to block the schedule, let's add the test in another follow-up bug, thanks.
(Assignee)

Updated

11 months ago
Blocks: 1396480
(Assignee)

Comment 22

11 months ago
Correct myself
(In reply to Fischer [:Fischer] from comment #21)
> (In reply to Fred Lin [:gasolin] from comment #19)
> > Comment on attachment 8901746 [details]
> > Bug 1392472 - Implement the blue dot on the overlay icon button,
> > 
> > https://reviewboard.mozilla.org/r/173188/#review180376
> > 
> > Though local test passed, browser_onboarding_overlay_icon_button.js test
> > fail on treeherder, please take a look on that
> > 
> The test on the comment 7 was passed. And the same as you, the test always
  The test on the comment 17 was passed. And the same as you, the test always
> passes locally. Per the offline talk, I'm afraid this is about intermittent.
> Since investigation intermittent issue takes many try runs, in order to not
> to block the schedule, let's add the test in another follow-up bug, thanks.
No longer blocks: 1396480
(Assignee)

Updated

11 months ago
Blocks: 1396480
Comment hidden (mozreview-request)
(Reporter)

Comment 24

11 months ago
mozreview-review
Comment on attachment 8901746 [details]
Bug 1392472 - Implement the blue dot on the overlay icon button,

https://reviewboard.mozilla.org/r/173188/#review180874
Attachment #8901746 - Flags: review?(gasolin) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(Assignee)

Updated

11 months ago
Blocks: 1395480

Comment 25

11 months ago
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b94452304c3
Implement the blue dot on the overlay icon button, r=gasolin
Keywords: checkin-needed
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1392471
(Reporter)

Updated

11 months ago
Duplicate of this bug: 1396021
https://hg.mozilla.org/mozilla-central/rev/8b94452304c3
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Comment 29

11 months ago
I have reproduced this Bug with Nightly 57.0a1 (2017-08-21) on Ubuntu 16.04, 64 Bit!

The bug's fix is now verified on latest Nightly 57.0a1

Build ID     20170905100117
User Agent   Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170906]

Comment 30

11 months ago
I have reproduced this Bug with Nightly 57.0a1 (2017-08-23) on Windows 7, 64 Bit!

The bug's fix is verified on latest Nightly!

Build ID     20170905100117
User Agent   Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170906]
I have verified that this is fixed on today's nightly.
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.
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.