[e10s] Remove Underline from tabs

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Theme
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Jonathan Howard, Assigned: billm)

Tracking

Trunk
Firefox 40
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(e10sm8+, firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Maybe make it a optional preference.

The underline screams something is broken to testers. (Also is ugly.) I think it should be off now that e10s is the nightly default.

Don't let the cold in.
(Reporter)

Updated

3 years ago
Blocks: 1003313
tracking-e10s: --- → ?

Updated

3 years ago
Blocks: 516752
Component: General → Tabbed Browser

Updated

3 years ago
Status: UNCONFIRMED → NEW
Component: Tabbed Browser → Theme
Ever confirmed: true
Created attachment 8519405 [details] [diff] [review]
remove underline

Removing the underline is easy enough, but maybe we should keep some way to tell if something is a remote tab.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8519405 - Flags: review?(wmccloskey)
Comment on attachment 8519405 [details] [diff] [review]
remove underline

AFAIK this makes the 'remote' attribute unused, so we should remove that too.
Attachment #8519405 - Flags: review?(wmccloskey) → review-
Not really: http://mxr.mozilla.org/mozilla-central/search?string=getAttribute%28%22remote&find=&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=mozilla-central
But as far as I remember the attribute was added for this underline. Is there no other blessed way to tell if a browser / tab is remote?
(Assignee)

Comment 5

3 years ago
I think Dão is referring to the fact that we have a "remote" attribute on both the <browser> and the <tab>. The one on <browser> is really important and used by platform code. The one on <tab> was added when we added the underlining:
  http://hg.mozilla.org/mozilla-central/rev/45cd37a5b834
Based on Tom's search in comment 3, it appears that we only use the attribute on the <tab> in the browser_e10s_switchbrowser.js test. If we just change selectedTab to selectedBrowser in all those uses, I think we could remove the attribute on <tab> (although I haven't looked at each one in detail).

I'm also not sure we want to make this change. I don't think that we need to have an indicator for each tab. But it is nice to know if a window is e10s or not. Is there an easy way to do that? Maybe something like what we do for private windows? Maybe we could put the lightning bolt icon in the corner where the private browsing icon goes. That would look pretty cool.
Duplicate of this bug: 1095746

Updated

3 years ago
Duplicate of this bug: 1096405
tracking-e10s: ? → m8+
Assignee: evilpies → nobody

Updated

3 years ago
Assignee: nobody → jmathies
Flags: firefox-backlog+
(Assignee)

Comment 8

2 years ago
Today we decided to make the underline controlled by a pref that would be false on aurora and true on nightly.
Assignee: jmathies → wmccloskey
(Assignee)

Comment 9

2 years ago
Created attachment 8598983 [details] [diff] [review]
patch

Here's an updated patch that removes the underlines and uses a tooltip to distinguish e10s tabs.

I looked into adding a visual indicator similar to the private browsing icon, but it would be a lot of work. We do the private browsing indicator differently on each platform, and even within each platform. So I think this is a nice compromise that won't be too difficult.

It's possible we'll want to go back to the underlines on nightly, but I'd like to try this out for a bit and see if people are okay with it. I suspect that, in a few weeks, it will seem crazy that we kept the underlines for so long.
Attachment #8519405 - Attachment is obsolete: true
Attachment #8598983 - Flags: review?(dao)
Comment on attachment 8598983 [details] [diff] [review]
patch

>-          event.target.setAttribute("label", tab.mOverCloseButton ?
>-                                             tab.getAttribute("closetabtext") :
>-                                             tab.getAttribute("label"));
>+          let label;
>+          if (tab.mOverCloseButton) {
>+            label = tab.getAttribute("closetabtext");
>+          } else {
>+            label = tab.getAttribute("label");
>+            if (this.AppConstants.E10S_TESTING_ONLY && tab.linkedBrowser.isRemoteBrowser) {
>+              label += " - e10s";
>+            }
>+          }
>+          event.target.setAttribute("label", label);

Since the e10s suffix is temporary and we'll want to remove that code again, I'd prefer if you could avoid refactoring this in a way that won't make much sense later on. Instead you can just add it like this:

>          event.target.setAttribute("label", tab.mOverCloseButton ?
>                                             tab.getAttribute("closetabtext") :
>                                             tab.getAttribute("label") +
>                                               (this.AppConstants.E10S_TESTING_ONLY && tab.linkedBrowser.isRemoteBrowser ? " - e10s" : ""));
Attachment #8598983 - Flags: review?(dao) → review+

Comment 11

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/628c47a140bb
https://hg.mozilla.org/mozilla-central/rev/628c47a140bb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40

Updated

2 years ago
Iteration: --- → 40.3 - 11 May
Depends on: 1160551
You need to log in before you can comment on or make changes to this bug.