remove dup test case and do the copy editing

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gasolin, Assigned: gasolin)

Tracking

unspecified
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments)

59 bytes, text/x-review-board-request
Fischer
: review+
Details
59 bytes, text/x-review-board-request
Fischer
: review+
Details
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Please assign priorities when filing new bugs that you're going to work on in the Tours component.
Priority: -- → P3
Summary: remove dup test case → remove dup test case and do the copy editing
removed the dup test case and copy editing the onboarding/ by grammarly
Comment on attachment 8924038 [details]
Bug 1412761 - copy editing;

https://reviewboard.mozilla.org/r/195258/#review200330

::: browser/extensions/onboarding/content/onboarding.js:599
(Diff revision 1)
>    }
>  
>    /**
> -   * Wrap keyboard focus within the dialog and focus on first element after last
> -   * when moving forward or last element after first when moving backwards. Do
> -   * nothing if focus is moving in the middle of the list of dialog's focusable
> +   * Wrap keyboard focus within the dialog and focus on the first element after
> +   * the last.
> +   * When moving forward or the last element after first when moving backward.

These 2 lines are not breakable. From the codes and the original comments, it is doing: On moving forward, go focusing the 1st element when the current focused element is the last one. On moving backward, go focusing the last element when the current focused element is the 1st one.

::: browser/extensions/onboarding/content/onboarding.js:774
(Diff revision 1)
>        // Set aria-hidden to true for the rest of the document.
>        [...doc.body.children].forEach(
>          child => child.id !== "onboarding-overlay" &&
>                   child.setAttribute("aria-hidden", true));
> -      // When dialog is opened with the keyboard, focus on the 1st uncomplete tour
> -      // because it will be the selected tour
> +      // When dialog is opened with the keyboard, focus on the first
> +      // incomplete tour because it will be the selected tour.

Really nit if we wanted to explore words....
1. 1st is valid for "first"
2. In our context here, "uncompleted" is more accurate because "uncompleted" means "not completed" (= the tour is not yet done) and usually "incomplete" means "not complete" (= the tour is not a complete tour)

::: browser/extensions/onboarding/data_events.md:65
(Diff revision 1)
>  
>  | KEY | DESCRIPTION |   |
>  |-----|-------------|:-----:|
>  | `addon_version` | [Required] The version of the Onboarding addon. | :one:
>  | `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
> -| `client_id` | [Required] An identifier generated by [ClientID](https://github.com/mozilla/gecko-dev/blob/master/toolkit/modules/ClientID.jsm) module to provide an identifier for this device. Auto append by `ping-centre` module | :one:
> +| `client_id` | [Required] An identifier generated by [ClientID](https://github.com/mozilla/gecko-dev/blob/master/toolkit/modules/ClientID.jsm) module to provide an identifier for this device. Auto-append by `ping-centre` module | :one:

Really nit: "... device and automatically appended by `ping-centre` module"

::: browser/extensions/onboarding/data_events.md:68
(Diff revision 1)
>  | `addon_version` | [Required] The version of the Onboarding addon. | :one:
>  | `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
> -| `client_id` | [Required] An identifier generated by [ClientID](https://github.com/mozilla/gecko-dev/blob/master/toolkit/modules/ClientID.jsm) module to provide an identifier for this device. Auto append by `ping-centre` module | :one:
> +| `client_id` | [Required] An identifier generated by [ClientID](https://github.com/mozilla/gecko-dev/blob/master/toolkit/modules/ClientID.jsm) module to provide an identifier for this device. Auto-append by `ping-centre` module | :one:
>  | `event` | [Required] The type of event. allowed event strings are defined in the below section | :one:
>  | `impression` | [Optional] An integer to record how many times the current notification tour is shown to the user. Each Notification tour can show not more than 8 times. We put `-1` when this field is not relevant to this event | :one:
> -| `ip` | [Auto populated by Onyx] The IP address of the client. Onyx does use (with the permission) the IP address to infer user's geo information so that it could prepare the corresponding tiles for the country she lives in. However, Ping-centre will NOT store IP address in the database, where only authorized Mozilla employees can access the telemetry data, and all the raw logs are being strictly managed by the Ops team and will expire according to the Mozilla's data retention policy.| :two:
> +| `ip` | [Auto populated by Onyx] The IP address of the client. Onyx does use (with the permission) the IP address to infer user's geo-information so that it could prepare the corresponding tiles for the country she lives in. However, Ping-centre will NOT store IP address in the database, where only authorized Mozilla employees can access the telemetry data, and all the raw logs are being strictly managed by the Ops team and will expire according to the Mozilla's data retention policy.| :two:

After the 2nd thought, "geo-info" should be correct one.
Attachment #8924038 - Flags: review?(fliu)
Comment on attachment 8924037 [details]
Bug 1412761 - remove dup test case;

https://reviewboard.mozilla.org/r/195256/#review200336

::: browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js:49
(Diff revision 1)
>    BrowserTestUtils.synthesizeMouseAtPoint(2, 2, {}, tab.linkedBrowser);
>    await promiseOnboardingOverlayClosed(tab.linkedBrowser);
>    await highlightClosePromise;
>    is(highlight.state, "closed", "Should close UITour highlight after closing the overlay by clicking the overlay");
>  
>    // Trigger UITour showHighlight

Here trigger

::: browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js:56
(Diff revision 1)
> -  BrowserTestUtils.synthesizeMouseAtPoint(2, 2, {}, tab.linkedBrowser);
> -  await promiseOnboardingOverlayClosed(tab.linkedBrowser);
> -  await highlightClosePromise;
> -  is(highlight.state, "closed", "Should close UITour highlight after closing the overlay by clicking the overlay");
> -
>    // Trigger UITour showHighlight again

Here trigger again right away?
Attachment #8924037 - Flags: review?(fliu)
Comment on attachment 8924037 [details]
Bug 1412761 - remove dup test case;

https://reviewboard.mozilla.org/r/195256/#review200338

::: browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js:49
(Diff revision 1)
>    BrowserTestUtils.synthesizeMouseAtPoint(2, 2, {}, tab.linkedBrowser);
>    await promiseOnboardingOverlayClosed(tab.linkedBrowser);
>    await highlightClosePromise;
>    is(highlight.state, "closed", "Should close UITour highlight after closing the overlay by clicking the overlay");
>  
>    // Trigger UITour showHighlight

removed

::: browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js:56
(Diff revision 1)
> -  BrowserTestUtils.synthesizeMouseAtPoint(2, 2, {}, tab.linkedBrowser);
> -  await promiseOnboardingOverlayClosed(tab.linkedBrowser);
> -  await highlightClosePromise;
> -  is(highlight.state, "closed", "Should close UITour highlight after closing the overlay by clicking the overlay");
> -
>    // Trigger UITour showHighlight again

keep the second one
Comment on attachment 8924038 [details]
Bug 1412761 - copy editing;

https://reviewboard.mozilla.org/r/195258/#review200330

> These 2 lines are not breakable. From the codes and the original comments, it is doing: On moving forward, go focusing the 1st element when the current focused element is the last one. On moving backward, go focusing the last element when the current focused element is the 1st one.

updated to 
```
   * Wrap keyboard focus within the dialog.
   * When moving forward, focus on the first element when the current focused
   * element is the last one.
   * When moving backward, focus on the last element when the current focused
   * element is the first one.
```

> Really nit if we wanted to explore words....
> 1. 1st is valid for "first"
> 2. In our context here, "uncompleted" is more accurate because "uncompleted" means "not completed" (= the tour is not yet done) and usually "incomplete" means "not complete" (= the tour is not a complete tour)

use `uncompleted` back

> Really nit: "... device and automatically appended by `ping-centre` module"

updated

> After the 2nd thought, "geo-info" should be correct one.

geo-information seems correct since when you google `geo-information` or `geo information` will return `geo-information` for common usage. But search `geo-info` returns `Geo-Info is a company of Specialist Surveyors...`
Comment on attachment 8924038 [details]
Bug 1412761 - copy editing;

https://reviewboard.mozilla.org/r/195258/#review200330

> geo-information seems correct since when you google `geo-information` or `geo information` will return `geo-information` for common usage. But search `geo-info` returns `Geo-Info is a company of Specialist Surveyors...`

fast typing, yes, i mean "geo-information"
Comment on attachment 8924038 [details]
Bug 1412761 - copy editing;

https://reviewboard.mozilla.org/r/195258/#review201738
Attachment #8924038 - Flags: review?(fliu) → review+
Comment on attachment 8924037 [details]
Bug 1412761 - remove dup test case;

https://reviewboard.mozilla.org/r/195256/#review201740
Attachment #8924037 - Flags: review?(fliu) → review+
thanks!
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 09318d473367 -d 59856e397ead: rebasing 432270:09318d473367 "Bug 1412761 - remove dup test case;r=Fischer"
rebasing 432271:cd3e1d4f6266 "Bug 1412761 - copy editing;r=Fischer" (tip)
merging browser/extensions/onboarding/content/onboarding.js
merging browser/extensions/onboarding/data_events.md
warning: conflicts while merging browser/extensions/onboarding/data_events.md! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
https://hg.mozilla.org/mozilla-central/rev/a8c72ed62848
https://hg.mozilla.org/mozilla-central/rev/a17c2be44903
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.