Closed Bug 1392472 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: New Tab Page, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 + verified

People

(Reporter: gasolin, Assigned: Fischer)

References

(Blocks 1 open bug)

Details

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

Attachments

(3 files)

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)
Need address these issues
- in blue dot mode, show speech bubble when hovered
- address a11y change
Blocks: 1392474
Blocks: 1392475
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.
Flags: qe-verify+
Priority: -- → P2
QA Contact: jwilliams
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
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: nobody → fliu
Status: NEW → ASSIGNED
Priority: P2 → P1
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;
> }
> ```
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)
This bug should be rebased after the bug 1392468 and bug 1392467
Depends on: 1392468, 1392467
(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
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
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)
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 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+
Whiteboard: [photon-onboarding] → [photon-onboarding][photon-onboarding-newui]
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"
(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
Tracked for 57, this bug (among others) will track the "Onboarding spec change" work planned for early Beta57
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)
Blocks: 1396477
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
(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.
Blocks: 1396480
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
Blocks: 1396480
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+
Keywords: checkin-needed
Blocks: 1395480
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
https://hg.mozilla.org/mozilla-central/rev/8b94452304c3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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]
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.