Closed Bug 1346961 Opened 7 years ago Closed 7 years ago

Black strip at bottom of page (just above horizontal scrollbar), in pages that trigger horizontal scrollbar

Categories

(Core :: Widget: Gtk, defect, P1)

Product:
Core
Component:
Widget: Gtk
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- verified
firefox53 --- verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: dholbert, Assigned: karlt)

References

Details

(Keywords: regression)

Attachments

(6 files)

testcase 1 (wide content)
613 bytes, text/html
Details
testcase 2 (just "overflow:scroll" on <html>)
93 bytes, text/html
Details
screencast of bug
235.19 KB, video/ogg
Details
screenshot of bug
24.24 KB, image/png
Details
bug 1346961 draw opaque scrollbar background on outermost scrollbar element even for GTK versions >= 3.20
59 bytes, text/x-review-board-request
jhorak
: review+
gchang
: approval-mozilla-aurora+
gchang
: approval-mozilla-beta+
jcristau
: approval-mozilla-esr52+
Details
screencast after landing in comment 13
206.41 KB, video/ogg
Details 
STR:
 1. Load attached testcase.
 2. Look at the bottom of the page.

EXPECTED RESULTS: Just a horizontal scrollbar.
ACTUAL RESULTS: Horizontal scrollbar, plus a black strip above it.

It looks like we're doing some math wrong when we set aside space on the page for the horizontal scrollbar -- we're thinking its twice as tall as it actually is, or something.

Regression pushlog from mozregression:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4b36a41a329ecc9d125eb00b0689e0cb6d2c923a&tochange=013a4088a2307fa31995331c4d18b47fbf75a80b
--> regression from bug 1289148

I suspect this depends on the particulars of your theme. It also seems to be Linux-specific, given the regression range.  FWIW, I'm using Ubuntu and I've reproduced this on two different machines -- one with Ubuntu 16.10, and one with the Ubuntu 17.04 prerelease.

    
    

    
  


  
This happens even if there's no actual overflow (i.e. if the horizontal scrollbar is present but disabled), as shown by this testcase with "overflow:scroll" on <html>.

    
    

    
  
I can reproduce with current Nightly, as well as with Firefox 52 release (from getfirefox.com) and 53 beta and 54 Aurora.

I believe this was a new regression shipped in Firefox 52, based on when bug 1289148 landed. (That's the proximal cause of the regression, based on mozregression testing.)

          status-firefox52:
          --- → affected

          status-firefox53:
          --- → affected

          status-firefox54:
          --- → affected
Keywords: regression

    
    

    
  

      
      
      
      

        Attached video
          
        screencast of bug
        — 
      

    


  
Here's a screencast showing the bug.  Notice how the black strip repaints inconsistently when I resize the window -- I think that indicates it's a rendering artifact -- a chunk of the screen where we're just not painting, or something.

    
    

    
  

      
      
      
      

        Attached image
          
        screenshot of bug
        — 
      

    


  
(And for simplicity, here's just a static screenshot showing the bug.)

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #0)
> I suspect this depends on the particulars of your theme.

Following up on this: I verified that this reproduces using the *default theme* on an Ubuntu 17 prerelease Live CD. (So assume it reproduces using the default theme in 16.10 as well, since it affects a 16.10 environment that I've got.)

It does not seem to affect 16.04, based on some brief testing by mccr8 -- there, it looks like the black region in my screenshots is grey instead, and the orange scrollbar expands to the full breadth of the gray area when it's hovered.  (Kind of a half-overlay-scrollbar effect.)

    
    

    
  
Hi,
could you please check which version of GTK the affected and not affected versions of Ubuntu has? Should be obtained by I guess by:
dpkg --status libgtk-3-0

    
    

    
  
On the Ubuntu 17.04 box:
 Version: 3.22.7-1ubuntu2
On the Ubuntu 16.10 box:
 Version: 3.20.9-1ubuntu2.1
Flags: needinfo?(jhorak)

    
    

    
  
(Those were both the "affected versions", i.e. the ones showing the bug).

The Ubuntu 16.04 box (the unaffected version, i.e. not showing a black bar) has:
  Version: 3.18.9-1ubuntu3.2

    
  
Priority: -- → P1

    
    

    
  
Comment on attachment 8847960 [details]
bug 1346961 draw opaque scrollbar background on outermost scrollbar element even for GTK versions >= 3.20

https://reviewboard.mozilla.org/r/120896/#review123016

Looks good to me. At least on Fedora 25 (gtk 3.22) and Ubuntu 16.10 (gtk 3.20).

        Attachment #8847960 -
        Flags: review?(jhorak) → review+

    
    

    
  
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82dc64a35133
draw opaque scrollbar background on outermost scrollbar element even for GTK versions >= 3.20 r=jhorak+328198

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/82dc64a35133
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox55:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

    
    

    
  
Comment on attachment 8847960 [details]
bug 1346961 draw opaque scrollbar background on outermost scrollbar element even for GTK versions >= 3.20

Approval Request Comment
[Feature/Bug causing the regression]:
libgtk changed its native theming ABI, which caused many problems, tracked
in bug 1264079.  There are still many unresolved issues, but immediately before changes for bug 1289148 landed, the scrollbars with Ubuntu 16.10's default theme
happened to look reasonable, even though not correct.  

[User impact if declined]:
Horizontal scrollbars do not look reasonable with Ubuntu 16.10's default
theme.  There is a high risk of problems even with vertical scrollbars in some other themes.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
No.

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Potentially.

[Why is the change risky/not risky?]:
Implementation is still incomplete after GTK 3.20 ABI changes.
There is risk that this change may highlight other deficiencies in the
implementation, that show only with some GTK themes.
This change is a step in the right direction, however, and so should be an
improvement in more situations than it may be a regression.
Risk is limited to recent GTK versions, where things are still incomplete,
because the patch changes behavior only with those versions.

[String changes made/needed]:
None.

        Attachment #8847960 -
        Flags: approval-mozilla-beta?

        Attachment #8847960 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)

    
    

    
  


  
Here's a screencast of how this behaves now, with the fix (on Ubuntu 16.10 as well as 17.04)

It seems like we now just draw a solid-gray bar (same color as the scrollbar background), where we previously drew a solid-black bar.  And this piece of the scrollbar is still non-functional. :-/  If I click on this solid-gray strip (anywhere at a Y-position that's above than the orange scrollbar's bounds) it has no effect.

So, this is still quite annoying because we seem to be presenting a "dummy" scrollbar target which actually has no effect.  So, still a bit of a papercut. Karl, did you hit this as well, & should I file a followup for this remaining usability issue?

(Vertical scrollbars are simply super-skinny -- no dummy extra track there.)

    
    

    
  
(I think we can consider comment 16 as verification of the fix, modulo karlt's thoughts on the non-functional extra strip of gray in the horizontal scrollbar, shown in my latest screencast.)
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)

    
    

    
  
(Sorry, I meant "we can consider comment 17 as verification", not 16)

    
  
Flags: needinfo?(karlt)

    
    

    
  
In Linux the Firefox does not support overlay scrollbars (scrollbar is actually drawn over part of the widget which the scrollbar contains). Ubuntu is using overlay scrollbars and we seems to have problems with it (at least for Ubuntu's the default theme). 

Karl, is there any significant problem with overlay scrollbars and GTK3 or can I try to write some patch?

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #17)
> It seems like we now just draw a solid-gray bar (same color as the scrollbar
> background), where we previously drew a solid-black bar.  And this piece of
> the scrollbar is still non-functional. :-/  If I click on this solid-gray
> strip (anywhere at a Y-position that's above than the orange scrollbar's
> bounds) it has no effect.

Yes.  Adwaita's thumb and (invisible track) change in breadth on hover of the
scrollbar container.
Gecko's scrollbar track and thumb do not reflow on hover.
Mouse events only scroll on the track and thumb trace, not the scrollbar
container, and the track and thumb are (incorrectly) still the unhovered size.

I've been thinking about working around that by measuring for layout assuming
hover because hit testing will then use hover metrics.  Drawing would need to
compensate in the non-hover situation.

These dimensions change on hover even for GTK apps with
GTK_OVERLAY_SCROLLING=0 in the environment.
 
> So, this is still quite annoying because we seem to be presenting a "dummy"
> scrollbar target which actually has no effect.  So, still a bit of a
> papercut. Karl, did you hit this as well, & should I file a followup for
> this remaining usability issue?

Yes, filing a bug, blocking bug 1264079, would be helpful thanks.  I want to
fix that too, but I'm not sure a fix would be suitable for beta, and a
consistent background color provided by the patch here is at least an
improvement over glaring, changing black strips.

> (Vertical scrollbars are simply super-skinny -- no dummy extra track there.)

That is due to not having ltr/rtl set when asking the theme for
margins, and so the scrollbar container breadth is incorrect.  I wrote a
partial fix for that, but then decided it wasn't much of a win to have a grey
border that does nothing.  It will be better to fix this once the track is
wider.
Flags: needinfo?(karlt)

    
    

    
  
(In reply to Jan Horak from comment #20)
> In Linux the Firefox does not support overlay scrollbars (scrollbar is
> actually drawn over part of the widget which the scrollbar contains).

Yes, bug 1147847 tracks overlay scrollbars.

> Ubuntu is using overlay scrollbars and we seems to have problems with it (at
> least for Ubuntu's the default theme).

Adwaita's non-overlay scrollbars look similar to overlay scrollbars, but the
problems reported in comment 17 are not problems with the overlay scrollbars.
Gecko is using style contexts that don't match the overlay scrollbar styling.

Still, using the overlay scrollbars, as GTK does by default would be the ideal
solution.

> Karl, is there any significant problem with overlay scrollbars and GTK3 or
> can I try to write some patch?

I suspect implementation of overlay scrollbars should be possible, thanks.
The place to start looking would be
http://searchfox.org/mozilla-central/search?q=eIntID_UseOverlayScrollbars

That was designed for Mac overlay scrollbars.
I don't know how similar their behavior is to GTK overlay scrollbars.

I have been refactoring non-overlay scrollbar GTK metric and drawing code
a bit for bug 1343802, so you may like to wait and see what I'm doing there
before changing too much in that code, but you may like to start by adding
eIntID_UseOverlayScrollbars and I suspect you'll find other things to change.

    
    

    
  
Comment on attachment 8847960 [details]
bug 1346961 draw opaque scrollbar background on outermost scrollbar element even for GTK versions >= 3.20

Although there is still a solid-gray dummy bar, the page looks better. Aurora54+ & Beta53+.

        Attachment #8847960 -
        Flags: approval-mozilla-beta?

        Attachment #8847960 -
        Flags: approval-mozilla-beta+

        Attachment #8847960 -
        Flags: approval-mozilla-aurora?

        Attachment #8847960 -
        Flags: approval-mozilla-aurora+

    
    

    
  
Root of the problem is this part of Ambiance Theme css style:

/* Adding margins, so actual visible size is: -GtkRange-slider-width - margin
 * this allows to define some kind of proximity effect also on mouse-enter
 */
.scrollbar.vertical:dir(ltr):not(:hover):not(.dragging),
scrollbar.vertical:dir(ltr):not(:hover):not(.dragging) {
    margin-left: 7px;
}

.scrollbar.vertical:dir(rtl):not(:hover):not(.dragging),
scrollbar.vertical:dir(rtl):not(:hover):not(.dragging) {
    margin-right: 7px;
}

.scrollbar.horizontal:not(:hover):not(.dragging),
scrollbar.horizontal:not(:hover):not(.dragging) {
    margin-top: 7px;
}

More of the source there:
http://bazaar.launchpad.net/~ubuntu-art-pkg/ubuntu-themes/trunk/view/head:/Ambiance/gtk-3.20/gtk-widgets.css#L1597

Since we don't respect dir(ltr), dir(rtl) anyhow, these rules are ignored. However the 'scrollbar.horizontal:not(:hover):not(.dragging)' is taken, therefore the 'extra' 7px size of the horizontal scrollbar. In fact the extra 7px is quite fine, otherwise the vertical scrollbar are too narrow (only 5px) and are very difficult to control. What's bad is that extra 7px for horizontal scrollbar does not process click events (only trough is processing them).

I think things are getting more and more complicated with these css themes. There are very powerful and with current code we're unable to cover all states (take for example the dir(rtl), dir(ltl)), we can also define different box model for example for hover, active or dragging states.

    
    

    
  
Seems like this'd be worth an ESR52 backport as well?

          status-firefox52:
          affected → wontfix

          status-firefox-esr52:
          --- → affected
Flags: needinfo?(karlt)

    
    

    
  
Comment on attachment 8847960 [details]
bug 1346961 draw opaque scrollbar background on outermost scrollbar element even for GTK versions >= 3.20

(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)
> Seems like this'd be worth an ESR52 backport as well?

Maybe.  People using the latest GTK version are better off not using ESR, but
I guess you could claim that about most users.  There may be a case for backport this early in the cycle.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Obvious rendering glitch.

User impact if declined:

Horizontal scrollbars do not look reasonable with Ubuntu 16.10's default
theme.  There is a high risk of problems even with vertical scrollbars in some
other themes.

Fix Landed on Version:

55

Risk to taking this patch (and alternatives if risky):

Implementation is still incomplete after GTK 3.20 ABI changes.
There is risk that this change may highlight other deficiencies in the
implementation, that show only with some GTK themes.
This change is a step in the right direction, however, and so should be an
improvement in more situations than it may be a regression.
Risk is limited to recent GTK versions, where things are still incomplete,
because the patch changes behavior only with those versions.

String or UUID changes made by this patch:

None.
Flags: needinfo?(karlt)

        Attachment #8847960 -
        Flags: approval-mozilla-esr52?

    
    

    
  
Comment on attachment 8847960 [details]
bug 1346961 draw opaque scrollbar background on outermost scrollbar element even for GTK versions >= 3.20

fix rendering glitch on recent gtk3, esr52+

        Attachment #8847960 -
        Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Flags: qe-verify+

    
    

    
  
Indeed, this issue is not reproducible on Ubuntu 16.04, neither on 14.04. 
Could you please try to verify if the issue is still reproducible on the affected platforms, as we don't have available any of those? 
Thank you!
Flags: needinfo?(dholbert)

    
    

    
  
(In reply to Iulia Cristescu, QA [:IuliaC] from comment #31)
> Indeed, this issue is not reproducible on Ubuntu 16.04, neither on 14.04.

Right, this is new as of 16.10.

> Could you please try to verify if the issue is still reproducible on the
> affected platforms, as we don't have available any of those? 
> Thank you!

Already did (in Nightly at least) in comment 17, in response to comment 16.  The situation is slightly complicated as described there, but it's basically fixed, yeah.

I've now also tested:
 - Latest release (52) -- still broken (as expected)
 - Latest Beta 53.0b10 (64-bit) -- fixed
 - Latest Aurora 54.0a2 (2017-04-10) (64-bit) -- fixed

          status-firefox53:
          fixed → verified

          status-firefox54:
          fixed → verified

          status-firefox55:
          fixed → verified
Flags: needinfo?(dholbert)

    
    

    
  
Also:
 - A recent ESR52 treeherder build (which calls itself Firefox ESR 52.0.3 (64-bit)), built from
  https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr52&revision=1fcf426dd867c00fa7b58b1221762bca76189184
 ...and that's fixed as well.

          status-firefox-esr52:
          fixed → verified

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #21)
> > So, this is still quite annoying because we seem to be presenting a "dummy"
> > scrollbar target which actually has no effect.  So, still a bit of a
> > papercut. Karl, did you hit this as well, & should I file a followup for
> > this remaining usability issue?
> 
> Yes, filing a bug, blocking bug 1264079, would be helpful thanks.

Sorry, I don't think I ever followed up on this part. I've now filed bug 1355143 on this.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: