Closed Bug 1248302 Opened 8 years ago Closed 8 years ago

We should not show any decoration for tab with usercontextid=0

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- verified

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

cc.patch
3.46 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
issue.png
59.18 KB, image/png
Details
Attached patch bb.patch (obsolete) — — Splinter Review 
I think there are 2 approaches here: a. we don't set 'usercontextid' when the value is 0, or we change the CSS in order to support this value. I tried with the second approach but I know zero about css. Paolo, do you want to help me?
Attachment #8719342 - Flags: feedback?(paolo.mozmail)
Blocks: ContextualIdentity
Depends on: 1246907
If you want to remove the background image from http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabs.inc.css#400, you could use
> .tabbrowser-tab[usercontextid="0"] {
>   background-image: none;
> }

Or maybe undo all background properties:
> .tabbrowser-tab[usercontextid="0"] {
>   background: none;
> }

Or instead of undoing, modify that selector to exclude usercontextid=0
> .tabbrowser-tab[usercontextid]:not([usercontextid="0"])
>   background-image: linear-gradient(to right, transparent 20%, #909090 30%, #909090 70%, transparent 80%);
>   background-size: auto 2px;
>   background-repeat: no-repeat;
> }

But then it will have more specificity than things like .tabbrowser-tab[usercontextid="1"], so all contexts will be gray. To increase the specificity of these you could use .tabbrowser-tab[usercontextid][usercontextid="1"]
Comment on attachment 8719342 [details] [diff] [review]
bb.patch

No, please!

The assumption all around the place is that the attribute is not set when we have no user context. See for example:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2527
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4005

Setting the attribute to 0 slipped through the cracks in bug 1246907 but shouldn't be done.
Attachment #8719342 - Flags: feedback?(paolo.mozmail) → feedback-
Attached patch cc.patch (obsolete) — — Splinter Review 
Here the second approach.
Attachment #8719342 - Attachment is obsolete: true
Attachment #8719467 - Flags: review?(paolo.mozmail)
Could someone explain what's going on here? My bug report was duped here, but this bug has no image attachments or links for explanation.

Is there documentation on Firefox tab decorations?
(In reply to B.J. Herbison from comment #6)
> Could someone explain what's going on here? My bug report was duped here,
> but this bug has no image attachments or links for explanation.

This is the bug number where the work to fix the problem is happening. Thanks for your original report and the screenshot! Most of the mail you'll get for this bug will be about the technical review, but you may be interested in tracking it for resolution.

> Is there documentation on Firefox tab decorations?

The artifact you've seen is part of a feature we're experimenting with and should not be visible unless you flip an about:config preference. More information about the feature here if you're curious:

https://wiki.mozilla.org/Security/Contextual_Identity_Project/Containers
Comment on attachment 8719467 [details] [diff] [review]
cc.patch

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

::: browser/base/content/tabbrowser.xml
@@ +1726,5 @@
>              b.setAttribute("contextmenu", this.getAttribute("contentcontextmenu"));
>              b.setAttribute("tooltip", this.getAttribute("contenttooltip"));
>  
> +            if (userContextId &&
> +                userContextId != Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID) {

This check shouldn't be needed. We can assume DEFAULT_USER_CONTEXT_ID is falsy.

@@ +6278,5 @@
>          <body>
>          <![CDATA[
> +          if (aUserContextId != Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID) {
> +            this.linkedBrowser.setAttribute("usercontextid", aUserContextId);
> +            this.setAttribute("usercontextid", aUserContextId);

I would assume that setUserContextId would remove the attribute if it handles the case of !aUserContextId.
Attachment #8719467 - Flags: review?(paolo.mozmail)
Attached patch cc.patch — — Splinter Review 
userContextId is always a unsigned long, but event.target.getAttribute() returns a string. parseInt() fixes this issue without additional checks.
Attachment #8719467 - Attachment is obsolete: true
Attachment #8719514 - Flags: review?(paolo.mozmail)
Attachment #8719514 - Flags: review?(paolo.mozmail) → review+
https://hg.mozilla.org/mozilla-central/rev/3c0f35eeadc0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Reproduced the original issue using the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-15-03-02-13-mozilla-central/

Opening a link via "Open Link in New Tab" would result in a decoration for a tab with a usercontextid=0 which is incorrect. Tabs using usercontextid=0 shouldn't be decorated.

Went through verification using the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-16-03-02-45-mozilla-central/

Test Cases Used:

* Opening a new tab via Right Click -> Open Link in New Tab
* Opening a new window via Right Click -> Open Link in New Window
* Opening a new window via Right Click -> Open Link in New Private Window
* Opening a new tab via "+" next to the tabs
* Opening a new tab via File -> New Tab
* Opening a new window via File -> New Window
* Opening a new window via File -> New Private Window
* Opening a new window via File -> Non-e10s Window
* Opening a new tab via "CTRL + T"
* Opening a new window via "CTRL + T"
* Opening a link from Gmail that opens a new tab
* ensured that the "Personal, Work, Banking & Shopping" containers are using the correct tab/awesome bar decoration
** via File -> New Container Tab
** via Right Click -> Open Link in New Container Tab
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
(In reply to Kamil Jozwiak [:kjozwiak] from comment #14)
> Opening a link via "Open Link in New Tab" would result in a decoration for a
> tab with a usercontextid=0 which is incorrect. Tabs using usercontextid=0
> shouldn't be decorated.
> 
What was it decorated with?
> What was it decorated with?

having the attribute 'usercontextid' always set (also with value 0), our CSS was showing the generic 'unknown' usercontextid tab with a small gray topbar.
Attached image issue.png — 
(In reply to Tanvi Vyas [:tanvi] from comment #15)
> (In reply to Kamil Jozwiak [:kjozwiak] from comment #14)
> > Opening a link via "Open Link in New Tab" would result in a decoration for a
> > tab with a usercontextid=0 which is incorrect. Tabs using usercontextid=0
> > shouldn't be decorated.
> > 
> What was it decorated with?

Users were seeing a "grey" container indicator above usercontextid=0 tabs that were being opened using "Open Link in New Tab".
Keywords: regression
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: