Closed Bug 1383879 Opened 7 years ago Closed 7 years ago

Chrome icons are too small

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
57 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- verified

People

(Reporter: jrmuizel, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

Small chrome icons
422.18 KB, image/png
Details
Fresh profile
1.11 MB, image/png
Details
Before bug 1381993
306.85 KB, image/png
Details
After bug 1381993
303.54 KB, image/png
Details
Bug 1383879 - Always position the nav-bar when the TabsToolbar is not collapsed since the positioning of the animations is dependent on the container having position:relative.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
570a1-MacOS_10.12.5.png
91.71 KB, image/png
Details 
I updated to today's nightly and I get Chrome icons that are too small.
Attached image Small chrome icons — 
This is on 10.9
Attached image Fresh profile — 
With a fresh profile things look better but the refresh button still seems too small
When I switch into fullscreen mode everything looks fine.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Created attachment 8889585 [details]
> Fresh profile
> 
> With a fresh profile things look better but the refresh button still seems
> too small

Better isn't really how I would describe that... Any chance you can find a regression window? Things look fine on latest nightly on my osx 10.12 machine.
Component: Toolbars and Customization → Theme
Flags: needinfo?(jmuizelaar)
Keywords: regression, regressionwindow-wanted
This is as good a regression window as I was able to get:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=de22d4444e76e5b8cdb80e772776d283210eaaf2&tochange=4df8bfcb4e9f1c64e99e04cb0d54376dae98b2de

I got a bunch of messages like the following during bisection:
Unable to find build info using the taskcluster route 'gecko.v2.autoland.revision.327ab38679196ba30e6b5698beeeff3bada6d877.firefox.macosx64-opt'
Flags: needinfo?(jmuizelaar)
Markus, based on the window in comment #5, any chance this is caused by bug 1350643? That seems plausible, at least...
Flags: needinfo?(mstange)
Depends on: 1384136
I am also seeing a small back and refresh icon like attachment 8889585 [details].

I am on macOS 10.12.6 with the 2017-07-24 Nightly.  I have a 2016 15-in MBP with external monitor.

Only a single window on the laptop's screen appears affected (there is another on that screen that is not affected).  All windows on the external monitor appear to be unaffected.
(In reply to :Gijs from comment #6)
> Markus, based on the window in comment #5, any chance this is caused by bug
> 1350643? That seems plausible, at least...

I agree, it seems plausible. Samael, any idea what could be causing this? It's interesting that this bug is happening in the parent process and not in the content process.
Flags: needinfo?(mstange) → needinfo?(sawang)
Blocks: 1350643
Component: Theme → DOM
Product: Firefox → Core
I'm trying to look into this, but couldn't reproduce it on a 2015 15" MBP with macOS 10.12 and an external 1080p monitor. I built a version with one suspicious patch reverted on top of m-c:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95b7bdf402ed9aa708865b824f77cca821046073

Jeff, since you can reproduce it, could you try to install this version and see if the symptom still exists? The dmg link is at 

https://queue.taskcluster.net/v1/task/fQVUa-vjTBK8KfXrQREmxQ/runs/0/artifacts/public/build/target.dmg

If that still doesn't work, could you launch it with MOZ_LOG="WidgetScreen:5" and paste the WidgetScreen log here?
Assignee: nobody → sawang
Flags: needinfo?(sawang) → needinfo?(jmuizelaar)
I tried to intentionally change nsChildView::GetDPI, nsIScreen.contentsScaleFactor / nsIScreen.defaultCSSScaleFactor, and nsChildView::GetDefaultScaleInternal to some strange values, but still couldn't mess up Chrome UI in this way. I got incorrect popup menu / tooltip size & location, and incorrect web content resolutions instead.

In the screenshot it looks the height & alignment of URL bar is somehow incorrect. Not sure how this happens.
FWIW I knew some addons, such as tab-tree can cause similar symptom due to incorrect nav-bar height, but comment 2 mentioned even with fresh profile the symptom exists so I'm confused...
It seems like this is likely caused by bug 1381993
(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> It seems like this is likely caused by bug 1381993

I mean, I guess it's possible, but it's hard to say for me because I can't reproduce. But almost all the changes in that cset are to the animation, and as far as I can tell the only change to the button itself that would affect it at rest is to set position:relative. That wouldn't explain why the back button looks wonky in the fresh profile screenshot, so tbh it seems unlikely to me.
(In reply to Samael Wang [:freesamael] from comment #9)
> If that still doesn't work, could you launch it with
> MOZ_LOG="WidgetScreen:5" and paste the WidgetScreen log here?

Jeff or Ryan, could either of you post the widgetscreen log for a broken build, given people are having trouble reproducing?
Flags: needinfo?(jryans)
Flags: needinfo?(jmuizelaar)
I confirmed that this is caused by bug 1381993 by building locally.
Blocks: 1381993
No longer blocks: 1350643
Flags: needinfo?(jmuizelaar)
I am assuming the WidgetScreen log is no longer needed...?  Please ni? again if you still want it, I am happy to collect it.
Flags: needinfo?(jryans)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> I confirmed that this is caused by bug 1381993 by building locally.

So are there CSS errors when you see this, or something? I guess I don't see why the changes in that bug would affect any other buttons like they do in the screenshot...
Attached image Before bug 1381993 — 

    
    

    
  

      
      
      
      

        Attached image
          
        After bug 1381993
        — 
      

    


  

    
    

    
  
(In reply to :Gijs from comment #18)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> > I confirmed that this is caused by bug 1381993 by building locally.
> 
> So are there CSS errors when you see this, or something? I guess I don't see
> why the changes in that bug would affect any other buttons like they do in
> the screenshot...

Looks like the dynamic height calculation doesn't work right.  In the working case, .customization-target > toolbaritem gets `--toolbarbutton-height: 38px;`.  In the broken case, it is set to 30px.

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #21)
> (In reply to :Gijs from comment #18)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> > > I confirmed that this is caused by bug 1381993 by building locally.
> > 
> > So are there CSS errors when you see this, or something? I guess I don't see
> > why the changes in that bug would affect any other buttons like they do in
> > the screenshot...
> 
> Looks like the dynamic height calculation doesn't work right.  In the
> working case, .customization-target > toolbaritem gets
> `--toolbarbutton-height: 38px;`.  In the broken case, it is set to 30px.

But the height only gets used for the animation container, which should be invisible unless the reload/stop animation is being changed. It also shouldn't impact the navbar sizing. :-\

    
    

    
  
Correct. The custom property that is defined in that style attribute is used by the .toolbarbutton-animatable-image descendant to set the height of the image to equal that of the toolbar.

The container of .toolbarbutton-animatable-image, .toolbarbutton-animatable-box, is abspos so shouldn't affect the layout of the toolbar.

    
    

    
  
(In reply to :Gijs from comment #22)
> > Looks like the dynamic height calculation doesn't work right.  In the
> > working case, .customization-target > toolbaritem gets
> > `--toolbarbutton-height: 38px;`.  In the broken case, it is set to 30px.
> 
> But the height only gets used for the animation container, which should be
> invisible unless the reload/stop animation is being changed. It also
> shouldn't impact the navbar sizing. :-\

Hmm, maybe I am confusing things, tweaking that to the "good" value doesn't fix anything...  I guess it's more of just a data point between good vs. bad windows.

    
    

    
  
jryans, could you apply the patch in bug 1384341 and see if that fixes the problem for you? Note, you cannot use `mach build faster` with that patch because `build faster` won't pickup the Pocket changes.
Flags: needinfo?(jryans)

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> jryans, could you apply the patch in bug 1384341 and see if that fixes the
> problem for you? Note, you cannot use `mach build faster` with that patch
> because `build faster` won't pickup the Pocket changes.

I tried the patch, but it did not help.  Note that I do not have the Library in my toolbar, and also my Library itself doesn't appear to mention Pocket at all, so I wasn't sure how to test a case of having Pocket in the toolbar.
Flags: needinfo?(jryans)

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> > jryans, could you apply the patch in bug 1384341 and see if that fixes the
> > problem for you? Note, you cannot use `mach build faster` with that patch
> > because `build faster` won't pickup the Pocket changes.
> 
> I tried the patch, but it did not help.  Note that I do not have the Library
> in my toolbar, and also my Library itself doesn't appear to mention Pocket
> at all, so I wasn't sure how to test a case of having Pocket in the toolbar.

Okay, we found my Pocket icon (apparently system add-ons need multiple restarts to appear).  Placing the Pocket add-on in my toolbar and using the patch in bug 1384341 only made things more strange, now the area with Pocket and others is also very small.

Disconnecting my external monitor did not help.  (The bad window lives on the laptop's screen.)

For me, it only seems to happen in a single window (out of many) from my default profile.  A clean profile with one window is unaffected.

    
    

    
  
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> I confirmed that this is caused by bug 1381993 by building locally.

Like Jeff, the issue appears to go away for me by reverting bug 1381993.  However, I had some conflict doing the revert using m-c as a base.

Here's what my revert looks like:

https://gist.github.com/jryans/b6707d9bbaf302f16a707db078e555b4

    
    

    
  
Unassign myself according to comment 16 & comment 28.
Assignee: sawang → nobody

    
  
Assignee: nobody → jaws

    
    

    
  
I can't reproduce this bug locally, but I think the bug might be related to the position:relative on the stop/reload button. I pushed a patch to tryserver that might fix this, builds should show up here: https://archive.mozilla.org/pub/firefox/try-builds/jwein@mozilla.com-d0f7b10fbdb0094b4bc26dbc0cba47828784bb91/

Can one of you try the build and let me know if it fixed the issue for you?
Status: NEW → ASSIGNED
Flags: needinfo?(jryans)
Flags: needinfo?(jmuizelaar)

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> I can't reproduce this bug locally, but I think the bug might be related to
> the position:relative on the stop/reload button. I pushed a patch to
> tryserver that might fix this, builds should show up here:
> https://archive.mozilla.org/pub/firefox/try-builds/jwein@mozilla.com-
> d0f7b10fbdb0094b4bc26dbc0cba47828784bb91/
> 
> Can one of you try the build and let me know if it fixed the issue for you?

Not sure if the builds will end up there from TC?

This appears to be your try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f7b10fbdb0094b4bc26dbc0cba47828784bb91

Looks like the macOS build is: 

https://queue.taskcluster.net/v1/task/ATFRBuMrT_iH7a6A-ZYUFw/runs/0/artifacts/public/build/target.dmg

    
    

    
  
Yes, those are the correct links. Thanks for posting them.

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #32)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> > I can't reproduce this bug locally, but I think the bug might be related to
> > the position:relative on the stop/reload button. I pushed a patch to
> > tryserver that might fix this, builds should show up here:
> > https://archive.mozilla.org/pub/firefox/try-builds/jwein@mozilla.com-
> > d0f7b10fbdb0094b4bc26dbc0cba47828784bb91/
> > 
> > Can one of you try the build and let me know if it fixed the issue for you?
> 
> Not sure if the builds will end up there from TC?
> 
> This appears to be your try build:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d0f7b10fbdb0094b4bc26dbc0cba47828784bb91
> 
> Looks like the macOS build is: 
> 
> https://queue.taskcluster.net/v1/task/ATFRBuMrT_iH7a6A-ZYUFw/runs/0/
> artifacts/public/build/target.dmg

No, this build doesn't appear to help.  The icons are still small as before.
Flags: needinfo?(jryans)

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #34)
> No, this build doesn't appear to help.  The icons are still small as before.

Oh actually, I guess it is improved, at least partially!

The bad window now seems correct if I make sure the toolbar doesn't contain Pocket.

If I do add Pocket to the toolbar, the icons become small again.

I believe without your patch, both cases are bad.

So, maybe there are two separate issues.

    
    

    
  
New push with changes for Pocket as well,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=978ea4d9c76a946bce78abe573fc10454aa66fb0

If this fixes the bug 95% of the time (where things only shift wildly during an animation) then I think we should still land this and then get some help from some layout people since it doesn't seem to be an issue in the front-end.
Flags: needinfo?(jryans)

    
    

    
      
  

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #36)
> New push with changes for Pocket as well,
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=978ea4d9c76a946bce78abe573fc10454aa66fb0
> 
> If this fixes the bug 95% of the time (where things only shift wildly during
> an animation) then I think we should still land this and then get some help
> from some layout people since it doesn't seem to be an issue in the
> front-end.

Yes, this version seems to have correctly sized icons with and without Pocket in the toolbar in the tab loaded state.  Occasionally the shifting during loading still happens, but not reliably.

So, I'd call it an improvement over the current state.
Flags: needinfo?(jryans)

    
    

    
  
This build improves things when not loading but they're still reliably sized wrong during loading.
Flags: needinfo?(jmuizelaar)

    
    

    
      
  

    
    

    
  
[Tracking Requested - why for this release]: The UI is broken on 10.9. Though maybe it's photon related in which case it won't impact beta 56?

          tracking-firefox56:
          --- → ?

    
    

    
  
(In reply to Jeff Muizelaar [:jrmuizel] from comment #41)
> [Tracking Requested - why for this release]: The UI is broken on 10.9.
> Though maybe it's photon related in which case it won't impact beta 56?

You can download a build from Cedar[1] to verify if the behavior will affect 56, though it shouldn't as this is all wrapped in Photon ifdefs.

[1] https://treeherder.mozilla.org/#/jobs?repo=cedar

    
    

    
      
  

    
    

    
  
@jrmuizel, jryans was able to confirm that disabling/removing Tree Style Tabs fixed this bug for him. Are you using Tree Style Tabs? Does this bug reproduce for you in Safe Mode?
Flags: needinfo?(jmuizelaar)

    
    

    
  
Nope, I'm not using Tree Style Tabs and running in Safe mode doesn't help.
Flags: needinfo?(jmuizelaar)

    
    

    
  
This does not reproduce on cedar, so I'm changing the tracking flag to 57

          tracking-firefox56:
          ? → ---

          tracking-firefox57:
          --- → ?

    
    

    
  
I created a clean profile to demonstrate the issue.  The only customization after new profile creation was to add the Tree Style Tabs add-on and set it to show tabs on the left with the same TST appearance I use.  (I also set the profile to show tabs from last time.)

https://drive.google.com/file/d/0BwVM8svT30XYTzBYR0JmbnZUZ00/view (too large for bugzilla)

To trigger the small icons with this profile:

1. Start the browser
2. In the existing window, click the dotted line area that appears in the splitter between the tabs on left and the content area, which collapses the tab bar
3. Do one of the following:
  a. Resize the browser window
  b. Open a new browser window and switch back to the first window
4. Icons should now appear small like this bug

        Attachment #8891046 -
        Attachment is obsolete: true

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

    
    

    
  
Comment on attachment 8892088 [details]
Bug 1383879 - Always position the nav-bar when the TabsToolbar is not collapsed since the positioning of the animations is dependent on the container having position:relative.

https://reviewboard.mozilla.org/r/163094/#review168380

::: browser/themes/osx/browser.css:250
(Diff revision 1)
>  
> +%ifdef MOZ_PHOTON_ANIMATIONS
> +#nav-bar {
> +  /* The toolbar button animations require a positioned #nav-bar. This also
> +     positions the toolbar above the bottom of background tabs. */
> +  position: relative;

Looks like there are still some pieces missing here. #nav-bar doesn't always have position: relative on Linux or Windows. Does this bug not happen there?

::: browser/themes/osx/browser.css:263
(Diff revision 1)
>  #TabsToolbar:not([collapsed="true"]) + #nav-bar:-moz-lwtheme {
>    border-top: 1px solid hsla(0,0%,0%,.3);
>    background-clip: padding-box;
>    margin-top: calc(-1 * var(--navbar-tab-toolbar-highlight-overlap));
> +%ifndef MOZ_PHOTON_ANIMATIONS
>    /* Position the toolbar above the bottom of background tabs */

This comment still applies to the z-index.

        Attachment #8892088 -
        Flags: review-

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #49)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=10b4bdce9e96695c010053ef2abb8a4956fc7bbf

This build seems to fix the problem for me.

    
    

    
      
  

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #50)
> Comment on attachment 8892088 [details]
> Bug 1383879 - Always position the nav-bar when the TabsToolbar is not
> collapsed since the positioning of the animations is dependent on the
> container having position:relative.
> 
> https://reviewboard.mozilla.org/r/163094/#review168380
> 
> ::: browser/themes/osx/browser.css:250
> (Diff revision 1)
> >  
> > +%ifdef MOZ_PHOTON_ANIMATIONS
> > +#nav-bar {
> > +  /* The toolbar button animations require a positioned #nav-bar. This also
> > +     positions the toolbar above the bottom of background tabs. */
> > +  position: relative;
> 
> Looks like there are still some pieces missing here. #nav-bar doesn't always
> have position: relative on Linux or Windows. Does this bug not happen there?

The bug doesn't happen on Linux or Windows because both of those themes have the following rule defined:

Linux:
"""
#TabsToolbar:not([collapsed="true"]) + #nav-bar {
  border-top: 1px solid hsla(0,0%,0%,.3) !important;
  background-clip: padding-box;
  /* Move up into the TabsToolbar for the inner highlight at the top of the nav-bar */
  margin-top: calc(-1 * var(--navbar-tab-toolbar-highlight-overlap));
  /* Position the toolbar above the bottom of background tabs */
  position: relative;
  z-index: 1;
}
"""

Windows:
"""
#TabsToolbar:not([collapsed="true"]) + #nav-bar {
  /* Move up into the TabsToolbar for the inner highlight at the top of the nav-bar */
  margin-top: calc(-1 * var(--navbar-tab-toolbar-highlight-overlap));
  /* Position the toolbar above the bottom of background tabs */
  position: relative;
  z-index: 1;
}
"""

On OSX we have a similar rule (also requires tabsintitlebar) but it only applies on -moz-mac-yosemite-theme. 10.9 is the only platform we support that doesn't match -moz-mac-yosemite-theme, which is why this bug is only reproducible on 10.9.

This patch adds a similar rule to what is found on Linux and Windows, but only the position:relative part since we don't need to draw the toolbar shadow / highlight in all cases.

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #53)
> > ::: browser/themes/osx/browser.css:250
> > (Diff revision 1)
> > >  
> > > +%ifdef MOZ_PHOTON_ANIMATIONS
> > > +#nav-bar {
> > > +  /* The toolbar button animations require a positioned #nav-bar. This also
> > > +     positions the toolbar above the bottom of background tabs. */
> > > +  position: relative;
> > 
> > Looks like there are still some pieces missing here. #nav-bar doesn't always
> > have position: relative on Linux or Windows. Does this bug not happen there?
> 
> The bug doesn't happen on Linux or Windows because both of those themes have
> the following rule defined:
> 
> Linux:
> """
> #TabsToolbar:not([collapsed="true"]) + #nav-bar {

Which means that this bug will happen with the tabs toolbar collapsed, doesn't it?

    
    

    
  
We don't need to position the nav-bar if the tabs toolbar is collapsed since the buttons that we would animate (such as stop/reload) are visiblity:collapse when the TabsToolbar is hidden.

    
    

    
      
  

    
    

    
  
Comment on attachment 8892088 [details]
Bug 1383879 - Always position the nav-bar when the TabsToolbar is not collapsed since the positioning of the animations is dependent on the container having position:relative.

https://reviewboard.mozilla.org/r/163094/#review168942

rs=me, though if we ever implement bug 1332447 we will need to update the code here to also work when the tabstrip *is* collapsed.

    
    

    
  
Comment on attachment 8892088 [details]
Bug 1383879 - Always position the nav-bar when the TabsToolbar is not collapsed since the positioning of the animations is dependent on the container having position:relative.

https://reviewboard.mozilla.org/r/163094/#review168944

        Attachment #8892088 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/193272c14083
Always position the nav-bar when the TabsToolbar is not collapsed since the positioning of the animations is dependent on the container having position:relative. r=Gijs

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/193272c14083
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

    
    

    
  

      
      
      
      

        Attached image
          
        570a1-MacOS_10.12.5.png
        — 
      

    


  
The tiny icons have been around for me for a couple weeks. Still seen here 57.0a1 on Mac OS 10.12.5

    
    

    
  
Update: my issue was connected to using the Vertical Tabs Simplified add-on. Once disabled, icons display properly.

    
    

    
  
IIUC, this only affects the Photon theme, so 56 is not affected by virtue of being disabled at this point. Please set the status to affected and request uplift if I got that wrong, though :)

          status-firefox55:
          --- → unaffected

          status-firefox56:
          --- → disabled

          status-firefox-esr52:
          --- → unaffected

    
    

    
  
Since this bug is fixed, I don't feel the need to track it for 57.

          tracking-firefox57:
          ? → ---

    
    

    
  
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-08-03), so I'm marking this bug as VERIFIED. Thanks.
Status: RESOLVED → VERIFIED

          status-firefox57:
          fixed → verified
QA Contact: Virtual
Version: unspecified → 57 Branch
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: