Closed Bug 911574 Opened 11 years ago Closed 11 years ago

Can not scroll on Yahoo Fantasy Sports Football

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox28 wontfix, firefox29 wontfix, firefox30 verified, firefox31 verified, fennec+)

Status:
VERIFIED FIXED
Milestone:
Firefox 31
Tracking Flags:
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- verified
firefox31 --- verified
fennec + ---

People

(Reporter: jthg, Assigned: kats)

References

(
URL
)

Details

Attachments

(3 files, 4 obsolete files)

index73251164.pdf
96.22 KB, application/pdf
Details
Reduced test case.
1.11 KB, text/html
Details
Patch v3
4.15 KB, patch
wesj
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release) Build ID: 20130820141247 Steps to reproduce: 1. Go to Yahoo Sports Fantasy College Football Pickem - My Picks [1]. (if you can't access the page, sign up for a free pickem account) [1] http://football.fantasysports.yahoo.com/college/11183/1 Actual results: 2. Notice in portrait mode, you can only scroll ~ 4/5 of the way down. 3. Switch to landscape mode, and you only can't scroll at all and you only see 1/5 of the page. Expected results: You should be able to scroll the entire page in portait or landscape. WORK AROUNDS: A. If you enable Request Desktop Site, it will scroll the entire page. B. Enabling a private tab does not fix the problem. C. Clearing the cache does not fix the problem.
URL: http://football.fantasysports.yahoo.c...
OS: Linux → Android
Hardware: x86_64 → ARM
Attached file index73251164.pdf —
reported to Yahoo
I also tested this page on CM7 Android 2.3.7 Browser. Same result. So, probably, a problem with Yahoo!?
Using: LG Nexus 4(Android 4.2.2) On Chrome and Dolphin browser the page is displayed correctly. You can scroll down the page in both landscape and portrait mode. On Firefox, on all versions, the page is cut off. Even on Firefox 21. So it is an yahoo issue.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → other
Component: General → Other
Product: Firefox for Android → Tech Evangelism
Version: Firefox 23 → Trunk
This will never be fixed if it's sitting in other and not re-summarized. This needs to take a look at their viewport code to understand what the issue is and to offer a suggestion over to their web-administration and development team for Yahoo Sports. Here is Yahoo's College Football contact-us form: https://io.help.yahoo.com/contact/index?page=contact&locale=en_US&y=PROD_SPORTS_P_COL_FOOT
Alias: yahoo
Assignee: other → nobody
Component: Other → Mobile
Summary: Cannot scroll entire web page → Can not scroll on Yahoo Fantasy Sports Football
Assignee: nobody → miket
OK, reduced test case here: https://miketaylr.com/bzla/911574.html (will upload as well). The missing rule from the previous comment was: html, body { height: 100%; } Chrome (and Opera) Mobile, Android Internet browser, Opera Classic (Presto) Mobile all PASS. Firefox and Firefox Beta FAIL (but only Mobile, can't reproduce on Desktop).
Moving to Firefox for Android, this seems more like a core or product bug than TE, IMO.
Assignee: miket → nobody
Component: Mobile → General
Product: Tech Evangelism → Firefox for Android
tracking-fennec: --- → ?
Thanks for the reduced test case! That makes it much easier to track down.
Blocks: 703573
Attached patch Patch (obsolete) — — Splinter Review
Turns out fixing it for the test case is easy enough. The question is, what does it break? Dun dun dunnnnn...
Try push at https://tbpl.mozilla.org/?tree=Try&rev=ce9f75f1bf73. This will need some testing before I would consider landing it. The most likely mode of regression is that pages that used to scroll asynchronously will now scroll synchronously, and so be slower and jankier in general.
Assignee: nobody → bugmail.mozilla
tracking-fennec: ? → +
I tested the patch more and it does in fact break async scrolling on a number of sites. I need to talk to somebody in layout to figure out why sometimes it's the <html> element that has the overflow and why sometimes it's the <body> element. If we can get consistent behaviour there then we should be able to come up with a fix for this.
Attached patch Patch (obsolete) — — Splinter Review
Ok, this one actually works correctly, I believe. We should just be treating the body like any other element, so if it has overflow:scroll on it then we can do synchronous subframe panning on it. Normal pages have the <html> element scrolling and that still works fine. Note that if you test this, you might run into bug 886969 which this patch does not address.
Attachment #808966 - Attachment is obsolete: true
Attachment #810927 - Flags: review?(wjohnston)
Comment on attachment 810927 [details] [diff] [review] Patch Review of attachment 810927 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +4156,2 @@ > sendMessageToJava({ type: "Panning:Override" }); > + } No need for braces here @@ +4613,3 @@ > * - It has overflow 'auto' or 'scroll' > * - It's a textarea > * - It's an HTML/BODY node Update the comment.
Attachment #810927 - Flags: review?(wjohnston) → review+
Landed with the comment updated and braces removed: https://hg.mozilla.org/integration/fx-team/rev/5a520e97e6a5
The bustage was caused by bug 911152, so this one should be safe to re-land.
Keywords: checkin-needed
Attached patch Patch (obsolete) — — Splinter Review
Updating the patch to the final version that I actually landed (review comments addressed).
Attachment #810927 - Attachment is obsolete: true
Attachment #811683 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/14207ffc2b2f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
I backed this out for breaking iframe scrolling (bug 927863). https://hg.mozilla.org/integration/fx-team/rev/16de756941cf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 27 → ---
Attached patch Patch v2 (obsolete) — — Splinter Review
Same patch you r+'d before, but additionally changes the _hasScrollableOverflow function to treat things that are overflow:visible as scrollable. This fixes this bug without causing the regression that previously happened (bug 927863). This also fixes bug 988991 and bug 953239 (which are dupes of each other).
Attachment #811683 - Attachment is obsolete: true
Attachment #8398092 - Flags: review?(wjohnston)
Comment on attachment 8398092 [details] [diff] [review] Patch v2 Review of attachment 8398092 [details] [diff] [review]: ----------------------------------------------------------------- I'm a little confused. Why does the overflow:visible stuff fix iframe panning?
All elements are by default overflow:visible. So there's a bunch of possible cases: 1) div that is overflow:visible. This is never scrollable 2) div that is overflow:scroll. This is scrollable, and scrollTopMax/scrollLeftMax may be > 0 if the content exceeds the bounds 3) iframe that is overflow:visible. This is scrollable, and scrollTopMax/scrollLeftMax may be > 0 if the content exceeds the bounds 4) iframe that is overflow:scroll. Same as above case So previously we were checking scroll*Max > 0 and overflow:scroll. This caught cases 2 and 4. Case 3 was caught by the special-casing for body elements. With the original patch I removed the special-casing for body elements, and so case 3 ended up regressing. With the new patch I check scroll*Max > 0 and !overflow:hidden. This will catch cases 2, 3, and 4, as desired, without the body element special-casing. Does that make sense?
Comment on attachment 8398092 [details] [diff] [review] Patch v2 Review of attachment 8398092 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +4951,5 @@ > return false; > var computedStyle = win.getComputedStyle(elem); > if (!computedStyle) > return false; > + return !(computedStyle.overflowX == 'hidden' && computedStyle.overflowY == 'hidden'); Add a comment here to explain briefly why "hidden" is all we care about, so that someone doesn't flip this back unknowingly later.
Attachment #8398092 - Flags: review?(wjohnston) → review+
Argh. The robocop tests are in quirks mode, and so for them it's the document.body that's scrollable as opposed to document.documentElement. My patch makes it scroll synchronously rather than async, and that's causing the bustage because the scrolling is lagging behind the simulated input. This will probably affect content in the wild too, so I should find a way to deal with it. Yay for tests that actually catch bugs!
Flags: needinfo?(bugmail.mozilla)
Attached patch Patch v3 — — Splinter Review
Sorry for the churn :( Try run at https://tbpl.mozilla.org/?tree=Try&rev=0133bb33f023, it passes. The difference here is that the quirks mode/standards mode code to pick the right top-level scrolling document. This seems ugly but I think it's the right fix, because the "page size" that we get from gecko to determine how much to scroll implicitly has this condition in it somewhere.
Attachment #8398092 - Attachment is obsolete: true
Attachment #8401116 - Flags: review?(wjohnston)
Comment on attachment 8401116 [details] [diff] [review] Patch v3 Review of attachment 8401116 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +4626,2 @@ > let doc = BrowserApp.selectedBrowser.contentDocument; > + let rootScrollable = (doc.compatMode == "BackCompat" ? doc.body : doc.documentElement); tripple equals please.
Attachment #8401116 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/f4b4df9eba88
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
No longer blocks: 953239
Is this safe enough to uplift to Aurora? We are close to the middle of the cycle and had the SV team verify that the duplicate cases are fixed as well.
Flags: needinfo?(bugmail.mozilla)
I'd like to let it bake on m-c for a few more days before considering uplift. This code has caused regressions in the past and I wouldn't be surprised if there are more.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8401116 [details] [diff] [review] Patch v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): initial native fennec User impact if declined: some pages are not scrollable when they should be, some things (e.g. youtube video embeds) are scrollable when they shouldn't be. Testing completed (on m-c, etc.): on m-c, locally Risk to taking this patch (and alternatives if risky): this code has been the source of regressions before so I would say medium risk. affects android only. String or IDL/UUID changes made by this patch: none
Attachment #8401116 - Flags: approval-mozilla-aurora?
Comment on attachment 8401116 [details] [diff] [review] Patch v3 Given audience size, the risk doesn't scare me about getting this on Aurora but it will have to ride the trains from there so we can hopefully uncover most impacting potential regressions before shipping 30.
Attachment #8401116 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Agreed, I don't want to uplift it any farther than that either.
status-firefox28: affected → wontfix
status-firefox29: affected → wontfix
Verified as fixed on: Builds: 30.0b2 31.0a2 (2014-05-08) 32.0a1 (2014-05-08) Devices: Galaxy Nexus (Android 4.2.1) Asus Transformer Pad TF300T (Android 4.2.1)
Status: RESOLVED → VERIFIED
status-firefox30: fixed → verified
status-firefox31: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: