enable overlay scrollbars on root scrollframes in root content documents

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
mozilla34
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Assignee

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
Posted patch patchSplinter Review
This is easy now.
Depends on: 980500
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Version: 29 Branch → Trunk
Blocks: 1038178
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?
Flags: needinfo?(tnikkel)
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
Assignee

Comment 6

5 years ago
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.
Flags: needinfo?(tnikkel)
Assignee

Updated

5 years ago
Attachment #8405687 - Flags: review?(roc)
Assignee

Comment 9

5 years ago
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.
Assignee

Comment 10

5 years ago
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.
Assignee

Comment 11

5 years ago
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.
Assignee

Comment 12

5 years ago
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.
Attachment #8458431 - Flags: review?(matt.woodrow)
Attachment #8458431 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/7e2f74a8740c
https://hg.mozilla.org/mozilla-central/rev/95f31fa01c7a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1043859
Depends on: 1078316
No longer depends on: 1078316
Depends on: 1078316
Depends on: 1078373
[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.
blocking-b2g: --- → 2.1?
Note that this is already on 2.1. It landed on m-c when that was 34, which is now 2.1.
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.
blocking-b2g: 2.1? → ---
You need to log in before you can comment on or make changes to this bug.