Closed Bug 1325902 Opened 7 years ago Closed 7 years ago

Increase visibility of container highlight on tabs

Categories

(Firefox :: Security, defect, P1)

Product:
Firefox
Component:
Security
Version:
51 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox55 --- wontfix
firefox56 --- verified
firefox57 --- unaffected

People

(Reporter: jkt, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][domsecurity-active])

Attachments

(1 file, 1 obsolete file)

Bug 1325902 - Moving container tab highlight from test pilot experiment
59 bytes, text/x-review-board-request
dao
: review+
lizzard
: approval-mozilla-beta+
Details 
We need to look at the simplest to implement underline possibility in CSS which can be uplifted. I'm going to wrap the work in a pref which the test pilot experiment could potentially check which is the better performing in the browser.

From Tanvi:

Taipei team came out with a few proposals on how we could change the tab highlighting here - https://www.dropbox.com/s/8ywv6f2ty6s74en/Container%20Tab%20proposal.png?dl=0.

We know overline is a problem, so that throws out 1-3.  And 6 just looks odd to me.  So I'm in favor of 4 or 5 - pretty much changing our overline in mozilla-central to an underline, as John proposed. If we can get an underline that is the full tab width (proposal 5), then that would be great.  But if not, we can underline a portion of the tab (proposal 4).

Can you add these details to that bug?  If its simple to implement and not particularly risky, we can try to uplift to 51 (beta).
This is a WIP, there hasn't been any testing for the square tabs theme which I know there will be issues. There also appears to be a issue in treeherder too.
Fixed the regression mentioned earlier. Also fixed the devedition style, however a decision is needed on which style is needed underline vs blue colour change (discussion thread in email).

Baku the code is ready for review, the change for the double stripe is:

.tabbrowser-tab[usercontextid][data-tab-experiment="under"] {
+  background-image: linear-gradient(to right, var(--identity-tab-color) 0%, var(--identity-tab-color) 100%), linear-gradient(to right, var(--chrome-background-color) 0%, var(--chrome-background-color) 100%);
+  background-size: auto 2px, auto 1px;
+  background-repeat: no-repeat;
+  background-position: 0 29px, 0 28px;
-  background-image: linear-gradient(to right, var(--identity-tab-color) 0%, var(--identity-tab-color) 100%);
-  background-size: var(--identity-stroke-background-size);
-  background-repeat: no-repeat;
-  background-position: 0 29px;
Flags: needinfo?(amarchesini)
Priority: -- → P2
Whiteboard: [userContextId]
Comment on attachment 8822299 [details]
Bug 1325902 - add container tab experiment styling code for increasing visibility of tabs

https://reviewboard.mozilla.org/r/101246/#review102512

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:99
(Diff revision 2)
>      AsyncShutdown.profileBeforeChange.addBlocker("ContextualIdentityService: writing data",
>                                                   () => this._saver.finalize());
>  
>      this.load();
> +    this.observe(null, "nsPref:changed", "privacy.userContext.ui.tabStyleExperiment");
> +    Services.prefs.addObserver("privacy.userContext.ui.tabStyleExperiment", this, false);

why do we need to have this observer if you check the value of tabStyleExperiment in restyleTabs?

Remove it.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:132
(Diff revision 2)
>      }, (error) => {
>        this.loadError(error);
>      });
>    },
>  
> +  observe() {

this is not needed.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:287
(Diff revision 2)
>      return gBrowserBundle.GetStringFromName(identity.l10nID);
>    },
>  
> +  restyleTabs(tabs, userContextId = false) {
> +    // this is triggered when the pref changes, the restyle happens before we observe the change
> +    this.tabExperiment = Services.prefs.getCharPref("privacy.userContext.ui.tabStyleExperiment");

you do this all the time, so yeah, this is enough. And use a local variable.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:292
(Diff revision 2)
> +    this.tabExperiment = Services.prefs.getCharPref("privacy.userContext.ui.tabStyleExperiment");
> +
> +    for (let tab of tabs) {
> +      if (tab.hasAttribute("usercontextid") &&
> +          (userContextId == false || tab.getAttribute("usercontextid") == userContextId)) {
> +        this.setTabStyle(tab);

maybe you can pass tabExperiment as argument.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:297
(Diff revision 2)
> +        this.setTabStyle(tab);
> +      }
> +    }
> +  },
> +
>    setTabStyle(tab) {

do we use this method elsewhere? If not, just make it: setStyle(tab, useExperimentalStyle)
Attachment #8822299 - Flags: review?(amarchesini) → review-
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
Priority: P2 → P1
Version: 52 Branch → 51 Branch
Flags: needinfo?(amarchesini)
Whiteboard: [userContextId] → [userContextId][domsecurity-active]
The current patch has an issue with Mac, it also might be simpler to put this in just Test Pilot instead.
Mass wontfix for bugs affecting firefox 52.
status-firefox52: affected → wontfix
Component: DOM: Security → Security
Product: Core → Firefox
Attachment #8822299 - Attachment is obsolete: true
Attachment #8865916 - Flags: review?(amarchesini) → review?(gijskruitbosch+bugs)
Attachment #8865916 - Flags: review?(gijskruitbosch+bugs) → review?(dao+bmo)
Comment on attachment 8865916 [details]
Bug 1325902 - Moving container tab highlight from test pilot experiment

https://reviewboard.mozilla.org/r/137496/#review141060

Dão is a better reviewer here as he and the photon visual team are working on updating tab styles anyway. But I already spotted something just glancing at this, so r- for now. Please get further reviews from Dão.

::: browser/components/contextualidentity/content/usercontext.css:75
(Diff revision 1)
>  
>  #userContext-icons {
>    -moz-box-align: center;
>  }
>  
> -.tabbrowser-tab[usercontextid] {
> +/* special styles run through a psuedo-class off of

pseudo


Also, can we wrap this at something closer to 80ch, and use Capital letters to start sentences and .s to end them?

::: browser/components/contextualidentity/content/usercontext.css:80
(Diff revision 1)
> -.tabbrowser-tab[usercontextid] {
> -  background-image: linear-gradient(to right, transparent 20%, var(--identity-tab-color) 30%, var(--identity-tab-color) 70%, transparent 80%);
> -  background-size: auto 2px;
> -  background-repeat: no-repeat;
> +/* special styles run through a psuedo-class off of
> +these elements so they need to be relatively positioned.
> +these styles address both regular and compact themes,
> +special cases are addressed below */
> +.tabbrowser-tab[usercontextid] .tab-background-middle,
> +#main-window[style*="compact"] .tabbrowser-tab[usercontextid] .tab-content,

r- for `style*=compact`. Can we not detect this some other way?
Attachment #8865916 - Flags: review-
Sorry :Gijs, this was pasted from the test pilot code without checking it over. Thanks!
Comment on attachment 8865916 [details]
Bug 1325902 - Moving container tab highlight from test pilot experiment

https://reviewboard.mozilla.org/r/137496/#review142308

::: browser/components/contextualidentity/content/usercontext.css:80
(Diff revision 2)
> -.tabbrowser-tab[usercontextid] {
> -  background-image: linear-gradient(to right, transparent 20%, var(--identity-tab-color) 30%, var(--identity-tab-color) 70%, transparent 80%);
> -  background-size: auto 2px;
> -  background-repeat: no-repeat;
> +/* Special styles run through a pseudo-class off of these elements so they need
> +   to be relatively positioned.
> +   These styles address both regular and compact themes, special cases are
> +   addressed below. */
> +.tabbrowser-tab[usercontextid] .tab-background-middle {
> +  position: relative;

I'm not sure this is the right approach. There might be cheaper ways to do this. I'll need to think more about this.

In any case, please use the child selector.

::: browser/components/contextualidentity/content/usercontext.css:83
(Diff revision 2)
> -  background-repeat: no-repeat;
> +   addressed below. */
> +.tabbrowser-tab[usercontextid] .tab-background-middle {
> +  position: relative;
> +}
> +
> +.tabbrowser-tab[usercontextid] .tab-background-middle::after,

please use the child selector

::: browser/components/contextualidentity/content/usercontext.css:84
(Diff revision 2)
> +.tabbrowser-tab[usercontextid] .tab-background-middle {
> +  position: relative;
> +}
> +
> +.tabbrowser-tab[usercontextid] .tab-background-middle::after,
> +:-moz-lwtheme .tabbrowser-tab[usercontextid] .tab-content::after {

please use the child selector and move the :-moz-lwtheme pseudo-class to a resonable place in this selector

::: browser/components/contextualidentity/content/usercontext.css:95
(Diff revision 2)
> +  position: absolute;
> +  right: 0;
> +  width: 100%;
> +}
> +
> +.tabbrowser-tab[usercontextid] .tab-background-middle::after {

please use the child selector

::: browser/components/contextualidentity/content/usercontext.css:102
(Diff revision 2)
> +  border-radius: 2px 2px 0 0;
> +  bottom: 1px;
> +  height: 3px;
> +}
> +
> +.tabbrowser-tab[usercontextid]:not([visuallyselected="true"]) .tab-background-middle::after {

please use the child selector

::: browser/components/contextualidentity/content/usercontext.css:104
(Diff revision 2)
> +  height: 3px;
> +}
> +
> +.tabbrowser-tab[usercontextid]:not([visuallyselected="true"]) .tab-background-middle::after {
> +  bottom: 2px;
> +  height: 2px !important;

why !important?

::: browser/components/contextualidentity/content/usercontext.css:107
(Diff revision 2)
> +.tabbrowser-tab[usercontextid]:not([visuallyselected="true"]) .tab-background-middle::after {
> +  bottom: 2px;
> +  height: 2px !important;
> +}
> +
> +.tabbrowser-tab[usercontextid][pinned="true"] .tab-background-middle::after {

please use the child selector

::: browser/components/contextualidentity/content/usercontext.css:109
(Diff revision 2)
> +  height: 2px !important;
> +}
> +
> +.tabbrowser-tab[usercontextid][pinned="true"] .tab-background-middle::after {
> +  left: -150%;
> +  width: 400%;

please document how you got to these numbers
Attachment #8865916 - Flags: review?(dao+bmo) → review-
Hey Dao, Sorry for the delay on these comments.

I still wasn't able to find a nicer solution to the position relative (I copied the original styles verbatim to what was used in test pilot which is why the issues).

The containers team would like to land this before your square tabs patch for 57 so we can minimise breakage etc.

Thanks
Jonathan
Flags: needinfo?(dao+bmo)
Hey Dao,

Are you able to take another look at the updated patch?

The position relative issue you mentioned is still there, I'm not sure a better fix for this right now: https://reviewboard.mozilla.org/r/137496/diff/4/

Thanks
Jonathan
I think this will have to wait until after bug 1349555. In fact that should make it simpler to implement something sane here.
Flags: needinfo?(dao+bmo)
My issue is we would like to land this as soon as possible, we can always do a post Bug 1349555 update to simplify. I didn't want to have to wait post 57 to ship this if possible as it means our addon will still need to remain using legacy code and be rewritten for 57.

Is there anyway we get this in for 56?
Flags: needinfo?(dao+bmo)
(In reply to Jonathan Kingston [:jkt] from comment #22)
> My issue is we would like to land this as soon as possible, we can always do
> a post Bug 1349555 update to simplify. I didn't want to have to wait post 57
> to ship this if possible as it means our addon will still need to remain
> using legacy code and be rewritten for 57.
> 
> Is there anyway we get this in for 56?

usercontext.css still has the legacy .tabbrowser-tab[usercontextid] styling. Is this not sufficient as an interim solution for 56?
Flags: needinfo?(dao+bmo) → needinfo?(jkt)
> usercontext.css still has the legacy .tabbrowser-tab[usercontextid] styling. Is this not sufficient as an interim solution for 56?

So we plan to release in 54/55 or even sooner, the SDK code we have will likely need to be ripped out for Nightly and I would rather not maintain yet another version of our codebase for different trains.

This leaves the experience of new users to "release containers" like this:
- Test pilot launched with overline
- We migrated to underline
- We release to AMO and make a big noise
- Users start getting overline again
- In 57 they get underline again, with the confusion of the tab highlight in Photon

I'm hoping to make it more like:
- Test pilot launched with overline
- We migrated to underline
- We release to AMO and make a big noise
- Users notice the tab highlight which isn't for containers in 57

I can probably start migrating the code to bootstrap instead so that it works for 56/57 but I'm not sure on timescales, this is why I wanted to ship this before the tab squaring.

Is there a risk for getting this current version into 56, with the idea I can then make 57 use the hbox as discussed?
Flags: needinfo?(jkt) → needinfo?(dao+bmo)
(In reply to Jonathan Kingston [:jkt] from comment #24)
> > usercontext.css still has the legacy .tabbrowser-tab[usercontextid] styling. Is this not sufficient as an interim solution for 56?
> 
> So we plan to release in 54/55 or even sooner, the SDK code we have will
> likely need to be ripped out for Nightly and I would rather not maintain yet
> another version of our codebase for different trains.

I'm not sure I understand. I would r+ this code only if we replace it again in 57. Seems like you'll have to maintain different versions anyway, except that if we do NOT land this patch now, maintaining the legacy implementation would be simpler since the implementation is much simpler.
Flags: needinfo?(dao+bmo)
Basically because of timescales I would rather rip out SDK code and not replace with bootstrap code. Maintaining some CSS in central for 56 seems easier than rewriting to bootstrap whilst we still cater for 55/56/57 releases for our AMO addon/central populations.

As we are enabling extensions to write their own container implementations in AMO in Bug 1354602, there will be many more on-ramps to getting containers too that we can't fix the underline for too.

I can follow up with a 57 related fix as Bug 1349555 breaks all our current implementations anyway so we need that to land at the same time really.

Is there other central 56 issues you see with this code?
Which releases can we currently uplift this for?

Thanks again
Flags: needinfo?(dao+bmo)
(In reply to Jonathan Kingston [:jkt] from comment #26)
> Basically because of timescales I would rather rip out SDK code and not
> replace with bootstrap code. Maintaining some CSS in central for 56 seems
> easier than rewriting to bootstrap whilst we still cater for 55/56/57
> releases for our AMO addon/central populations.

I don't understand what "Maintaining some CSS in central for 56" really means. I don't want this code in central for 57+, and central is 57 as of today.
Flags: needinfo?(dao+bmo)
> I don't understand what "Maintaining some CSS in central for 56" really means.

I was replying to:

> maintaining the legacy implementation would be simpler since the implementation is much simpler.

I mean, ensuring this patch looks ok for the lifetime of 56. This is a very simple patch that is self contained only to impacting users with containers.

Can we just uplift this instead then? I don't need this for 57 either as soon as your patch lands.

The cost is rewriting SDK code into bootstrap that caters for all our trains and will be ripped out as soon as 57 becomes stable.
Flags: needinfo?(dao+bmo)
(In reply to Jonathan Kingston [:jkt] from comment #28)
> Can we just uplift this instead then? I don't need this for 57 either as
> soon as your patch lands.

Okay.
Flags: needinfo?(dao+bmo)
Comment on attachment 8865916 [details]
Bug 1325902 - Moving container tab highlight from test pilot experiment

https://reviewboard.mozilla.org/r/137496/#review168682

rs=me for uplift
Attachment #8865916 - Flags: review?(dao+bmo) → review+
Comment on attachment 8865916 [details]
Bug 1325902 - Moving container tab highlight from test pilot experiment

Approval Request Comment
[Feature/Bug causing the regression]: Tab highlight for containers
[User impact if declined]: Users will be confused by the 57 highlight as will have seen 4 variants of tab highlighting
[Is this code covered by automated tests?]: N/A
[Has the fix been verified in Nightly?]: No, it's not for 57 https://bugzilla.mozilla.org/show_bug.cgi?id=1325902#c28
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's isolated to containers users only. worst case scenario some of the users could be patched with our addon however that is what we are trying to avoid.
[String changes made/needed]: N/A
Attachment #8865916 - Flags: approval-mozilla-beta?
See Also: → 1387117
Comment on attachment 8865916 [details]
Bug 1325902 - Moving container tab highlight from test pilot experiment

This should land on beta only so that 56 users will have a consistent experience. 

Jonathan or Dão, can you describe in a comment how QE can test this for the 56 beta 1 build?
Flags: needinfo?(jkt)
Flags: needinfo?(dao+bmo)
Flags: needinfo?(andrei.vaida)
Attachment #8865916 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
So the way to check is that the tab highlight of a container tab should move from an overline to an underline.

So without test pilot containers installed ensure "privacy.userContext.enabled" is enabled. Open a container tab.
If the patch has made the tab have an underline, then it has worked. If it has a line at the top it hasn't worked.

Thanks :lizzard!
Flags: needinfo?(jkt)
Flags: needinfo?(dao+bmo)
https://hg.mozilla.org/releases/mozilla-beta/rev/08dfb6963e72
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox51: affected → ---
status-firefox52: wontfix → ---
status-firefox53: affected → ---
status-firefox55: --- → wontfix
status-firefox56: --- → fixed
status-firefox57: --- → unaffected
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
I have verified this issue on Windows 10 x64 and Mac OS 10.12 with the latest Nightly(57.0a1-20170904100131) and the latest Beta (56.0b8-20170831165232), when opening a container tab you can observe that it has an underline highlight.

[Notes]:
- In order to test this on Beta, I had to modify the following prefs:
privacy.userContext.enabled;true
privacy.userContext.longPressBehavior;2
privacy.userContext.ui.enabled;true
privacy.usercontext.about_newtab_segregation.enabled;true
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: needinfo?(andrei.vaida)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: