If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add tab decoration signifying userContextId

RESOLVED FIXED in Firefox 44

Status

()

Firefox
Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: englehardt, Assigned: englehardt)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8643836 [details]
Containers-comparison.png

The userContextId attribute is an OriginAttribute (Bug 1179985) that represents the Container (Bug 1191418) the current tab belongs to. We want to signify which Container a tab belongs to by displaying a corresponding color bar at the top of the tab. Mock-ups of the effect are attached. Color values for the default Containers given in Bug 1181953.
(Assignee)

Updated

2 years ago
Assignee: nobody → senglehardt
Blocks: 1191418
Status: NEW → ASSIGNED
Depends on: 1191442
(Assignee)

Updated

2 years ago
Blocks: 1191494
(Assignee)

Updated

2 years ago
Depends on: 1191460
No longer depends on: 1191442

Comment 1

2 years ago
#c25
Thank you for your reply and link Gavin.

I have read https://support.mozilla.org/en-US/kb/how-stop-firefox-making-automatic-connections and I agree that if a user follows all these steps, they can stop firefox from pre fetching or pre connecting to malicious servers.

Unfortunately most users will not even think to look for this page because firefox gives no responsive UI indications that it is currently pre fetching a link. Dns or tcp connections.

You could fix this problem by making pre fetching on hover explicit: Every time firefox autonomously connects to something, the user should receive a visual indication of this.

This way, the user will be informed that these features are active. And interested users will know to look for the page you have just linked.

This could be anything from triggering the "page loading" animation in the address bar, to simply adding the word "PRELOADING" next to the url in the "status bar" / "status popup" at the bottom of the page when the user hovers a link.

Or you could add a single checkbox in the Preferences in the privacy section that turns all pre-fetching on or off. This way users who are curious about privacy settings will be informed of this behavior and have the choice to protect themselves at the cost of one round trip ping time in performance gains per connection.

Please don't ignore these issues Gavin. The trade-off between performance and privacy must AT LEAST be made EXPLICIT to the users. Then they can make their own choices.

Adding Stacy  from privacy team (That correct?).
It's really hard to figure out who's on what team on bugzilla. Who do we contact for the user story / UI team?
Flags: needinfo?(smartin)
(Assignee)

Comment 2

2 years ago
(In reply to dfgdvevfrgeadfv from comment #1)
> #c25
> Thank you for your reply and link Gavin.
> 
> [snip]

I believe you posted this comment to the incorrect bug.

Comment 3

2 years ago
Sorry.
Too many tabs open.
Too little coffee.
(Assignee)

Updated

2 years ago
Depends on: 1191442
(Assignee)

Comment 4

2 years ago
Created attachment 8650247 [details]
1191451-v1.png

v1 demo
(Assignee)

Comment 5

2 years ago
Created attachment 8650251 [details] [diff] [review]
1191451-tab-decoration.patch

WIP patch. Applied on top of patches from Bug 1191442. See screenshot "1191451-v1.png" for a visual.
Attachment #8650251 - Flags: feedback?(tanvi)
(Assignee)

Comment 6

2 years ago
Created attachment 8650253 [details] [diff] [review]
1191451-tab-decoration.patch
Attachment #8650251 - Attachment is obsolete: true
Attachment #8650251 - Flags: feedback?(tanvi)
Attachment #8650253 - Flags: feedback?(tanvi)

Comment 7

2 years ago
Comment on attachment 8650253 [details] [diff] [review]
1191451-tab-decoration.patch

As discussed in person:

+      <xul:stack class="tab-stack" xbl:inherits="usercontextid" flex="1
We might be able to skip this and use tabbrowser-tab for the decoration.

+.tab-stack[usercontextid="1"] {
+  background: linear-gradient(to right, transparent 20%, #00a7e0 30%, #00a7e0 70%, transparent 80%);
+  background-size: auto 2px;
+  background-repeat: no-repeat;
+}
Pull the last two attributes out and just set them once on [usercontextid].
Attachment #8650253 - Flags: feedback?(tanvi) → feedback+
(Assignee)

Comment 8

2 years ago
Created attachment 8650672 [details] [diff] [review]
1191451-tab-decoration.patch

In response to Comment 7: I tried breaking up the styles in this way, but the attributes of [usercontext="1"] seem to overwrite [usercontext] so I then lose the [usercontext] styling. Leaving as is for now.
Attachment #8650253 - Attachment is obsolete: true
Attachment #8650672 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

2 years ago
Flags: needinfo?(smartin)

Comment 9

2 years ago
Comment on attachment 8650672 [details] [diff] [review]
1191451-tab-decoration.patch

Review of attachment 8650672 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/tabs.inc.css
@@ +404,5 @@
> +.tabbrowser-tab[usercontextid="1"] {
> +  background: linear-gradient(to right, transparent 20%, #00a7e0 30%, #00a7e0 70%, transparent 80%);
> +  background-size: auto 2px;
> +  background-repeat: no-repeat;
> +}

(In reply to Steven Englehardt [:senglehardt] from comment #8)
> In response to Comment 7: I tried breaking up the styles in this way, but
> the attributes of [usercontext="1"] seem to overwrite [usercontext] so I
> then lose the [usercontext] styling. Leaving as is for now.

You have to use background-image, you're currently using the "background" shorthand that indeed would reset any background-size and background-repeat you set previously.

Please add comments to each usercontextid rule to indicate whether the ID is for work, personal, and so on.

Also, I suggest to choose one of the four colors we have and include it in the base rule instead, as the default to use in case we set an unknown ID.
Attachment #8650672 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 10

2 years ago
Created attachment 8655756 [details] [diff] [review]
1191451-tab-decoration.patch

(In reply to :Paolo Amadini from comment #9)
> Also, I suggest to choose one of the four colors we have and include it in
> the base rule instead, as the default to use in case we set an unknown ID.

We want the default case to have no decoration, as the default user context (userContextId = 0), corresponds to the user's storage that isn't within a container. For example, any browsing a user does prior to using the feature will have it's storage untouched in the default user context.

We should never set an unknown context.
Attachment #8650672 - Attachment is obsolete: true
Attachment #8655756 - Flags: review?(paolo.mozmail)

Comment 11

2 years ago
(In reply to Steven Englehardt [:senglehardt] from comment #10)
> We want the default case to have no decoration
> 
> We should never set an unknown context.

There's a difference between no usercontextid, and a usercontextid greater than 4. It's true that a value greater than 4 is unexpected, so we might not worry too much about that.

However, this may still be set experimentally by an add-on or by a developer when testing, and is supported by the platform. I'm suggesting we indicate that case in the UI, to distinguish it from the default user context (and don't incorrectly apply only half of the CSS rule in such cases).

Comment 12

2 years ago
Comment on attachment 8655756 [details] [diff] [review]
1191451-tab-decoration.patch

Review of attachment 8655756 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +1789,5 @@
>              var t = document.createElementNS(NS_XUL, "tab");
>  
>              var uriIsAboutBlank = !aURI || aURI == "about:blank";
>  
> +            t.setAttribute("usercontextid", aUserContextId);

This is actually setting 'usercontextid="undefined"' on normal tabs.

::: browser/themes/shared/tabs.inc.css
@@ +403,5 @@
> +/* User Context UI - change tab decoration depending on userContextId */
> +.tabbrowser-tab[usercontextid] {
> +  background-size: auto 2px;
> +  background-repeat: no-repeat;
> +}

Using a fifth color may be better, just add the following property declaration to this rule:

  background-image: linear-gradient(to right, transparent 20%, #909090 30%, #909090 70%, transparent 80%);

This will also help catching bugs like the one above, if someone changes the code handling the usercontextid attribute.
Attachment #8655756 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 13

2 years ago
Created attachment 8657286 [details] [diff] [review]
1191451-tab-decoration.patch

See https://bug1191455.bmoattachments.org/attachment.cgi?id=8657285 for a visual of the gray decoration for unknown contexts.
Attachment #8655756 - Attachment is obsolete: true
Attachment #8657286 - Flags: review?(paolo.mozmail)

Comment 14

2 years ago
Comment on attachment 8657286 [details] [diff] [review]
1191451-tab-decoration.patch

Review of attachment 8657286 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

I haven't re-tested the latest patch queue, it might be worth doing a final round of manual sanity checks and see if all interactions including tab detaching work as expected.

Do you have tryserver access to start a build? If not I can do that as soon as you upload the latest versions of the other patch.
Attachment #8657286 - Flags: review?(paolo.mozmail) → review+

Comment 15

2 years ago
As well as checking that nothing breaks with the about:config option disabled.
(Assignee)

Comment 16

2 years ago
(In reply to :Paolo Amadini from comment #14)
> Comment on attachment 8657286 [details] [diff] [review]
> 1191451-tab-decoration.patch
> 
> Review of attachment 8657286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> I haven't re-tested the latest patch queue, it might be worth doing a final
> round of manual sanity checks and see if all interactions including tab
> detaching work as expected.
> 
> Do you have tryserver access to start a build? If not I can do that as soon
> as you upload the latest versions of the other patch.

(In reply to :Paolo Amadini from comment #15)
> As well as checking that nothing breaks with the about:config option
> disabled.

Sounds good. I did a bunch of manual testing of all the interactions I could think of. Everything works as expected. Pushed to try to make sure we don't break anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ba18a86e004
(Assignee)

Comment 17

2 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69a039ba0e8f
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 18

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/5868176e93a6
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5868176e93a6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.