Closed
Bug 1095475
Opened 10 years ago
Closed 9 years ago
[e10s] Remove Underline from tabs
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
People
(Reporter: jonathan, Assigned: billm)
References
Details
Attachments
(1 file, 1 obsolete file)
3.93 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Blocks: 1003313
tracking-e10s:
--- → ?
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Component: Tabbed Browser → Theme
Ever confirmed: true
Comment 1•10 years ago
|
||
Removing the underline is easy enough, but maybe we should keep some way to tell if something is a remote tab.
Comment 2•10 years ago
|
||
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-
Comment 3•10 years ago
|
||
Not really: http://mxr.mozilla.org/mozilla-central/search?string=getAttribute%28%22remote&find=&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=mozilla-central
Comment 4•10 years ago
|
||
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•10 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.
Updated•10 years ago
|
Updated•10 years ago
|
Assignee: evilpies → nobody
Updated•10 years ago
|
Assignee: nobody → jmathies
Updated•9 years ago
|
Flags: firefox-backlog+
Assignee | ||
Comment 8•9 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•9 years ago
|
||
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 10•9 years ago
|
||
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 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/628c47a140bb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Updated•9 years ago
|
Iteration: --- → 40.3 - 11 May
You need to log in
before you can comment on or make changes to this bug.
Description
•