Closed Bug 1349555 Opened 7 years ago Closed 7 years ago

Make tabs rectangular (remove tab curve)

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: dao, Assigned: dao)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

(Whiteboard: [photon-visual][p1][57])

Attachments

(2 files, 6 obsolete files)

Bug 1349555 - Implement most of the photon tab strip.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1349555 - Fix windowopen reflows for OS X and Windows.
59 bytes, text/x-review-board-request
dao
: review+
Details 
Needinfo Stephen for UX spec. Are there any changes we should do here besides just removing the curve? E.g. in the Photon overview presentation, the selected tab has a colored line at the top.
Flags: needinfo?(shorlander)
status-firefox55: affected → ---
Blocks: photon-ui-refresh
Whiteboard: [photon]
No longer blocks: photon-ui-refresh
Priority: -- → P1
What's actually the reason for removing the curve? To adjust the layout more to the one of the OS?

Sebastian
Flags: needinfo?(dao+bmo)
(In reply to Sebastian Zartner [:sebo] from comment #1)
> What's actually the reason for removing the curve? To adjust the layout more
> to the one of the OS?

Not sure, but I think fitting in better with OSes, especially Windows 10, is part of the story. In his presentation Stephen also referred to the curvy tabs as "heavily stylistic elements," which I guess means that the unique style / usability trade-off isn't quite right.
Flags: needinfo?(dao+bmo)
Blocks: 1354332
Depends on: 1355390
Flags: needinfo?(shorlander)
Whiteboard: [photon] → [photon][57]
Whiteboard: [photon][57] → [photon-visual][57]
Blocks: photon-tabs
No longer blocks: photon-visual
Flags: qe-verify+
Priority: P1 → P2
QA Contact: ovidiu.boca
Whiteboard: [photon-visual][57] → [photon-visual][p1][57]
QA Contact: ovidiu.boca → brindusa.tot
Blocks: 1363023
(In reply to Dão Gottwald [::dao] from comment #2)
> (In reply to Sebastian Zartner [:sebo] from comment #1)
> > What's actually the reason for removing the curve? To adjust the layout more
> > to the one of the OS?
> 
> Not sure, but I think fitting in better with OSes, especially Windows 10, is
> part of the story. In his presentation Stephen also referred to the curvy
> tabs as "heavily stylistic elements," which I guess means that the unique
> style / usability trade-off isn't quite right.

Regardless of OS, it's time for a change. Look, it's been the same since 2013 (at least, far as I'm aware, first time I saw those curves was 2013), so to make it more simplistic would be better, at least from a "modern" perspective. The world has changed a bit since. I don't use Windows, I use Fedora, but still...
With the drag area on the tab bar gone, the curvey tabs look really crammed. I'd really appreciate to have this bug get tackled soon, to get a more realistic outlook into the future of the tab strip.
(In reply to Axel Hecht [:Pike] from comment #4)
> With the drag area on the tab bar gone, the curvey tabs look really crammed.
> I'd really appreciate to have this bug get tackled soon, to get a more
> realistic outlook into the future of the tab strip.

Wish I were at a computer so I could see what that looks like as of current.
Blocks: 1370112
Blocks: 1371306
Assignee: nobody → dale
Blocks: 1367439
Assignee: dale → nobody
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 56.1 - Jun 26
Depends on: 1375893
No longer depends on: 1375893
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Blocks: 1366555
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
No longer blocks: 1377789
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
No longer blocks: 1383887
Attached patch WIP patch (obsolete) — — Splinter Review 
Green on Try, manually tested only on Linux so far.
Keywords: uiwanted
Attached patch WIP patch (obsolete) — — Splinter Review 

        Attachment #8891694 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8891722 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
  

        Attachment #8891824 -
        Attachment is obsolete: true

    
    

    
  
As discussed in Bug 1325902 this is a blocker there. This currently completely breaks container styling such that it isn't visible using the SDK overrides or using nightly container highlighting when the tab is selected.

Also for some reason the container name is taking the tab highlight text colour instead of the same as the icon.

Should we implement another hbox similar to .tab-line as part of this patch/the other bug?

    
    

    
  
(In reply to Jonathan Kingston [:jkt] from comment #11)
> Should we implement another hbox similar to .tab-line as part of this
> patch/the other bug?

Bug 1325902 could add another such node, yes.

    
    

    
      
  

    
    

    
  
Comment on attachment 8891980 [details]
Bug 1349555 - Implement most of the photon tab strip.

https://reviewboard.mozilla.org/r/162992/#review168722

This looks good overall, just some small things need adjustments. I'm not completely through the review, I'll keep looking at it, just wanted to send the first batch of comments.

There are some known issues, but we can/should solve those as a follow-up:

- This doesn't fully implement Photon style for tab bar buttons (need to file a bug).
- This looks very ugly in MacOS when enabling the title bar (Nihanth is giving this a lot of thought in bug 1385520)
- The window control size on OSX is a total mess, especially in combination with UI density, but I can fix that in bug 1375335.

::: browser/base/content/tabbrowser.css:22
(Diff revision 1)
>  .tab-close-button {
>    -moz-binding: url("chrome://browser/content/tabbrowser.xml#tabbrowser-close-tab-button");
>  }
>  
>  .tab-close-button[pinned],
> -.tabbrowser-tabs[closebuttons="activetab"] > * > * > * > .tab-close-button:not([selected="true"]),
> +.tabbrowser-tabs[closebuttons="activetab"] > .tabbrowser-tab > .tab-stack > .tab-content > .tab-close-button:not([selected="true"]),

Uh, yeah, what was that about?

::: browser/themes/shared/customizableui/customizeMode.inc.css
(Diff revision 2)
>  #customization-header {
> -%ifdef MOZ_PHOTON_THEME;
> -  border-bottom: 1px solid ThreeDLightShadow;
>    font-size: 1.75em;
>    line-height: 1.75em;
> -  color: GrayText;

Your changes would be wrong because the ifdef was set wrong and we're landing a fix (bug 1386575), so I'd recommend not touching this code for now. :)

::: browser/themes/shared/customizableui/customizeMode.inc.css:133
(Diff revision 2)
>    line-height: 1.75em;
> -  color: GrayText;
>    font-weight: 200;
> -  padding-bottom: 12px;
> -%else
> -  font-weight: 600;
> +  text-shadow: 0 0 1em var(--toolbar-non-lwt-bgcolor),
> +               0 0 1em var(--toolbar-non-lwt-bgcolor),
> +               0 0 .5em var(--toolbar-non-lwt-bgcolor);

I don't really understand the purpose of that text-shadow and it doesn't have a visual effect on either platform I tested it on.

::: browser/themes/shared/toolbarbuttons.inc.css:7
(Diff revision 2)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +%ifndef MOZ_PHOTON_THEME
>  %filter substitution
>  %define toolbarShadowColor hsla(209,67%,12%,0.35)

Is this definition still used anywhere?

::: browser/themes/windows/windowsShared.inc:8
(Diff revision 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  %filter substitution
>  
>  %define toolbarHighlight rgba(255,255,255,.4)
> -%define fgTabTexture linear-gradient(transparent 2px, @toolbarHighlight@ 2px, @toolbarHighlight@)
> +%define fgTabTexture linear-gradient(@toolbarHighlight@, @toolbarHighlight@)

Is this still used or can we remove it?

        Attachment #8891980 -
        Flags: review?(jhofmann)

    
    

    
  
(In reply to Johann Hofmann [:johannh] from comment #14)
> - This looks very ugly in MacOS when enabling the title bar (Nihanth is
> giving this a lot of thought in bug 1385520)

I don't understand what that has to do with this bug. Must be equally ugly with and without this patch, right?

> ::: browser/themes/shared/customizableui/customizeMode.inc.css
> (Diff revision 2)
> >  #customization-header {
> > -%ifdef MOZ_PHOTON_THEME;
> > -  border-bottom: 1px solid ThreeDLightShadow;
> >    font-size: 1.75em;
> >    line-height: 1.75em;
> > -  color: GrayText;
> 
> Your changes would be wrong because the ifdef was set wrong and we're
> landing a fix (bug 1386575), so I'd recommend not touching this code for
> now. :)

I need to touch it. Please push bug 1386575 only to beta to avoid conflicts here.

> ::: browser/themes/shared/customizableui/customizeMode.inc.css:133
> (Diff revision 2)
> >    line-height: 1.75em;
> > -  color: GrayText;
> >    font-weight: 200;
> > -  padding-bottom: 12px;
> > -%else
> > -  font-weight: 600;
> > +  text-shadow: 0 0 1em var(--toolbar-non-lwt-bgcolor),
> > +               0 0 1em var(--toolbar-non-lwt-bgcolor),
> > +               0 0 .5em var(--toolbar-non-lwt-bgcolor);
> 
> I don't really understand the purpose of that text-shadow and it doesn't
> have a visual effect on either platform I tested it on.

It's for lightweight themes.


> 
> ::: browser/themes/shared/toolbarbuttons.inc.css:7
> (Diff revision 2)
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +%ifndef MOZ_PHOTON_THEME
> >  %filter substitution
> >  %define toolbarShadowColor hsla(209,67%,12%,0.35)
> 
> Is this definition still used anywhere?

Yes, in browser/themes/windows/browser.css and browser-aero.css.

    
    

    
      
  

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #15)
> (In reply to Johann Hofmann [:johannh] from comment #14)
> > - This looks very ugly in MacOS when enabling the title bar (Nihanth is
> > giving this a lot of thought in bug 1385520)
> 
> I don't understand what that has to do with this bug. Must be equally ugly
> with and without this patch, right?

I guess that's subjective, to me the non-selected tabs look much worse, but as I said, I think I'm good if we find a resolution to bug 1385520 eventually.

> > ::: browser/themes/shared/customizableui/customizeMode.inc.css
> > (Diff revision 2)
> > >  #customization-header {
> > > -%ifdef MOZ_PHOTON_THEME;
> > > -  border-bottom: 1px solid ThreeDLightShadow;
> > >    font-size: 1.75em;
> > >    line-height: 1.75em;
> > > -  color: GrayText;
> > 
> > Your changes would be wrong because the ifdef was set wrong and we're
> > landing a fix (bug 1386575), so I'd recommend not touching this code for
> > now. :)
> 
> I need to touch it. Please push bug 1386575 only to beta to avoid conflicts
> here.

I'm pretty sure you don't need to touch the code inside what's currently an ifdef because that's supposed to be ifndef MOZ_PHOTON_THEME. In any case, as long as you don't change the faulty ifdef we'll have no conflicts. I already requested landing bug 1386575 and I don't think you can cancel that. 
     
> > ::: browser/themes/shared/customizableui/customizeMode.inc.css:133
> > (Diff revision 2)
> > >    line-height: 1.75em;
> > > -  color: GrayText;
> > >    font-weight: 200;
> > > -  padding-bottom: 12px;
> > > -%else
> > > -  font-weight: 600;
> > > +  text-shadow: 0 0 1em var(--toolbar-non-lwt-bgcolor),
> > > +               0 0 1em var(--toolbar-non-lwt-bgcolor),
> > > +               0 0 .5em var(--toolbar-non-lwt-bgcolor);
> > 
> > I don't really understand the purpose of that text-shadow and it doesn't
> > have a visual effect on either platform I tested it on.
> 
> It's for lightweight themes.

Ah, I'll try that. Can't we put it behind a :moz-lwtheme selector then?

    
    

    
      
  
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15

    
    

    
  
Comment on attachment 8891980 [details]
Bug 1349555 - Implement most of the photon tab strip.

https://reviewboard.mozilla.org/r/162992/#review169210

Looks good to me, thanks.

        Attachment #8891980 -
        Flags: review?(jhofmann) → review+

    
  

        Attachment #8891980 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8892873 [details]
Bug 1349555 - Implement most of the photon tab strip.

https://reviewboard.mozilla.org/r/163888/#review169216

        Attachment #8892873 -
        Flags: review?(jhofmann) → review+

    
    

    
      
  

    
    

    
  
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9594a10a88a
Implement most of the photon tab strip. r=johannh

    
    

    
  
Backed out for failing browser_windowopen_reflows.js:

https://hg.mozilla.org/integration/autoland/rev/7c6f894e8a3e5415ad2f748365948e392fa5e857

Push with failures (go to the bottom of the failure list): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c9594a10a88aecc4fb4681de5dd73adf7a35dc1a&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Push without failures from previous push

The unexpected passes of the wpt2 tests also start with that push, #servo was curious (if noone comments here, check its log, please).

Failure log reflow: https://treeherder.mozilla.org/logviewer.html#?job_id=120406263&repo=autoland

15:22:27     INFO -  717 INFO TEST-START | browser/base/content/test/performance/browser_windowopen_reflows.js
15:22:28     INFO -  TEST-INFO | started process screenshot
15:22:28     INFO -  TEST-INFO | screenshot: exit 0
15:22:28     INFO -  Buffered messages logged at 15:22:27
15:22:28     INFO -  718 INFO Entering test bound
15:22:28     INFO -  719 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  720 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  721 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  722 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  723 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  724 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  725 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  726 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  727 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  728 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  729 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  730 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  731 INFO Note: ensureDirtyRootFrame threw an exception.
15:22:28     INFO -  Buffered messages finished
15:22:28    ERROR -  732 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_windowopen_reflows.js | unexpected uninterruptible reflow
15:22:28     INFO -  [
15:22:28     INFO -  	"rect@chrome://browser/content/browser-tabsintitlebar.js:106:23",
15:22:28     INFO -  	"_update@chrome://browser/content/browser-tabsintitlebar.js:157:28",
15:22:28     INFO -  	"init@chrome://browser/content/browser-tabsintitlebar.js:48:7",
15:22:28     INFO -  	"handleEvent@chrome://browser/content/tabbrowser.xml:6581:15",
15:22:28     INFO -  	"EventListener.handleEvent*tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml:5999:11",
15:22:28     INFO -  	""
15:22:28     INFO -  ]
15:22:28     INFO -   - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/head.js :: reflow :: line 104
15:22:28     INFO -  Stack trace:
15:22:28     INFO -  chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reflow:104
15:22:28     INFO -  chrome://browser/content/browser-tabsintitlebar.js:rect:106
15:22:28     INFO -  chrome://browser/content/browser-tabsintitlebar.js:_update:157
15:22:28     INFO -  chrome://browser/content/browser-tabsintitlebar.js:init:48
15:22:28     INFO -  chrome://browser/content/tabbrowser.xml:handleEvent:6581
Flags: needinfo?(dao+bmo)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix for OS X. (obsolete)
        — 
          — Splinter Review
      

    


  
As dao mentioned in IRC, some stacks changed on us, and (at least for OS X), there aren't any new reflows here, just signature changes.

The test is disabled for Linux, so we just have to figure out Windows now. Looking at that next.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix for OS X and Windows (obsolete)
        — 
          — Splinter Review
      

    


  
Okay, I've verified that we're really just horsetrading on reflow stacks here. No new reflows got added - just some signatures changed.

If you apply this patch, dao, I believe this should make the tests happy.

        Attachment #8892999 -
        Attachment is obsolete: true

        Attachment #8893003 -
        Flags: review+

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8893011 [details]
Bug 1349555 - Fix windowopen reflows for OS X and Windows.

https://reviewboard.mozilla.org/r/164014/#review169330

        Attachment #8893011 -
        Flags: review?(dao+bmo) → review+

    
  

        Attachment #8893003 -
        Attachment is obsolete: true
Flags: needinfo?(dao+bmo)

    
    

    
  
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #24)
> The unexpected passes of the wpt2 tests also start with that push, #servo
> was curious (if noone comments here, check its log, please).

16:27	bz_sleep	mbrubeck: I would not expect scrollbox.xml to be relevant to any wpt tests...
16:27	bz_sleep	In that scrolling on the web does not use scrollbox
16:28	bz_sleep	So...
16:28	bz_sleep	That test is a "expected fail" across the board
16:28	bz_sleep	right?
16:29	bz_sleep	And it started passing _only_ with stylo with the tabstrip changes
16:29	bz_sleep	Seems suspect

bz is right, web content does not use scrollbox.xml. This looks very much like a stylo bug. I'm guessing that it got triggered by the tab strip (intentionally) becoming slightly taller.

    
    

    
  
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ef0f35adc9f
Implement most of the photon tab strip. r=johannh
https://hg.mozilla.org/integration/autoland/rev/5f1e7e8de03f
Fix windowopen reflows for OS X and Windows. r=dao

    
  
Blocks: 1385702

    
    

    
  
Most of these are on the tier2 stylo builds, but some are tier-1 non-stylo builds.

Also, sorry for using "failures", since these are unexpected passes.

    
    

    
  
Seems like comment 30 again. Hey Manishearth, who should we talk to about this test and a probable Stylo bug?
Flags: needinfo?(manishearth)

    
  
Depends on: 1386964

    
    

    
  
(In reply to Wes Kocher (:KWierso) from comment #32)
> I had to back these out for wpt failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=120531787&repo=autoland

Filed bug 1386964.

I'm going to try to land this with the tab height reduced.
Flags: needinfo?(dao+bmo)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/320bbc19104c
Implement most of the photon tab strip. r=johannh
https://hg.mozilla.org/integration/autoland/rev/9aea49fa17a1
Fix windowopen reflows for OS X and Windows. r=dao

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/320bbc19104c
https://hg.mozilla.org/mozilla-central/rev/9aea49fa17a1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

    
    

    
  
Noticed an improvement:

== Change summary for alert #8513 (as of August 03 2017 07:30 UTC) ==

Improvements:

  5%  Images summary linux64-stylo opt stylo     8,039,664.45 -> 7,641,575.20

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8513

    
    

    
  
Moving over to other bug
Flags: needinfo?(manishearth)

    
    

    
  
Oh, that seems to fail on nostylo builds too

    
  
Depends on: 1387117
Depends on: 1387171
Depends on: 1387296

    
  
Depends on: 1387273

    
  
Depends on: 1387402

    
  
Depends on: 1387655

    
  
No longer depends on: 1387655

    
  
No longer depends on: 1387402

    
    

    
  
I can see the change "Make tabs rectangular (remove tab curve)" the tabs are no longer curve in shape with Latest Nightly 57.0a1 on Windows 8.1 (64 bit).

Build ID : 20170808114032
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170809]
QA Contact: brindusa.tot → ovidiu.boca
Depends on: 1389476
Depends on: 1391328

    
  
No longer depends on: 1391328

    
  
Blocks: 1392699

    
    

    
  
[bugday-20170823] 

status-ff57.0a1 : VERIFIED & FIXED.

Change in tabs can be seen as rectangular now on Win 10 X 64 on Firefox latest Nightly [BuildID : 20170823100553 , 57.0a1 (2017-08-23) (32-bit)]
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1401122

    
  
Depends on: 1402602

    
  
No longer depends on: 1402602
Depends on: 1419272

    
  
Depends on: 1404469

    
    

    
  
Per https://www.reddit.com/r/firefox/comments/7zsml3/could_i_have_found_an_unused_css_variable_andor/,

There is a typo (`--toolbar-gbimage`, should be `--toolbar-bgimage`) introduced by this bug patch.

    
    

    
  
(In reply to fireattack from comment #47)
> Per
> https://www.reddit.com/r/firefox/comments/7zsml3/
> could_i_have_found_an_unused_css_variable_andor/,
> 
> There is a typo (`--toolbar-gbimage`, should be `--toolbar-bgimage`)
> introduced by this bug patch.

That is on file as bug 1440877.
No longer depends on: 1387273
Regressions: 1387273

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: