Closed Bug 763570 Opened 8 years ago Closed 8 years ago

click events in frameset documents offset

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox14 --- verified
firefox15 --- verified
firefox16 --- verified
blocking-fennec1.0 --- .N+
fennec 14+ ---

People

(Reporter: tnikkel, Assigned: kats)

References

()

Details

Attachments

(2 files, 2 obsolete files)

Zoom in on the left frame on https://forums.craigslist.org/?forumID=76&areaID=1 a little bit, pan down, try to click on a link, the actually linked clicked will be offset vertically by what seems to be the vertical pan amount.
tracking-fennec: --- → ?
blocking-fennec1.0: --- → ?
tracking-fennec: ? → 14+
blocking-fennec1.0: ? → +
This seems to be related to zoom to me more than scroll position. If I don't zoom, things are "fine". Quick test page:

http://dl.dropbox.com/u/72157/framesets.htm
Assignee: nobody → wjohnston
Dang it. I don't think there's a quick fix for this.

I now think there must be a bug somewhere in the buildDisplayList code that causes this. We pick up a target to send these events to in browser.js on touchstart:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2609

The target we get in this position often isn't the right one if the user has zoomed. In some cases its not even the right frame. I.e. if you open:

http://dl.dropbox.com/u/72157/framesets.htm

zoom in some, and then pan, you often end up panning the wrong thing. I don't think there's a nice easy bandaid patch for this (or even a non easy one) other than digging into the display list builder. I'll keep banging my head against it for a bit longer.
I think what might be happening is the native browser calls scrollTo(x,y) on the document where x,y are the top left of whats visible on screen. The call has no effect on a frameset document, it doesn't scroll, it has no scroll frame. If the document had a scroll frame we'd build a display list that took this scrolling into account. But we don't so that value is lost.

We could either bias the event coordinate by what the frontend codes thinks is the scroll value for framesets before passing it to hit testing. Or we could record the scrollTo value somewhere and move some frame or display list item by that amount when building a display list. They both sound gross.
Attached patch Hacky Patch (obsolete) — Splinter Review
This is a hacky patch to account for the fast the win.scrollX/Y are always zero in these documents, even when we're scrolled. I super awesome code to cache scroll position in our Tab object. That didn't seem very happy with some of our "find something close by" code, so I disabled it for now in order to put this up. This will also not fix touch events.
Attachment #632892 - Flags: feedback?(bugmail.mozilla)
Looking for feedback on... whether there is a better place to cache these values, and if you've got any genius ideas about better places to "adjust" our event coordinates/targeting.
Does this fix touch events delivered to content? In general I think window.top.scrollX will be wrong for content, and that might result in strange behaviour. I feel like fixing it in layout is better. I haven't looked at the relevant layout code but is it at all possible to generate a scroll frame for framesets?
This will not fix touch events. The number of pages that will ever exist using both touch events and framesets is likely zero. But if there is one, this will not fix them. Its a bandaid for this next release.

tn is probably a better person to answer about whether we can generate a scrollframe here. I'm worried fixing window.top.scrollX will require deviating from the spec, but I don't see anything in the spec saying that frameset pages can't scroll, there was just no way to make them scroll until now.

An alternative fix could catch events in presShell hereish:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#2325

If the doc is a frameset, look up the fake scroll position... somewhere (Java is the only person who would have it at this point?) and adjust the coordinates.
Comment on attachment 632892 [details] [diff] [review]
Hacky Patch

Review of attachment 632892 [details] [diff] [review]:
-----------------------------------------------------------------

So I was thinking something more like this:

diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp
index f62ca24..2ca961a 100644
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -2671,11 +2671,13 @@ nsCSSFrameConstructor::SetUpDocElementContainingBlock(nsIContent* aDocElement)
   bool isScrollable = !isXUL;
 
   // Never create scrollbars for frameset documents.
+#if 0
   if (isHTML) {
     nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(mDocument);
     if (htmlDoc && htmlDoc->GetIsFrameset())
       isScrollable = false;
   }
+#endif
 
   if (isPaginated) {
     isScrollable = presContext->HasPaginatedScrolling();


Applying the above along with a revert of bug 747493 seems to work pretty well. Build at https://people.mozilla.com/~kgupta/tmp/frameset.apk
Attachment #632892 - Flags: feedback?(bugmail.mozilla) → feedback-
(Obviously, the #if 0 should be replaced by #ifndef ANDROID or some such more appropriate define)
blocking-fennec1.0: + → .N+
This works great for me, fixes both bug 747439 and this one.
Attachment #633244 - Flags: review?(tnikkel)
Also fixes bug 764639 as far as I can tell (I'm not able to reproduce it exactly, but the test page behaves normally with these patches).
Attachment #633244 - Flags: review?(tnikkel) → review?(bzbarsky)
Comment on attachment 633243 [details] [diff] [review]
(1/2) Revert of patch from bug 747439

r+ for backout.
Attachment #633243 - Flags: review?(tnikkel) → review+
Comment on attachment 633244 [details] [diff] [review]
(2/2) Generate a scrollframe for framesets on fennec

You need to fix the comments at the beginning of the method a bit.

Is this the right ifdef?  Does b2g define MOZ_WIDGET_ANDROID?

Have you checked _why_ we don't create scrollframes for framesets, by any chance?  Might be worth knowing what this has the potential to break....
(In reply to Boris Zbarsky (:bz) from comment #15)
> You need to fix the comments at the beginning of the method a bit.

Done, attached updated patch.

> Is this the right ifdef?

I believe so. It's the one that seems to be used in all the OMTC code as well for fennec async pan/zoom stuff.

> Does b2g define MOZ_WIDGET_ANDROID?

No, b2g has MOZ_WIDGET_GONK.

> Have you checked _why_ we don't create scrollframes for framesets, by any
> chance?  Might be worth knowing what this has the potential to break....

Not really, no. From the code it looks like just a easy way to disable scrolling on framesets (and probably an optimization) but there might be some other underlying reason that I'm not aware of. I'm guessing roc would be the best person to answer that? File history points to him, anyway.
Attachment #633244 - Attachment is obsolete: true
Attachment #633244 - Flags: review?(bzbarsky)
Attachment #633501 - Flags: review?(bzbarsky)
CC'ing roc for comment on ^
> No, b2g has MOZ_WIDGET_GONK.

OK.  Does it not need this behavior, then?

> File history points to him, anyway.

It does?  File history afaict points to bug 138540, which doesn't seem to have much in the way of rationale, sadly.
(In reply to Boris Zbarsky (:bz) from comment #18)
> > No, b2g has MOZ_WIDGET_GONK.
> 
> OK.  Does it not need this behavior, then?

I don't know for sure, but I think it doesn't. I know that at the moment b2g doesn't use the android scrolling behaviour (bug 750974 is being worked on to make this eventually possible). Regardless I'm hesitant to change this for b2g as well without first knowing that there is a need for it, particularly as this patch may get uplifted to the 15 or even 14.x releases. If b2g needs this behaviour I'd rather do it in a separate patch.

> 
> > File history points to him, anyway.
> 
> It does?  File history afaict points to bug 138540, which doesn't seem to
> have much in the way of rationale, sadly.

Ah, I took some wrong turns in my file history navigation, I think. From the bugs I looked at (72747, 28670, and 243519) and the one you pointed out, it looks like maybe the original purpose of removing the scrollframe was to remove the visible scrollbars. On android this is moot because we don't draw scrollbars in gecko anyway.
Comment on attachment 633501 [details] [diff] [review]
(2/2) Generate a scrollframe for framesets on fennec (v2)

Thanks for looking into that.  r=me
Attachment #633501 - Flags: review?(bzbarsky) → review+
Assignee: wjohnston → bugmail.mozilla
Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5397eb7d95
https://hg.mozilla.org/integration/mozilla-inbound/rev/47a56cdd7869

I also have a try run pushed to here:
https://tbpl.mozilla.org/?tree=Try&rev=1c86c866bfd5

It's colourful, but I'm fairly confident that all the failures are randoms and pre-existing.
Depends on: 747493
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/47a56cdd7869
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 632892 [details] [diff] [review]
Hacky Patch

Obsoleting the hacky patch.
Attachment #632892 - Attachment is obsolete: true
Comment on attachment 633243 [details] [diff] [review]
(1/2) Revert of patch from bug 747439

Requesting aurora/beta approval for both patches

[Approval Request Comment]
Bug caused by (feature/regressing bug #): partly by bug 747493, partly pre-existing
User impact if declined: frameset pages are pretty broken. they don't paint fully on load (need to pan/zoom to trigger paints), rotation makes things paint in strange places (bug 764639), touch events go to the wrong spot
Testing completed (on m-c, etc.): tested on m-c
Risk to taking this patch (and alternatives if risky): kinda risky; would like feedback from roc and/or other people who know layout intimately to minimize risk of fallout. the risk is mobile-only since the patch is basically an #ifdef android change
String or UUID changes made by this patch: none
Attachment #633243 - Flags: approval-mozilla-beta?
Attachment #633243 - Flags: approval-mozilla-aurora?
Attachment #633501 - Flags: approval-mozilla-beta?
Attachment #633501 - Flags: approval-mozilla-aurora?
No longer blocks: 764639
Duplicate of this bug: 764639
Attachment #633243 - Flags: review?(roc)
Attachment #633243 - Flags: approval-mozilla-aurora?
Attachment #633243 - Flags: approval-mozilla-aurora+
Attachment #633501 - Flags: review?(roc)
Attachment #633501 - Flags: approval-mozilla-aurora?
Attachment #633501 - Flags: approval-mozilla-aurora+
A data point that could be used to determine riskyness would be to apply the same change to the desktop browser and see what are the nature of the failures (if any).
Comment on attachment 633501 [details] [diff] [review]
(2/2) Generate a scrollframe for framesets on fennec (v2)

Review of attachment 633501 [details] [diff] [review]:
-----------------------------------------------------------------

very nice
Attachment #633501 - Flags: review?(roc) → review+
Actually, you know what? We should just do this for all documents. It'll simplify things by reducing the differences between mobile and desktop, and because frameset documents will look more like regular documents. I don't see any significant benefit from keeping the current path.

We might want to change frameset { overflow: -moz-hidden-unscrollable } to "hidden" in html.css, as a related but separate change.
Comment on attachment 633243 [details] [diff] [review]
(1/2) Revert of patch from bug 747439

Both patches approved to land on beta tip, to track 14.0.1, but not the relbranch
Attachment #633243 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #633501 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Tap targets are correct for both the craiglist link and for the test page, also all the frames are correctly painted. Marking as verified.

Verified on:
Nightly 16.0a1 2012-07-11/Aurora 15.0a2 2012-07-11/Firefox Mobile 14 beta 12 build 1
HTC Desire
Android 2.2.2
You need to log in before you can comment on or make changes to this bug.