Page MenuHomePhabricator

Unnecessary scroll bars for changesets in Firefox
Closed, ResolvedPublic

Assigned To
Authored By
BYK
Mar 30 2015, 8:19 AM
Referenced Files
F354271: test.html
Mar 30 2015, 9:20 PM
F354174: pasted_file
Mar 30 2015, 6:47 PM
F353814: Untitled.png
Mar 30 2015, 8:19 AM

Description

There are unnecessary scroll bars for certain changeset views on Firefox + Windows combo:

Untitled.png (1×998 px, 188 KB)

A solution my colleague found is to use the following Stylish rule:

/* Changeset scrollbars */
@-moz-document domain('your.phabricator-domain.com') {
  .changeset-view-content {
    overflow-y: hidden;
  }
}

Event Timeline

BYK raised the priority of this task from to Needs Triage.
BYK updated the task description. (Show Details)
BYK added a project: PHUI.
BYK added a subscriber: BYK.

Please provide complete steps to reproduce the issue, Our bug reporting guide is here for reference: https://secure.phabricator.com/book/phabcontrib/article/bug_reports/

We unfortunately won't make changes to Phabricator without understanding the underlying bug and have a proper test plan in place.

chad claimed this task.

I can't find a reproducible case for this ticket, at least on current Firefox testing Mac and Windows running through a few dozen diffs here. If you find a case here on secure please reopen the ticket.

Tested Firefox on Mac and Windows, and I don't see scrollbars. What specific OS/Browser are you using

Windows 8.1 EN with Firefox Aurora 38. That said I've had this issue for the last 2-3 versions so it should happen on latest stable Firefox too. I can verify if you want.

We don't provide support alpha/beta browsers. All bug reports should be against stable browsers and HEAD. You're welcome to test stable, but I have the same environment here and didn't have any luck, I have a second Windows 8.1 computer I'll test here shortly in case my VM is wonky, too. @epriestley, maybe you can reproduce?

We don't provide support alpha/beta browsers. All bug reports should be against stable browsers and HEAD.

FYI this is not "please fix this for me" and I'm not asking corporate support or something. I have solved this for myself already and I took the extra time to report the bug to you because I care about Phabricator. I'd very much prefer you to consider it from that angle.

Just verified that this is also happening on Firefox 36.0.4 (the latest stable) on Windows 8.1

Oh, haha, no I actually do want to be able to reproduce / fix the issue. It's not expected behavior, I just can't find a way to get it to break. Sorry!

I can reproduce this on Firefox 36 on Windows 7.

Removing the overflow-x: auto property hides the scrollbar, which makes zero sense to me, and there's no apparent reason for the scrollbar to exist (there are no CSS properties on the <div > which restrict its height).

Simplest fix I can immediately come up with seems to be an explicit overflow-y: hidden; next to the overflow-x: auto in the .differential-changeset rule.

This looks like a bug in Firefox to me, but I imagine reducing it to a test case is quite involved.

In particular, the .differential-changeset is the thing picking up the scrollbar -- not the .changeset-view-content -- but adding overflow-y: hidden to either element removes the scrollbar.

This looks like a bug in Firefox to me, but I imagine reducing it to a test case is quite involved.

I'll try to do this and cross-link the Bugzilla ticket here if that's it. Sounds good?

Oh, haha, no I actually do want to be able to reproduce / fix the issue. It's not expected behavior, I just can't find a way to get it to break. Sorry!

Welp, sorry then. Written communication without facial expressions I guess :D

I'll try to do this and cross-link the Bugzilla ticket here if that's it. Sounds good?

Yeah, that'd be great if you have time for it.

My main concern is something inside the diff is causing overflow, and we don't know what. For example this is a visual inconsistency with Chrome/Safari:

pasted_file (63×366 px, 13 KB)

The bright spans should not be bleeding out. I'll probably fix that regardless, but something minor like this is likely causing overflow to trigger.

Also, didn't we have some weird scrollbars in FF with ligatures or line-spacing? I can't find that old diff.

This was the old ligature block:

/*
  Disable ligatures in Firefox. Firefox 3 has fancypants ligature support, but
  it gets applied to monospaced fonts, which sucks because it means that the
  "fi" ligature only takes up one character, e.g. It's probably the font's
  fault that it even specifies ligatures (seriously, what the hell?) but
  that's hard to fix and this is "easy" to "fix": custom letter spacing
  disables ligatures, as long as it's at least 0.008333-repeating pixels of
  custom letter spacing. I have no idea where this number comes from, but note
  that .83333.. = 5/6. -epriestley
*/
letter-spacing:     0.0083334px;

Removed in D9240.

@epriestley - I was able to narrow it down to this:

That said now I see the issue on my Chrome too. When I remove the overflow-x: auto rule, the scrollbar disappears. Can you reproduce?

I narrowed it down a bit more, although this is in Safari and the behavior is generally super inconsistent/crazy across browsers: I see scrollbars in Safari and Firefox for your reduced case, but not Chrome. I see scrollbars in Safari and Chrome for my reduced case, but not Firefox (all on OSX).

<!DOCTYPE html>
<html>
  <head>
    <style type="text/css">
      td {
        padding: 0;
      }

      table {
        border-collapse: collapse;
      }

      span {
        border-bottom: 1px solid transparent;
      }

      div {
        overflow-x: auto;
      }
    </style>
  </head>
  <body>

  <div>
    <table>
      <tr>
        <td>
          <span>X</span>
        </td>
      </tr>
    </table>
  </div>

  </body>
</html>

Removing any of those rules removes the scrollbar for me.

I have zero clue what's going on here or whether this is a browser bug or not.

The 1px transparent border on the spans seems kind of suspicious, though.

I think this is the reason: http://stackoverflow.com/a/9014547/90297

Reading the current style value for overflowY in console gives me 'auto':

> getComputedStyle($0).overflowY
< "auto"

Ah, interesting.

It looks like this is probably not a bug then, and D12211 is a fairly reasonable fix.

It looks like this is probably not a bug then, and D12211 is a fairly reasonable fix.

Yup. It may be worth updating the comment in the patch by giving a link to the spec and/or to that StackOverflow answer for future devs. :)