995519 - enable overlay scrollbars on root scrollframes in root content documents

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Bug 980500 made this possible, but specifically disabled them for this case. It restored scrollbars where their removal was a regression, but root scrollframes in root content documents (well, display roots to be correct) had never had scrollbars on b2g. Bug 980500 didn't enable scrollbars for this case because scrollbars can cause trouble and we wanted a safe patch suitable for uplift. We should enable them on trunk.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

This is easy now.

Comment 2

[Kevin Grandon :kgrandon]

Hey Timothy,

Just chiming in to say that I tried this patch and it's exactly what we're looking for in the vertical homescreen for FxOS. I think it might introduce some side-effects in the displayport as I noticed some content flashing after applying it, but those could also be related to progressive paint.

I was wondering if we could get this prioritized. Any chance of having this sometime soon, possibly within the next few weeks?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

The patch is easy enough to land, but it will likely introduce scrollbars in other places in Gaia that aren't expecting it. We should figure out if that's ok or not.

Comment 4

[Kevin Grandon :kgrandon]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> The patch is easy enough to land, but it will likely introduce scrollbars in
> other places in Gaia that aren't expecting it. We should figure out if
> that's ok or not.

I tested a few apps, and didn't see unwanted scrollbars, but I'm sure it's possible. It does add scrollbars in the browser though, which I think is generally a good thing.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Ok. We can get it landed and see who complains. It might get backed out if there are apps that rely on the current behavior, in which case we might have to look at options to enable it (or disable it) per-app. No need to do that work unless it turns out to be necessary though.

Comment 6

[Timothy Nikkel (:tnikkel)]

The only reason we didn't land this months ago is the possibility of regressions (like bug 980647 could show up in more places) and no one seemed to care about these missing scrollbars. I'm in the same boat at Kats, we can land this now, and see if it causes any problems.

Comment 7

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b29602d9850

Comment 8

[Carsten Book [:Tomcat]]

sorry had to back this out since i guess this caused https://tbpl.mozilla.org/php/getParsedLog.php?id=43804224&tree=Mozilla-Inbound

Comment 9

[Timothy Nikkel (:tnikkel)]

The test fails because of scrollbars (predictably). There are scrollbars on the test, but not the reference. I can't get scrollbars to appear on desktop on the test no matter how small I make the window though, which is odd.

Comment 10

[Timothy Nikkel (:tnikkel)]

The weird thing is that there is already a vertical scrollbar on the reftest (both ref and test), my patch only adds a horizontal scrollbar to the test. I don't understand how that could happen.

Comment 11

[Timothy Nikkel (:tnikkel)]

It looks like the existing scrollbar must come from the containing chrome document (which appears to be remote). I think I can just restrict the width of the content in the reftest so it doesn't generate a scrollbar.

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: limit the width of some divs in the failing reftests

This should be testing the same thing.

I'm away until tuesday, so if anyone is in an hurry to get this landed, the patch plus this test fixup should be green and landable.

Comment 13

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e2f74a8740c
https://hg.mozilla.org/integration/mozilla-inbound/rev/95f31fa01c7a

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7e2f74a8740c
https://hg.mozilla.org/mozilla-central/rev/95f31fa01c7a

Comment 15

[Tony Chung [:tchung]]

[Blocking Requested - why for this release]:

Testing is ongoing by No-jun, but it looks as if landing this change could have minimal risk.  the suggested condition is to fix the dependent bugs that are blocking this as well if we want to uplift.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Note that this is already on 2.1. It landed on m-c when that was 34, which is now 2.1.

Comment 17

[Dietrich Ayala (:dietrich)]

Triage reviewed - already landed on 2.1 so clearing the blocking flag. The comments on this bug (and all the dependent issues) show that it does have enough risk that if we see problems it should be backed out or pref it off.