Closed Bug 1095475 Opened 5 years ago Closed 4 years ago

[e10s] Remove Underline from tabs

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
e10s m8+ ---
firefox40 --- fixed

People

(Reporter: jonathan, Assigned: billm)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1003313
tracking-e10s: --- → ?
Blocks: e10s
Component: General → Tabbed Browser
Status: UNCONFIRMED → NEW
Component: Tabbed Browser → Theme
Ever confirmed: true
Attached patch remove underline (obsolete) — Splinter Review
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-
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?
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
Duplicate of this bug: 1096405
Assignee: evilpies → nobody
Assignee: nobody → jmathies
Flags: firefox-backlog+
Today we decided to make the underline controlled by a pref that would be false on aurora and true on nightly.
Assignee: jmathies → wmccloskey
Attached patch patchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/628c47a140bb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Iteration: --- → 40.3 - 11 May
You need to log in before you can comment on or make changes to this bug.