Closed Bug 901962 Opened 11 years ago Closed 11 years ago

After zooming out, I can't click outside of the initially viewable area

Categories

(Firefox for Android Graveyard :: General, defect)

22 Branch
All
Android
defect
Not set
normal

Tracking

(firefox23 affected, firefox24 affected, firefox25 affected, firefox26 fixed, fennec+)

RESOLVED FIXED
Firefox 26
Tracking Status
firefox23 --- affected
firefox24 --- affected
firefox25 --- affected
firefox26 --- fixed
fennec + ---

People

(Reporter: costa, Assigned: kats)

References

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130627185035

Steps to reproduce:

I set my viewport to width=device-width. I set a min-width on my html and body elements to 600px.


Actual results:

When I load the webpage it loads zoomed in to what I suppose is the device-width. If I pan around and click on things everything works fine, but if I zoom out and try to click on links or buttons, they don't work consistently. From my observation everything to the right of the initially viewable area is not clickable. It's as if the viewport stayed where it was, as oppsed to expanded as I zoomed out. 


Expected results:

I should be able to click on all the links on the page no matter the zoom.
What version of firefox are you seeing this on? Can you check a nightly build (from nightly.mozilla.org) to see if it still happens there? Also if you can attach a testcase that reproduces this it would help us figure out what is going on. Thanks.
If I open this website on my Samsung Galaxy S3 in the Firefox for Android 22 or current Nightly (Aug 6, 2013, Firefox 26.0a1) I find the following bug.

You start out zoomed in slightly. If you pan around like this, all links are clickable. However, if you first zoom all the way out, anything to the right of the originally viewable area is not clickable. Links don't work, focus doesn't change etc. That makes me sad : (
Attachment #786396 - Attachment mime type: text/plain → text/html
might be related to Bug 879279
Yeah, it looks like the same bug to me. Would it be useful if I could reproduce that bug?
I can repro on a Nexus 4 on recent code.
Status: UNCONFIRMED → NEW
Ever confirmed: true
tracking-fennec: --- → ?
I don't know if this is the right approach to fix this bug but it does work and seems somewhat sane to me. Requesting feedback to make sure this is correct before I try to write a test for this and/or push it to try.
Attachment #789037 - Flags: feedback?(dbaron)
OS: Linux → Android
Hardware: x86_64 → All
Attachment #789037 - Flags: feedback?(dbaron) → feedback?(matt.woodrow)
Comment on attachment 789037 [details] [diff] [review]
Use the displayport area instead of the visual overflow rect when building a displaylist

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

I don't think this is correct sorry.

The overflow area of a frame covers all the pixels that are covered by it and its descendants. It shouldn't be affected by what areas are currently visible (the viewport/display port).

I suspect the issue is that the dirtyRect we're passing is incorrect for the display port.
Attachment #789037 - Flags: feedback?(matt.woodrow) → feedback-
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> I suspect the issue is that the dirtyRect we're passing is incorrect for the
> display port.

Can you elaborate on this?

To summarize what I found was happening: I had a page that had a 384-pixel wide CSS viewport (with a 2.0 widget scale, so the CSS viewport was 768 LayoutDevicePixel units wide). I zoomed the page out so I could see an around 600 CSS pixels (1200 LayoutDevicePixels) wide. Then I tapped on the right side of the screen, so the touch event generated had an mRefPoint.x value of something like 1050 (in LayoutDevicePixel units).

That event went through PositionedEventTargeting::FindFrameTargetedByInputEvent, which calls nsLayoutUtils::GetFramesForArea on the point. That call trickles down to building a display list at that "area" (really a 1x1 rect at that point) and hit the code I modified in the patch above. The "visual overflow rect" was only 384 CSS pixels wide, and so the intersect call returned false and it bailed out.

So if the dirty rect is supposed to be different, that means that it needs to be modified somewhere up the call stack, but I'm not sure where. The reason I thought it might be appropriate to use a display port instead of the visual overflow rect was because of similar code at https://hg.mozilla.org/mozilla-central/file/95df3ec3bb70/layout/base/nsLayoutUtils.cpp#l1925
Flags: needinfo?(matt.woodrow)
It sounds like the problem is the clipping performed by the root scrollframe. Are we passing INPUT_IGNORE_ROOT_SCROLL_FRAME to FindFrameTargetedByInputEvent for this? That's the only way you'll be able to get click events to target outside the CSS viewport.
Flags: needinfo?(matt.woodrow)
I played with clipSubdocument in bug 749481 for similar problems and couldn't get anything working, but sounds like we have something prettier now! If/when this is fixed, we should dup them and remove the ugly hack in presShell that we currently have:

http://mxr.mozilla.org/mozilla-beta/source/layout/base/nsPresShell.cpp#6000
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> It sounds like the problem is the clipping performed by the root
> scrollframe. Are we passing INPUT_IGNORE_ROOT_SCROLL_FRAME to
> FindFrameTargetedByInputEvent for this? That's the only way you'll be able
> to get click events to target outside the CSS viewport.

No, INPUT_IGNORE_ROOT_SCROLL_FRAME isn't getting set, and setting it does fix this bug. Is there any reason it shouldn't just always be set?
Comment on attachment 790275 [details] [diff] [review]
Set the ignore-scroll-root flag

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

If there's a reason we shouldn't just always set this please let me know.
Attachment #790275 - Flags: review?(roc)
Comment on attachment 790275 [details] [diff] [review]
Set the ignore-scroll-root flag

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

This patch is fine. However I think we could make FindFrameTargetedByInputEvent always use it and remove its flag.
Attachment #790275 - Flags: review?(roc) → review+
I'm a little concerned about just removing the flag unconditionally, since I tried tracing it backwards through the code to find out where it is originally set and found a few places where it is set to false (I think always in testing code, though). I pushed a try build with it removed to see if any tests rely on it.

https://tbpl.mozilla.org/?tree=Try&rev=5367aca5d059
Assignee: nobody → bugmail.mozilla
tracking-fennec: ? → +
So the try push looks clean enough. :roc, should I just rip the flag out entirely? i.e. from nsMouseEvent, from the various nsIDOMWindowUtils.idl interface methods, etc? Or is there still some value in keeping that code and just not passing it to FindFrameTargetedByInputEvent?
Flags: needinfo?(roc)
There are only two calls to FindFrameTargetedByInputEvent, and they're both from PresShell::HandleEvent, and I think both of them should have the INPUT_IGNORE_ROOT_SCROLL_FRAME flag. So you can rip it out. I don't know why you're mentioning nsMouseEvent etc since INPUT_IGNORE_ROOT_SCROLL_FRAME isn't used there.
Flags: needinfo?(roc)
Ah I see that for synthetic events we sometimes want INPUT_IGNORE_ROOT_SCROLL_FRAME to not be set. Thanks for pointing that out. So please leave it in :-).
Ok, landed the patch as-is:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a68e032ca51

(In reply to Wesley Johnston (:wesj) from comment #10)
> I played with clipSubdocument in bug 749481 for similar problems and
> couldn't get anything working, but sounds like we have something prettier
> now! If/when this is fixed, we should dup them and remove the ugly hack in
> presShell that we currently have:
> 
> http://mxr.mozilla.org/mozilla-beta/source/layout/base/nsPresShell.cpp#6000

Wesj, do you mind taking care of this? You probably know that code better than I do.
https://hg.mozilla.org/mozilla-central/rev/8a68e032ca51
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Costa, I would appreciate if you could test on tomorrow's nightly build to see if that fixes it for you. If it does we can try to get this fix uplifted to the beta channel so that it will be in Firefox 24.
Flags: needinfo?(costa)
Kat, just got back from vacation/adventure, awesome times : ) I just did the test, I'm getting very strange behavior. I load the page, zoom out, then when I click on the link (on the right area of the page) I get a fade animation, but no dotted outline to imply focus. If I click on the link (in the left area of the page) it loads the link. If I click and hold I get the same options (open in new tab, etc) regardless of where on the page.

This is the page I'm using, btw: http://costamichailidis.github.io/webmaking/
Flags: needinfo?(costa)
Ah, I see that too. That should be fixed by bug 788073. However the link is clickable everywhere, right? It's just the focus outline that's missing on the right side.
I'm getting very strange behavior now. Before it didn't direct me anywhere when I clicked on the right side. This time I opened the page in Nightly, zoomed all the way out, clicked on the link and it worked just fine, then when I navigated back it didn't work (the link didn't direct me anywhere, the background just faded to gray and back to white). Then when I clicked four or five times quickly it responded and directed me. Anytime the link actually works at directing me somewhere, I get the dotted focus outline. When it doesn't direct me I get this strange animation (background of the link fades to gray and then back to white) that isn't physically in my css.

I'm sorry this is so inconsistent.
Probably best to file a new bug for the issues you're seeing now.
Is there another test case that you've been working with, that I might test out?
I was testing primarily with the test case you attached to this bug. I wasn't really looking at the background fading or focus outline when I wrote the patch though, I just checked to see if the link was clickable.

The background fading thing is actually explainable by the CSS transition set in your CSS combined with our default :active state style. The transition you set applies to all properties so when we activate the link it fades the background in and out.
Hmm.... Then what might be causing the link to only work sometimes?
I'm not sure. I'm able to click on it every time. Sure, sometimes "clicking" on it doesn't do anything but that's usually because I miss when aiming for the text, or something like that. That happens to me on links on the left side as well.
Depends on: 907489
So after a few tries, I was able to reproduce the behaviour described by costa@innovationbound.com; 
I've opened "https://bug901962.bugzilla.mozilla.org/attachment.cgi?id=786396" 
1. Zoom out.
2. Tap on link "Seven Ways..."
3. Tap back.
4. Zoom out.
5. Tap on link "Seven Ways..."

Actual result:
You can see visual feedback, but there is not action to open the link.

I get this error in the log console whenever I tap on the link and the link does not open:

09-16 15:20:59.363: E/GeckoConsole(11831): [JavaScript Error: "aElement is null" {file: "chrome://browser/content/browser.js" line: 4584}]
Could you file a new bug for this then?
@Pop, that's a good description! I'm seeing the same thing.
Beta uplift?
(In reply to Pop Mihai from comment #31)
> So after a few tries, I was able to reproduce the behaviour described by
> costa@innovationbound.com; 

I can reproduce this as well, thanks for the STR! I've filed bug 917297 for it.

(In reply to Aaron Train [:aaronmt] from comment #34)
> Beta uplift?

I'll request uplift once the merge is done.
Comment on attachment 790275 [details] [diff] [review]
Set the ignore-scroll-root flag

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none, but this is a long-standing bug
User impact if declined: sometimes clicking on links after zooming out doesn't work
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): fairly low risk at this point, it's been baking for a while. there was one regression (bug 907489) which is also a trivial fix that i will request uplift for.
String or IDL/UUID changes made by this patch: none
Attachment #790275 - Flags: approval-mozilla-beta?
Comment on attachment 790275 [details] [diff] [review]
Set the ignore-scroll-root flag

Doesn't meet the bar for landing on Beta sadly.
Attachment #790275 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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: