Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The checkboxes in the publish panel are misleading #1957

Closed
julienw opened this issue Apr 18, 2019 · 11 comments
Closed

The checkboxes in the publish panel are misleading #1957

julienw opened this issue Apr 18, 2019 · 11 comments
Labels
polish Small features or changes that do not require planning to work on. These help out our end users. privacy Data sanitization, and privacy concerns

Comments

@julienw
Copy link
Contributor

julienw commented Apr 18, 2019

See this screenshot:
image

As a user I got confused by the UX:

  • the main checkbox "filter out potential information" is good: when it's checked, we filter things out.
  • the problem is with the other checkboxes: when they're not checked, we filter things out.

I think that all checkboxes should mean the same: when they're checked, we filter things out.
In that case, the label "Include additional data" could be "Filtering options", and all checkbox labels would have the word "Filter" as a prefix: Filter out hidden threads, etc. Of course the checkbox state itself would be reversed too.

@julienw julienw added polish Small features or changes that do not require planning to work on. These help out our end users. privacy Data sanitization, and privacy concerns labels Apr 18, 2019
@asutherland
Copy link

I'm so glad you filed this! I came here because I got confused by this UI. I checked "Filter out potentially identifying information" and checked "Resource URLs" because I thought this was filtering them out. (I think my brain/eyes glossed over "Include additional data" assuming it was an elaboration of the above checkbox, which is visually how things are presented.)

Also, I think it might make sense to elaborate on what each thing listed here is. And in particular, I think it might make sense to separate and/or re-think "Hidden threads" and "Hidden time range". In https://bugzilla.mozilla.org/show_bug.cgi?id=1542884 I'm trying to get a profile of "DOM Worker" threads, but this UI and the choice of the profiler to avoid showing Worker threads by default I think is causing that data to be lost. While I think JS stacks potentially include private data, it could arguably make sense to pose the privacy options as something separate and do the thread/time range stuff as a "What to export"

Like:

Export:
  (  ) the entire profile: all threads and the entire time range
  (  ) the entire time range of just the threads I've displayed (N threads excluded)
  (  ) just the selected time range and for the selected threads (N threads excluded)

For privacy reasons, exclude:
  [  ] Resource URLs:  These can reveal the sites active in the browser and any sites they were connecting to.
  [  ] Screenshots: These are pictures of what Firefox was displaying as you were using it.  These can reveal the active sites and any personal data or data you entered.
  [  ] Extension information: This includes the list of extensions you have installed.

@asutherland
Copy link

Er, elaborating on the "JS stacks" thing, what I mean is that I don't particularly think our C++/rust stacks have much in the way of privacy implications. Our built-in JS logic is probably also fine. But content JS frames do potentially reveal information both in their URLs and in the names of running functions. I didn't propose an option in the previous comment for excluding the JS stacks, but I think it does make sense to support omitting those content frames or very destructively re-mapping them to useless distinct symbols.

@canova
Copy link
Member

canova commented Apr 18, 2019

Btw, these checkboxes were reversed before but it's changed after UX discussion here: firefox-devtools/ux#54 (comment)
cc @gregtatum

@julienw
Copy link
Contributor Author

julienw commented Apr 18, 2019

Thanks for the link @canaltinova! I'll comment there.

@gregtatum
Copy link
Member

I agree it's confusing. I really like @asutherland's suggestion in #1957 (comment).

I tried keeping things really simple, but I think it's getting in the way. The simplest course I think is to reverse the options from "including" to "excluding", then maybe follow-up with @asutherland 's suggestion. I know @digitarald is looking at getting some UX help here too.

@fqueze
Copy link
Contributor

fqueze commented Apr 20, 2019

* the main checkbox "filter out potential information" is good: when it's checked, we filter things out.

* the problem is with the other checkboxes: when they're _not_ checked, we filter things out.

I agree that this UI is confusing due to checkboxes having opposite meanings. Checking more boxes to share more data seems 'natural'. But I'm not sure what the purpose of the "Filter out..." checkbox is; we can't treat privacy as optional.

@violasong
Copy link

I can see how this is confusing. My idea was to not have the first checkbox - just make it the stated default to filter out PII. Then there would only be one type of action in the rest of the checkboxes — check to add (which seems more straightforward than check to remove).

@julienw
Copy link
Contributor Author

julienw commented Apr 23, 2019

I see, this makes sense indeed.

@gregtatum
Copy link
Member

gregtatum commented Apr 24, 2019

I can see how this is confusing. My idea was to not have the first checkbox - just make it the stated default to filter out PII. Then there would only be one type of action in the rest of the checkboxes — check to add (which seems more straightforward than check to remove).

@violasong The problem here is that when implementing this, the Mozilla performance teams want to share everything by default and not lose any data. I'm not sure this was adequately communicated on my end, so sorry for that. This is a pretty strong opinion on their part from my understanding. However, for our "normal" user, we would want to have things sanitized by default. For instance, this "normal" user could be someone from Twitter with a performance problem, that is getting help diagnosing it. Their profile should definitely be sanitized.

To deal with this, we currently have two different behaviors. When profiling Nightly, nothing is sanitized by default. When profiling any other release, the information is sanitized by default. This way when we are doing internal performance testing we most likely won't lose data, while when we ask one of our users to provide a performance profile, they don't unwittingly overshare personal information.

Nightly default:

image

Nightly checked:

image

Release default:

image

Release expanded section:

image

I'm happy to discuss this further, and reach out the other stakeholders.

@asutherland
Copy link

I worry that the check-boxes aren't something most users will understand the full impact of, and that for expert users still amount to a "I know what I'm doing" checkbox that does nothing to eliminate the potential for confusion.

While it would mean making the dialog a full-page affair, it seems tractable to explain what the implications of the check-boxes are. For example, the URL sanitizing logic could be run in a speculative mode to figure out what origin names will be sent and its section could say something like: "URLs containing the following domains will be sent: benign.url, non-incriminating.url, YOU-DIDNT-THINK-THIS-ONE-WAS-IN-THERE-DID-YOU.url" Similarly, the list of extensions could be included if it will be sent. For screenshots, probably the existing display mechanism works.

The specific scenarios I'd be concerned about are:

  • Sites being opened in the background by the page thumbnailer or ServiceWorker mechanisms such as push notifications or BackgroundSync. A user's mental model is likely that only the tabs they have open are active, but that is no longer the case for the web platform.
  • Confusion about what Firefox (disk not performance) profile is in use. Especially if "-no-remote" is not passed when launching Firefox, it's very possible to think you're opening a tab in a different profile/window than you're opening it in.
  • Confusion about how Private Browsing interacts with the profiler, especially if the user has denied the profiler extension access to private browsing windows. (Maybe something special already happens for private browsing?)

And just to reiterate, I think "hidden threads" and "hidden time range" is an entirely different situation from these other checkboxes and I think they should be separate. At least my own personal mental model is that one would use those checkboxes in order to reduce the file size of the performance profile data or to make it less confusing when trying to explain to someone what you think is happening in a profile. (Noting that in general hiding what other threads are up to seems like it will just mislead others since you're hobbling their ability to form alternate hypotheses of what's happening in a performance profile.)

@gregtatum
Copy link
Member

I'm closing this PR, as it's a discussion issue, and I feel like we've come to a decision on the action items. #1978 has landed, which is most of the work, and there are follow-ups filed under the privacy category. Thanks everyone for your feedback, this was helpful. Feel free to re-file any issues where it's still confusing after more of the privacy features have landed, such as #1973, #1904, #1966.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
polish Small features or changes that do not require planning to work on. These help out our end users. privacy Data sanitization, and privacy concerns
Projects
None yet
Development

No branches or pull requests

6 participants