Closed Bug 1407568 Opened 7 years ago Closed 7 years ago

Add a spotlight indicator to a specific option/ section

Categories

(Firefox :: Settings UI, enhancement, P4)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: Tina_Hsieh, Assigned: evanxd)

References

Details

(Whiteboard: [preference-spotlight])

Attachments

(5 files)

spotlight-an-ui-compoent.png
606.66 KB, image/png
Details
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.
59 bytes, text/x-review-board-request
jaws
: review+
lchang
: review+
HHuang
: ui-review+
Details
spotlight-allow-send-data-to-mozilla.png
507.52 KB, image/png
Details
spotlight-high-contrast-mode.png
57.18 KB, image/png
Details
spotlight-ui-review.png
607.03 KB, image/png
Details 
When a user is led to Preferences by a link/ button from any UI in Firefox, we need to indicate which part of the options is related. 

For example, a form autofill prompt which contains a link to Preferences should lead users to Preferences and indicate the related option so that we can prevent users from getting lost.

See UX spec below:
https://mozilla.invisionapp.com/share/HSCNDG9UG#/screens/243549772
Note that it's possible to filter preferences to show only a specific subcategory on links like DRM does it https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/browser/base/content/browser-media.js#172

IMO that's even better than highlighting. We should at least get a consistent way of linking to preferences across the board.
Priority: P1 → --
CC Jared and Mike.
I agree with Johann in comment #1. We should add more data-subcategory searches that we can use here (we can't use the direct text-based search API since that would be different per locale).

See for example http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/browser/base/content/browser-data-submission-info-bar.js#66 and http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/browser/components/preferences/in-content/preferences.js#117-122

Note that we may still want to do the highlight that Tina has outlined, since some of the subcategories may be large enough that simply opening to the subcategory may still be confusing to the user as to which row they should look at.
Priority: -- → P4
Assignee: nobody → evan
Status: NEW → ASSIGNED
Hey Johann and Jared,
Thanks for the information! We know that the current implementation is to hide the options which are not related. But given that we're going to introduce an indicator/highlighter to point the option out, we prefer to not hiding any options in Preferences. We can scroll the page to the related section instead, so users will never feel confused about looking at a special version of Preferences.

I agree that we should use the consistent way to link to Preferenecs, so we've discussed all possible use cases and will try our best to make it flexible for any requirements in the future.
This bug is trying to create a API to lead users to Preferences's specific section or UI component by a link/ button from any UI in Firefox. For instance, the autofill system addon could use the API to lead users to setup autofill settings, just like the spec mentioned[1].

And I think we could reuse the `window.openPreferences` method to do it. We could add some new params and handle them in `gotoPref` function[2] in the Preferences codebase.

What the Spotlight feature want to do is scrolling to (or showing) specific section and indicating specific UI component on Preferences page. So we could add two new `urlParams` params for the `openPreferences` API function.
- `subcategory`
  - It is for scrolling to (or showing) a specific section.
- `componentid`
  - It is for indicating a specific UI component which belongs to the section.

And for helping `gotoPref` function to find out the `subcategory` section and the `componentid` UI component. We also need to add two more element attributes(`data-category` and `data-componentid`) in the Preferences XUL documents. For example, for searching the "Forms & Passwords" section and "Autofill credit cards" UI component, let's update the XUL document like the below.

```
<groupbox id="passwordsGroup" orient="vertical" data-category="panePrivacy" data-subcategory="password" hidden="true">
```

```
<hbox id="creditCardAutofill" data-componentid="creditCardAutofill">
  <description flex="1">
    <checkbox class="tail-with-learn-more" label="Autofill credit cards" checked="true"/>
    <label class="learnMore text-link" id="creditCardAutofillLearnMore" value="Learn more" href="https://support.mozilla.org/1/firefox/58.0a1/Darwin/en-US/autofill-card-address"/>
  </description>
  <hbox>
    <button class="accessory-button" label="Saved Credit Cards…"/>
  </hbox>
</hbox>
```

Then the `openPreferences` method will called like the below to indicate the `creditCardAutofill` UI component.

```
window.openPreferences("privacy",{
  urlParams: {
    subcategory: "password",
    componentid: "creditCardAutofill"
  },
  origin: "aboutHome"
});
```

Once we have the two new params, we also could use them to replace the current `openPreferences` calls in the codebase. For instance, `mainWindow.openPreferences("privacy-reports", { origin: "experimentsOpenPref" });` to `mainWindow.openPreferences("privacy", { urlParams: { subcategory: "reports" }, origin: "experimentsOpenPref" });`[3]. It might be easier to understand for API users, I think.

I hope the adding new params idea is good for us.
What do you think, Jared?

[1]: https://mozilla.invisionapp.com/share/HSCNDG9UG#/screens/243549775
[2]: http://searchfox.org/mozilla-central/diff/0640b9ab0f192b2c6c3713b5b72eb88d4bd3dba0/browser/components/preferences/in-content-old/preferences.js#158
[3]: http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1509
Flags: needinfo?(jaws)
We will still need to support the URL API (about:preferences#privacy-reports) since not all callers can use a JavaScript API (for example command-line usage).

You could repurpose the current subcategory implementation to do the scrolling and highlighting, using the subcategory name as the key to determine which element to scroll to and highlight. You shouldn't need subcategories anymore according to comment #4.
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> We will still need to support the URL API
> (about:preferences#privacy-reports) since not all callers can use a
> JavaScript API (for example command-line usage).
Good point. Let's keep the URL API.

> You could repurpose the current subcategory implementation to do the
> scrolling and highlighting, using the subcategory name as the key to
> determine which element to scroll to and highlight.
I think we might still need to have a new param or something to deal with some specific situations, for instance, we would like scroll to "Nightly Data Collection and Use" title and spotlight the "Allow Nightly to send crash reports to Mozilla" UI component, like the "spotlight-an-ui-compoent.png" attachment.

In Tina's idea, we should always scroll to some specific second level title (for example, Nightly Data Collection and Use) or first level title (if there is no second level title), and spotlight a specific UI component (for example, Allow Nightly to send crash reports to Mozilla).

We could do one of the below solution to do so.
1. The params solution:
```
```
window.openPreferences("privacy",{
  urlParams: {
    subcategory: "datacollection",
    componentid: "crashReports"
  },
  origin: "aboutHome"
});
```

2. The URI solution:
```
about:preferences#privacy-datacollection-crashreports
```

Is anyone of the above solution a good idea? Or there might be any other good idea? What do you think?

> You shouldn't need
> subcategories anymore according to comment #4.
Sorry, I didn't get this. What do you mean "subcategories" here? What I proposed on Comment 5 is adding the two params, "subcategory" and "componentid".
Flags: needinfo?(jaws)
What I mean is, our "subcategories" are hardcoded, meaning we control where "privacy-reports" takes the user.

We could change "privacy-reports" to "privacy-crashreports", then in our code that searches for the subcategory, it could find the crashreports item, highlight the crashreports item, but still scroll the page to the closest <groupbox> ancestor. If we needed something more complicated than scrolling to the closest <groupbox> ancestor, we could store a mapping in our JavaScript code or in the markup as an attribute.
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> We could change "privacy-reports" to "privacy-crashreports", then in our
> code that searches for the subcategory, it could find the crashreports item,
> highlight the crashreports item, but still scroll the page to the closest
> <groupbox> ancestor. If we needed something more complicated than scrolling
> to the closest <groupbox> ancestor, we could store a mapping in our
> JavaScript code or in the markup as an attribute.

Got it and it make sense. Let's do it.
Whiteboard: [photon-preference][triage] → [photon-preference]
Updated the WIP patch.
Whiteboard: [photon-preference] → [preference-spotlight]
Discussed the visual design of the spotlight section with Helen and Tina. The conclusion is setting the `background-color` as `rgba(0, 200, 215, 0.3)` for the spotlight section, like the "spotlight-allow-send-data-to-mozilla.png" attachment.

And what we need in the UX spec is adding specific examples to show what UI components will be a group to be spotlighted.

Hi Tina,

Could you update the spec here once you adding the examples.

Thank you.
Flags: needinfo?(thsieh)
Discussed with Helen and a colleague who need more color contrast in his/her daily life for the spotlight visual design in high contrast mode. And We will add a border in high contrast mode, just like we did for the `dialogBox`[1] and the "spotlight-high-contrast-mode.png" attachment.

[1]: http://searchfox.org/mozilla-central/diff/1551a6d72ac95a00b33464df75d44c1a7ea2af71/browser/themes/shared/incontentprefs/preferences.inc.css#239
Hi Evan,
Please see the latest spec which I just updated with the spotlight areas. 
https://mozilla.invisionapp.com/share/HSCNDG9UG#/261908945_5-3_Option_Groups1
Flags: needinfo?(thsieh)
Target Milestone: Firefox 58 → ---
Version: 58 Branch → unspecified
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review201438

::: browser/components/preferences/in-content/preferences.js:182
(Diff revision 6)
> +  // TODO: Need to ensure all elements are already rendered
> +  // before doing scroll and highlight,
> +  // or the groupbox ancestor's position might be not correct
> +  // in the scrollTo function.
> +  if (subcategory) {
> +    setTimeout(() => {

This 500ms setTimeout is a workaround to wait for all elements are rendered. Then we could get the correct position of closest groupbox ancestor and scroll to there. I'm not sure do we have any better idea to wait for the state?

::: browser/themes/shared/incontentprefs/preferences.inc.css:101
(Diff revision 6)
>  }
>  
> +.spotlight {
> +  background-color: rgba(0, 200, 215, 0.3);
> +  /* Show the border to spotlight the components in high-contrast mode. */
> +  border: 1px solid transparent;

This transparent border is for shot the spotlight in the high-contrast mode. What do you think, Jared? This refers to here[1], and it looks like this[2].

[1]: http://searchfox.org/mozilla-central/diff/1551a6d72ac95a00b33464df75d44c1a7ea2af71/browser/themes/shared/incontentprefs/preferences.inc.css#239
[2]: https://bug1407568.bmoattachments.org/attachment.cgi?id=8924407

::: browser/themes/shared/incontentprefs/preferences.inc.css:104
(Diff revision 6)
> +  background-color: rgba(0, 200, 215, 0.3);
> +  /* Show the border to spotlight the components in high-contrast mode. */
> +  border: 1px solid transparent;
> +}
> +
> +[data-subcategory="reports"] {

Add the 4px padding-inline-start and padding-inline-end for each row of UI components in Preferences page, then the spotlight area could have the padding (better visual look). If we only add the padding for `[data-subcategory="reports"]`, and this row will not left alight other rows, like the screenshot [1]. 

The straight way to make all rows left align to same place and make spotlight area have the padding is adding same (4px) padding, which also means we will need to declare many css selectors and add padding style for them to do this.

What do you think of this idea, Jared? Or do you have better way to make spotlight area's backgroud-color have the 4px padding.

[1]: https://bug1407568.bmoattachments.org/attachment.cgi?id=8924402
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

Hi Jared,

Could you give feedback for the patch?

I added comments to discuss the below topics about the patch.
- The timing issue to scroll.
- How to spotlight in high-contrast mode?
- How to add (4px) padding for spotlight area and make all rows left align same place? (The screenshot[1] shows the UI components in the spotlight area don't left align other rows)

Thank you very much.

[1]: https://bug1407568.bmoattachments.org/attachment.cgi?id=8924402
Attachment #8923671 - Flags: feedback?(jaws)
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review201466

::: browser/components/preferences/in-content/preferences.js:182
(Diff revision 6)
> +  // TODO: Need to ensure all elements are already rendered
> +  // before doing scroll and highlight,
> +  // or the groupbox ancestor's position might be not correct
> +  // in the scrollTo function.
> +  if (subcategory) {
> +    setTimeout(() => {

I think this timing issue for scrolling to correct position might related with the search function since it shows or hides elements by `element.hidden = false;` or `element.hidden = true;`.
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review201470

::: browser/themes/shared/incontentprefs/preferences.inc.css:104
(Diff revision 6)
> +  background-color: rgba(0, 200, 215, 0.3);
> +  /* Show the border to spotlight the components in high-contrast mode. */
> +  border: 1px solid transparent;
> +}
> +
> +[data-subcategory="reports"] {

The 4px padding is also for matching our visual spec.
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review201962

::: browser/components/preferences/in-content/preferences.js:182
(Diff revision 6)
> +  // TODO: Need to ensure all elements are already rendered
> +  // before doing scroll and highlight,
> +  // or the groupbox ancestor's position might be not correct
> +  // in the scrollTo function.
> +  if (subcategory) {
> +    setTimeout(() => {

This will be too error prone to use.

You can use BrowserUtils.promiseLayoutFlushed to wait for layout to be flushed, and then inside of that callback you can put this code inside of a requestAnimationFrame callback.

::: browser/components/preferences/in-content/preferences.js:183
(Diff revision 6)
> +  // before doing scroll and highlight,
> +  // or the groupbox ancestor's position might be not correct
> +  // in the scrollTo function.
> +  if (subcategory) {
> +    setTimeout(() => {
> +      let groupboxAncestor = getClosestGroupboxAncestor(subcategory);

You can replace this function with:
```
let groupboxAncestor = document.querySelector(`[data-subcategory="${subcategory}"]`).closest("groupbox");
```

::: browser/themes/shared/incontentprefs/preferences.inc.css:98
(Diff revision 6)
>    height: 30px;
>    min-width: 150px;
>    margin: 4px 0;
>  }
>  
> +.spotlight {

The spec shows a small border radius on the spotlight. Can you get the correct value from Tina?

::: browser/themes/shared/incontentprefs/preferences.inc.css:99
(Diff revision 6)
>    min-width: 150px;
>    margin: 4px 0;
>  }
>  
> +.spotlight {
> +  background-color: rgba(0, 200, 215, 0.3);

nit, no spaces after commas in color functions.

::: browser/themes/shared/incontentprefs/preferences.inc.css:104
(Diff revision 6)
> +  background-color: rgba(0, 200, 215, 0.3);
> +  /* Show the border to spotlight the components in high-contrast mode. */
> +  border: 1px solid transparent;
> +}
> +
> +[data-subcategory="reports"] {

Why are we only adding a rule for 'reports'?

Also, this selector should add a class selector to it since right now it is using the universal selector and checking all elements on the page for this attribute.

::: browser/themes/shared/incontentprefs/preferences.inc.css:104
(Diff revision 6)
> +  background-color: rgba(0, 200, 215, 0.3);
> +  /* Show the border to spotlight the components in high-contrast mode. */
> +  border: 1px solid transparent;
> +}
> +
> +[data-subcategory="reports"] {

To solve this, you can add a negative margin-left and margin-right of equal size so that the text still aligns but the highlight extends outside of the column. Like so:

[data-subcategory="reports"] {
  background: rgba(0, 200, 215, 0.3);
  border-radius: 2px;
  margin-left: -4px;
  margin-right: -4px;
  padding-left: 4px;
  padding-right: 4px;
}

[data-subcategory="reports"] > .groupbox-title {
  padding-inline-start: 4px;
}

That last line for .groupbox-title is necessary because .groupbox-title has pack="start" attribute.
Attachment #8923671 - Flags: feedback?(jaws) → feedback+
Updated the patch to fix the timing issue of scrolling to the position of a specific header.
Attached image spotlight-ui-review.png — 

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

Hi Helen,

Could you review the UI change[1] of the patch?
You could check the screenshot[1] to see the detail.
And I added a scroll animation, just like I showed you before.

[1]: https://bug1407568.bmoattachments.org/attachment.cgi?id=8926771

        Attachment #8923671 -
        Flags: ui-review?(hhuang)

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

The UI looks good to me, and thanks for adding the animation that improves the effect of indicator even better.

        Attachment #8923671 -
        Flags: ui-review?(hhuang) → ui-review+

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review201962

> The spec shows a small border radius on the spotlight. Can you get the correct value from Tina?

Yes, Helen think we could add the `border-radius: 2px` style, and we got the ui-review+ from her.

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review203316

::: browser/components/preferences/in-content/preferences.js:44
(Diff revision 11)
> +  /**
> +   * Wait for all content in Preferences is loaded.
> +   * (includes Preferences elements and other elements added by 3rd-party addons)
> +   * @returns {Promise} - The promise will resolve if all content is loaded.
> +   */
> +  waitForAllContentLoaded() {

For the timing issue of waiting for all elements are rendered, I found out it is caused by the form-autofill system addon[1]. It will add new element (some form form-autofill UI compoents). So we should find a good way to wait for the timing.

I discussed with Scott (the autofill project team member), we think we could use the below idea to fix the timing issue.
- The form-autofill addon will add new pref, such as `preferences.addon.formautofill.visible` as true of false to let Preferences module to know that form-autofill will add new elements into Preferences XUL document.
- Once form-autofill addon has added new elements into Preferences XUL document, it will notify a "preferences-addon-loaded" observer then Preferences will know the DOM injection is finished.

The `ContentLoadingObserver` object is implemented for doing the above idea. 

And we also will write tests to protect the code and avoid that other addons might add new element into Preferences without exporting a new pref (`preferences.addon.xxx.visible`).

I think it is a simple and easy way to fix the timing issue. What do you think of it, Jared?

[1]: http://searchfox.org/mozilla-central/source/browser/extensions/formautofill

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

Hi Jared,

Could you help review the patch?

If you think the `ContentLoadingObserver` object is good approach to fix the timing issue, I will continue to implement the `_getPreferencesVisibleAddonList` to read the prefs.

Thank you very much.

        Attachment #8923671 -
        Flags: review?(jaws)

    
    

    
  
CC Scott.

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review203316

> For the timing issue of waiting for all elements are rendered, I found out it is caused by the form-autofill system addon[1]. It will add new element (some form form-autofill UI compoents). So we should find a good way to wait for the timing.
> 
> I discussed with Scott (the autofill project team member), we think we could use the below idea to fix the timing issue.
> - The form-autofill addon will add new pref, such as `preferences.addon.formautofill.visible` as true of false to let Preferences module to know that form-autofill will add new elements into Preferences XUL document.
> - Once form-autofill addon has added new elements into Preferences XUL document, it will notify a "preferences-addon-loaded" observer then Preferences will know the DOM injection is finished.
> 
> The `ContentLoadingObserver` object is implemented for doing the above idea. 
> 
> And we also will write tests to protect the code and avoid that other addons might add new element into Preferences without exporting a new pref (`preferences.addon.xxx.visible`).
> 
> I think it is a simple and easy way to fix the timing issue. What do you think of it, Jared?
> 
> [1]: http://searchfox.org/mozilla-central/source/browser/extensions/formautofill

I think this approach will add a lot of complexity and it is likely that future system addons will not remember to do this.

Can you use a MutationObserver instead? Then you should disconnect the mutation observer once the user starts to interact or scroll the page since we shouldn't change the scrollPosition once they have started to use the page.

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review204708

r- due to the usage of ContentLoadingObserver. See my most recent comment about MutationObserver.

::: browser/components/preferences/in-content/preferences.js:271
(Diff revisions 6 - 11)
>        element.setAttribute("disabled", true);
>    }
>  }
>  
> -// TODO: Search closest groupbox ancestor.
> -function getClosestGroupboxAncestor(subcategory) {
> +async function spotlight(subcategory) {
> +  let highlightedElement = document.querySelector(".spotlight");

This should be changed to querySelectorAll and then loop on all matches.

::: browser/components/preferences/in-content/preferences.js:304
(Diff revisions 6 - 11)
> +  const SEARCH_CONTAINER_HEIGHT = document.querySelector(".search-container").clientHeight;
> +  let mainContent = document.querySelector(".main-content");
> +  let top = element.getBoundingClientRect().top - SEARCH_CONTAINER_HEIGHT;
> +  mainContent.scroll({
> +    top: top,
> +    left: 0,
> +    behavior: "smooth",
> +  });

This can all be replaced with Element.scrollIntoView. The code as-is has two sync reflows and I'm not sure if the `left: 0` in the code is RTL-friendly.

https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView

        Attachment #8923671 -
        Flags: review?(jaws) → review-

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #38)
> Can you use a MutationObserver instead? Then you should disconnect the
> mutation observer once the user starts to interact or scroll the page since
> we shouldn't change the scrollPosition once they have started to use the
> page.
Yes, I think we could use MutationObserver to observe the dom tree change caused by the form-autofill addon and remove the "preferences-addon-loaded" observer in current patch.

And I think we still need to add and check the `preferences.addon.formautofill.visible` pref (about:config) to know is the form-autofill addon enable/visible and is it going to insert elements into Preferences. If the pref is true, we should add the MutationObserver to observe the dom change and scroll the page. If it is false, then we don't need to add the MutationObserver.

Does the idea in above paragraph make sense, or you have a better idea to know when do we need to add the MutationObserver and when could we scroll the page since we need to wait for whole page is finished rendering (include form-autofill's element insertion) and start to scroll or we might scroll to wrong position?
Flags: needinfo?(jaws)

    
    

    
  
I think the pref is unneeded and will still add more complexity.

If we are going to highlight something (and scroll to it), then we could always set up the MutationObserver and disconnect it once the user interacts with the page (passive scroll event listener, mousedown, keydown).
Flags: needinfo?(jaws)

    
    

    
  
The real issue here is that form autofill is changing the page after it has loaded. This will create a janky appearance to the user and should be avoided. Scott, can you think of another way to add your elements to the preferences instead of showing/hiding them after the page has loaded?
Flags: needinfo?(scwwu)

    
    

    
  
One approach would be to have an earlier event or observer notification that form autofill and other extensions can listen to and synchronously inject their content. This would work for more than autofill.

For autofill specifically I wouldn't be totally against baking the top-level autofill prefs UI into m-c as it seems like the feature is here to stay. We would still want to keep the subdialogs in the system add-on (SAO) as they are undergoing changes.

    
    

    
  
I agree with the idea of having the autofill prefs UI in m-c instead of injecting it from SAO. I do think there's a need for an additional pref that indicates whether or not autofill should be visible or not. Currently, the check is done in the bootstrap phase[1], since it requires checking browser locale and region. We could create a pref that saves the result, and set visibility based on that. 

I do wonder if we could keep the preference related strings at the SAO, or would it be better to move preference specific strings into m-c as well?

1: https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/browser/extensions/formautofill/bootstrap.js#55-65
Flags: needinfo?(scwwu)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review204708

> This should be changed to querySelectorAll and then loop on all matches.

Sure, let's do it.

    
    

    
      
  

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review205356

::: browser/components/preferences/in-content/preferences.js:228
(Diff revision 14)
> +      element.classList.remove("spotlight");
> +    });
> +  }
> +  if (subcategory) {
> +    if (!gSearchResultsPane.categoriesInitialized) {
> +      // Wait for system addon finished its dom injection.

Because of some previous confusions, I created the `ContentLoadingObserver` object to ensure autofill system addon finished its injection.

After investigated preferences and form-autofill codebase, I think we could just simply add a `sync-pane-loaded` observer to wait for autofill system addon finishes its injection and then start to scroll since the observer is executed synchronously.

Does it make sense?

::: browser/components/preferences/in-content/preferences.js:267
(Diff revision 14)
> +}
> +
> +function scrollContentTo(element) {
> +  const SEARCH_CONTAINER_HEIGHT = document.querySelector(".search-container").clientHeight;
> +  let mainContent = document.querySelector(".main-content");
> +  let top = element.getBoundingClientRect().top - SEARCH_CONTAINER_HEIGHT;

This code might not be replaced with `Element.scrollIntoView` since using `Element.scrollIntoView` to scroll the element it will be covered by the sticky `search-container` header[1] and I didn't find out any solution to avoid this issue. Or do you have any solution to fix it?

And let's remove the `left: 0` here since we only want to scroll the vertical position of the scroll bar. (I think the `left: 0` is RTL-friendly since `scrollLeft` is RTL-friendly.)

[1]: https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/preferences.xul#192
[2]: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollLeft

        Attachment #8923671 -
        Flags: review?(evan)

    
  

        Attachment #8923671 -
        Flags: review?(evan)

    
  

        Attachment #8923671 -
        Flags: review?(jaws)

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

Hi Jared,

I've updated the patch for the review comments and fixing the timing issue of system addon's dom injection.
Could you help it again?

Thank you very much.

        Attachment #8923671 -
        Flags: review?(jaws)

    
    

    
  
Hi Jared,

Just a kindly reminder.

Could you give feedback for that we wait for "sync-pane-loaded" observer to fix the timing issue of scrolling the page in the patch, just like Comment 49 mentioned and review patch again once you have time?

Thank you very much.
Flags: needinfo?(jaws)

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review207466

::: browser/components/preferences/in-content/preferences.js:221
(Diff revisions 11 - 14)
> -  if (highlightedElement) {
> -    highlightedElement.classList.remove("spotlight");
> +  if (highlightedElements.length) {
> +    highlightedElements.forEach(element => {
> +      element.classList.remove("spotlight");
> +    });

Array.forEach is considered slow. Please use `for (let element of highlightedElements) {` instead.

::: browser/components/preferences/in-content/preferences.js:228
(Diff revisions 11 - 14)
> +      // Wait for system addon finished its dom injection.
> +      await new Promise((resolve) => {
> +        Services.obs.addObserver(function waitForInjectionFinished() {
> +          Services.obs.removeObserver(waitForInjectionFinished, "sync-pane-loaded");
> +          resolve();
> +        }, "sync-pane-loaded");
> +      });

Waiting for the system addon to load and waiting for 'sync-pane-loaded' are not guaranteed to happen in the order that this code expects. There is no guarantee that one will always happen before the other.

I think it would be wrong to make this assumption.

If the system addon has a pref that is set to true when it is installed/enabled, maybe you can just check that pref, and if that pref is true you can await a Promise that will resolve when the specific element is injected (look for the element by ID).

        Attachment #8923671 -
        Flags: review?(jaws) → review-
Flags: needinfo?(jaws)

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #52)
> ::: browser/components/preferences/in-content/preferences.js:228
> (Diff revisions 11 - 14)
> > +      // Wait for system addon finished its dom injection.
> > +      await new Promise((resolve) => {
> > +        Services.obs.addObserver(function waitForInjectionFinished() {
> > +          Services.obs.removeObserver(waitForInjectionFinished, "sync-pane-loaded");
> > +          resolve();
> > +        }, "sync-pane-loaded");
> > +      });
> 
> Waiting for the system addon to load and waiting for 'sync-pane-loaded' are
> not guaranteed to happen in the order that this code expects. There is no
> guarantee that one will always happen before the other.
> 
> I think it would be wrong to make this assumption.
> 
> If the system addon has a pref that is set to true when it is
> installed/enabled, maybe you can just check that pref, and if that pref is
> true you can await a Promise that will resolve when the specific element is
> injected (look for the element by ID).

The pref (which will be set true when it will be visible) is not existed yet, but let's add one.
But let's add it and check it to do the waiting thing, just like Scott mentioned at Comment 44.

    
    

    
      
  

    
  

        Attachment #8923671 -
        Flags: review?(jaws) → review?(lchang)

    
  

        Attachment #8923671 -
        Flags: review?(jaws)

    
    

    
  
Hi Luke,

Could you help review the form-autofill part of the patch?
I added a new pref `extensions.formautofill.visible` to tell if the form-autofill wil be visible or not there.

Thank you very much.

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review207466

> Array.forEach is considered slow. Please use `for (let element of highlightedElements) {` instead.

Sure, let's do it.

> Waiting for the system addon to load and waiting for 'sync-pane-loaded' are not guaranteed to happen in the order that this code expects. There is no guarantee that one will always happen before the other.
> 
> I think it would be wrong to make this assumption.
> 
> If the system addon has a pref that is set to true when it is installed/enabled, maybe you can just check that pref, and if that pref is true you can await a Promise that will resolve when the specific element is injected (look for the element by ID).

Sure, let's do it.

    
    

    
      
  

    
    

    
  
Hi Jared,

I've updated the patch for the review comments.
Could you help review it again?

Thank you very much.
Flags: needinfo?(jaws)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review208354

FormAutofill part looks good. I'm only concerned about the naming.

::: browser/app/profile/firefox.js:1695
(Diff revision 20)
>  
>  // Preferences for the form autofill system extension
>  // The truthy values of "extensions.formautofill.available" are "on" and "detect",
>  // any other value means autofill isn't available.
>  // "detect" means it's enabled if conditions defined in the extension are met.
> +pref("extensions.formautofill.visible", false);

The naming, `extensions.formautofill.visible`, may be confusing as we already have `available` and `enabled` (and many people have been confused with these two). Also, we should somehow hint users that it should be aligned with `available` and they shouldn't be modified separately. I'm thinking maybe `extensions.formautofill.available.result` would be a clearer name. Though we'll indeed need a comment for this to explain what it is for. What do you think?

BTW, the comment above doesn't apply to this new pref so I suggest moving it (and its comment) below `#endif`.

        Attachment #8923671 -
        Flags: review?(lchang)

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review208354

> The naming, `extensions.formautofill.visible`, may be confusing as we already have `available` and `enabled` (and many people have been confused with these two). Also, we should somehow hint users that it should be aligned with `available` and they shouldn't be modified separately. I'm thinking maybe `extensions.formautofill.available.result` would be a clearer name. Though we'll indeed need a comment for this to explain what it is for. What do you think?
> 
> BTW, the comment above doesn't apply to this new pref so I suggest moving it (and its comment) below `#endif`.

Sure, let's do it.

    
    

    
      
  

    
    

    
  
Hi Luke,

Thank you for previous review.
I've updated the patch for your review comments.
Could you help review it again?

Thank you very much.
Flags: needinfo?(lchang)

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review208422

Thanks.

::: browser/app/profile/firefox.js:1709
(Diff revision 21)
>  pref("extensions.formautofill.creditCards.available", false);
>  #else
>  pref("extensions.formautofill.available", "detect");
>  pref("extensions.formautofill.creditCards.available", true);
>  #endif
> +// The pref will be true if autofill system extension is enabled, otherwise keep it as false.

nit: The pref can be used to determine whetcher autofill is actually enabled especially when "extensions.formautofill.available" is set to "detect". Please keep it "false" here as it's supposed to be flipped at runtime only.

        Attachment #8923671 -
        Flags: review?(lchang) → review+
Flags: needinfo?(lchang)

    
    

    
      
  

    
    

    
  
(In reply to Luke Chang [:lchang] from comment #68)
> nit: The pref can be used to determine whetcher autofill is actually enabled
> especially when "extensions.formautofill.available" is set to "detect".
> Please keep it "false" here as it's supposed to be flipped at runtime only.

Sure, let's update the comment. And thank you for the review, Luke.

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review208536

r=me with the issues listed below fixed.

::: browser/components/preferences/in-content/preferences.js:253
(Diff revisions 14 - 22)
> +      let observer = new MutationObserver(mutations => {
> +        for (let mutation of mutations) {
> +          for (let node of mutation.addedNodes) {
> +            elementIdSet.delete(node.id);
> +            if (elementIdSet.size === 0) {
> +              observer.disconnect();
> -          resolve();
> +              resolve();
> -        }, "sync-pane-loaded");
> +            }
> +          }
> +        }
>        });

Please move this inside of the `if (elementIdSet.size > 0) {` block since it doesn't need to be created if there is no elementIdSet items.

::: browser/components/preferences/in-content/preferences.js:264
(Diff revisions 14 - 22)
> -          resolve();
> +              resolve();
> -        }, "sync-pane-loaded");
> +            }
> +          }
> +        }
>        });
> +      if (elementIdSet.size > 0) {

This can be simplified to just `if (elementIdSet.size) {`

::: browser/components/preferences/in-content/preferences.js:268
(Diff revisions 14 - 22)
> +        mainContent.addEventListener("scroll", () => { observer.disconnect(); }, {once: true});
> +        window.addEventListener("mousedown", () => { observer.disconnect(); }, {once: true});
> +        window.addEventListener("keydown", () => { observer.disconnect(); }, {once: true});

Each of these should remove the other event listeners too. You should pull out `() => { observer.disconnect(); }` to a separate function and then within it remove the event listeners from all three event targets.

::: browser/components/preferences/in-content/tests/browser_bug1020245_openPreferences_to_paneContent.js:28
(Diff revision 22)
>    is(prefs.selectedPane, "paneGeneral", "General pane is selected by default");
>    prefs = await openPreferencesViaOpenPreferencesAPI("privacy-reports", {leaveOpen: true});
>    is(prefs.selectedPane, "panePrivacy", "Privacy pane is selected by default");
>    let doc = gBrowser.contentDocument;
>    is(doc.location.hash, "#privacy", "The subcategory should be removed from the URI");
> -  ok(doc.querySelector("#locationBarGroup").hidden, "Location Bar prefs should be hidden when only Reports are requested");
> +  await BrowserTestUtils.waitForCondition(() => doc.querySelector(".spotlight"));

Replace BrowserTestUtils with TestUtils here and below.

Also, please supply a second argument to waitForCondition with a message as to what you are expecting.

::: browser/components/preferences/in-content/tests/browser_spotlight.js:8
(Diff revision 22)
> +add_task(async function test_reports_section() {
> +  let prefs = await openPreferencesViaOpenPreferencesAPI("privacy-reports", {leaveOpen: true});
> +  is(prefs.selectedPane, "panePrivacy", "Privacy pane is selected by default");
> +  let doc = gBrowser.contentDocument;
> +  is(doc.location.hash, "#privacy", "The subcategory should be removed from the URI");
> +  await BrowserTestUtils.waitForCondition(() => doc.querySelector(".spotlight"));

Same comments here as I gave for browser_bug1020245_openPreferences_to_paneContent.js.

::: browser/components/uitour/test/browser_openPreferences.js:49
(Diff revision 22)
>    let tab = await promiseTabOpened;
>    await BrowserTestUtils.waitForEvent(gBrowser.selectedBrowser, "Initialized");
>    let doc = gBrowser.selectedBrowser.contentDocument;
> -  let reports = doc.querySelector("groupbox[data-subcategory='reports']");
>    is(doc.location.hash, "#privacy", "Should not display the reports subcategory in the location hash.");
> -  is(reports.hidden, false, "Should open to the reports subcategory in the privacy pane in the new Preferences.");
> +  await BrowserTestUtils.waitForCondition(() => doc.querySelector(".spotlight"));

Same comments here too.

        Attachment #8923671 -
        Flags: review?(jaws) → review+
Flags: needinfo?(jaws)

    
    

    
  
Comment on attachment 8923671 [details]
Bug 1407568 - Add a spotlight indicator to a specific section and UI component.

https://reviewboard.mozilla.org/r/194792/#review208536

> Please move this inside of the `if (elementIdSet.size > 0) {` block since it doesn't need to be created if there is no elementIdSet items.

Sure.

> This can be simplified to just `if (elementIdSet.size) {`

Sure.

> Each of these should remove the other event listeners too. You should pull out `() => { observer.disconnect(); }` to a separate function and then within it remove the event listeners from all three event targets.

Good point. Let's do it.

> Replace BrowserTestUtils with TestUtils here and below.
> 
> Also, please supply a second argument to waitForCondition with a message as to what you are expecting.

Sure. After read the source code, learned why we should use TestUtils here. Thank you.

> Same comments here as I gave for browser_bug1020245_openPreferences_to_paneContent.js.

Sure.

> Same comments here too.

Sure.

    
    

    
      
  

    
    

    
  
Discussed with Jared, Luke, and Matthew, we would like to add a new helper function (like `isAutofillAvailable` function) to get the form autofill system addon's stage to instead to the `extensions.formautofill.available.result` pref in the current patch[1].

And Luke would like to help file the new bug and implement the helper function. Thank you very much for the help, Luke.

Let's wait the Bug is landed and then land this bug.

[1]: https://reviewboard.mozilla.org/r/194792/
Flags: needinfo?(lchang)

    
    

    
  
Bug 1421551 filed. Will implement it as soon as possible. Thanks for helping Form Autofill.
Flags: needinfo?(lchang)

    
    

    
      
  

    
    

    
  
Updated the `waitForSystemAddonInjectionsFinished` function, add a new param `isGoingToInject` to determine a element will be injected into Preferences or not. Then we could use new form-autofill helper function to assign the value into `isGoingToInject`.

    
    

    
      
  

    
    

    
  
Updated the patch to use `formAutofillParent.initialized` to determine a element will be injected into Preferences or not. Wait for Bug 1421551's patch is landed (into autoland), and then let's land this patch.

    
    

    
      
  

    
    

    
  
Updated patch to depend on the version 3[1] of Bug 1421551's patch and sent a new try[2]. If everything is good, let's land the code after Bug 1421551's patch is landed.

[1]: https://reviewboard.mozilla.org/r/203838/diff/3/
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14cd92cca125

    
    

    
  
The try[1] looks good. Let's land the patch.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14cd92cca125
Keywords: checkin-needed

    
    

    
  
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/098b128e39b1
Add a spotlight indicator to a specific section and UI component. r=jaws,lchang
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/098b128e39b1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox59:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
See Also:  → 1512187

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: