Closed Bug 940691 Opened 11 years ago Closed 11 years ago

Contacts app with APZ on lets you pan to the settings pane

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED DUPLICATE of bug 942995
Project Flags:
blocking-b2g 1.3+

People

(Reporter: milan, Assigned: botond)

References

Details

Attachments

(1 file)

Squish the FrameMetrics::mScrollableRect if the thing has overflow:hidden
813 bytes, patch
tnikkel
: feedback-
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #916185 +++

Take the index.html patch for contacts app from bug 916185, and turn on the APZ in the developer settings.  Create at least one contact.

I can now pan down or right to the settings pane of the contacts app.
Kats, can you take a quick look before you disappear in case this is a widespread issue and we can reuse your insight?
Flags: needinfo?(bugmail.mozilla)
After playing around with this a little bit, my gut feeling is that this has to do with overflow:hidden behaviour. I suspect the apps are laying out multiple screens of content and preventing scrolling currently by use of overflow:hidden. I don't think APZC currently respects overflow:hidden because the mScrollableRect that is on the FrameMetrics reports the full layer size and there is no other information that it uses to determine how far it should be able to scroll. I can try to come up with a quick test for this theory.
Flags: needinfo?(bugmail.mozilla)
Even if this isn't the right approach to fixing it, it confirms my theory above because it prevents panning over to unrelated screens in the contacts app. It also prevents panning on a test page I have that has overflow:scroll on the body.
Attachment #8335559 - Flags: feedback?(tnikkel)
Comment on attachment 8335559 [details] [diff] [review]
Squish the FrameMetrics::mScrollableRect if the thing has overflow:hidden

"overflow: hidden" basically means "clip the content and disable some of the most obvious ways to scroll the element (remove the scrolls bars, ignore some other user input that could scroll it), but they can still be scrolled in some ways, I think. I don't know the details.

This may disable panning on pages that use overflow: hidden where we do actually want the user to be able to pan. For example I think a page could set overflow: hidden and then listen for user input and manually update the scroll position via javascript. I'm not sure how big of a problem this is.
(In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> It also prevents panning on a test page I have that has overflow:scroll
> on the body.

That doesn't seem like something we want.
Comment on attachment 8335559 [details] [diff] [review]
Squish the FrameMetrics::mScrollableRect if the thing has overflow:hidden

So f- in that we can't land this with at least some more info about overflow hidden and scrolling and the problem kats pointed out. Not that I'm against this.
Attachment #8335559 - Flags: feedback?(tnikkel) → feedback-
Blocks: 916185
No longer depends on: 916185
So, is the solution to make the change on Gaia side?
The solution is to make APZC respect overflow:hidden - We need this for parity with all other browsers (yup, really) and it's a pain as an app developer to not be able to use it in this way. Not doing so would be a regression too, given current behaviour without APZC.
Component: Gaia → Panning and Zooming
Product: Firefox OS → Core
Changing the product based on comment 10.
So that we don't wait a week for Kats to be back - any idea as to the size and complexity of this work?
Flags: needinfo?(chrislord.net)
Flags: needinfo?(botond)
(In reply to Chris Lord [:cwiiis] from comment #10)
> The solution is to make APZC respect overflow:hidden - We need this for
> parity with all other browsers (yup, really) and it's a pain as an app
> developer to not be able to use it in this way. Not doing so would be a
> regression too, given current behaviour without APZC.

Is there any reason for an overflow:hidden element to have an APZC at all?
Flags: needinfo?(botond)
blocking-b2g: --- → 1.3+
(In reply to Botond Ballo [:botond] from comment #13)
> Is there any reason for an overflow:hidden element to have an APZC at all?

No, I don't think there is. We should simply not do async scrolling if the element is overflow:hidden in *both* directions. We shouldn't even build ScrollInfoLayers. We can still layerize and we'll still get accelerated scrolling if the app decides to scroll the element programmatically.
And this should be trivial to implement in nsGfxScrollFrameInner::BuildDisplayList.
(In reply to Milan Sreckovic [:milan] from comment #12)
> So that we don't wait a week for Kats to be back - any idea as to the size
> and complexity of this work?

It ought to be reasonably trivial to fix this, as roc mentions in comment #15. You ought to be able to have an initial patch in a day, I'd have thought.
Flags: needinfo?(chrislord.net)
Assignee: nobody → botond
Flags: needinfo?(nhirata.bugzilla)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> And this should be trivial to implement in
> nsGfxScrollFrameInner::BuildDisplayList.

I think nsGfxScrollFrameInner::BuildDisplayList already handles not building a scroll info layer for elements that are overflow:hidden in both directions: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2369

The thing is, nsGfxScrollFrameInner::BuildDisplayList is not the only thing that causes APZCs to be created.

An APZC is created for every container layer whose FrameMetrics reports a scroll id that's not NULL_SCROLL_ID. This corresponds to every frame for which RecordFrameMetrics has been called.

RecordFrameMetrics is called in two places: nsDisplayList::PaintForFrame (for root scroll frames) and nsDisplayScrollLayer::BuildLayer (for everything else). The code in nsGfxScrollFrameInner::BuildDisplayList handles the "everything else" case, but we need to handle the "root scroll frame" case.
I think there are actually two problems here:

   1) An element that shouldn't be scrollable - the app's <html> element, 
      which is overflow:hidden - is. I think this can be fixed as described 
      in the comment above.

   2) An element that should be scrollable - the overflow:scroll <div> 
      that contains all the contacts - isn't. This appears to be caused by 
      the fact that layout lays some frames out differently (in terms of 
      their size) with APZC enabled than without. I need to investigate this
      further.
Depends on: 942995
I wrote a patch for issue (1) (bug 942995), and an APZC is now not created for the overflow:hidden <html> element. However, this had the side effect of all apps being zoomed in a lot and being rendered at a low resolution. I guess there is some code somewhere that assumes that there is an APZC for the root scroll frame of an app, and my patch breaks that assumption? I will continue to investigate.

Moreover, issue (2) from the previous comment remains, and as a result nothing is now scrollable in the contacts app.
(In reply to Botond Ballo [:botond] from comment #19)
> I wrote a patch for issue (1) (bug 942995), and an APZC is now not created
> for the overflow:hidden <html> element. However, this had the side effect of
> all apps being zoomed in a lot and being rendered at a low resolution. I
> guess there is some code somewhere that assumes that there is an APZC for
> the root scroll frame of an app, and my patch breaks that assumption? I will
> continue to investigate.

The problem is in TabChild. When TabChild::Observe() receives a before-first-paint message, it "temporarily" sets the viewport to the default width (980), and sets the resolution to the value required to fit the viewport into the composition bounds (around 1/3 for my screen of width 320). It then calls HandlePossibleViewportChange(), expecting that that will set the viewport and resolution correctly according to the meta-viewport tag. However, HandlePossibleViewportChange() returns early if there is no root APZC, and doesn't do these things, leaving the viewport size and resolution and the undesirable "temporary" values.
(In reply to Botond Ballo [:botond] from comment #20)
> (In reply to Botond Ballo [:botond] from comment #19)
> > I wrote a patch for issue (1) (bug 942995), and an APZC is now not created
> > for the overflow:hidden <html> element. However, this had the side effect of
> > all apps being zoomed in a lot and being rendered at a low resolution. I
> > guess there is some code somewhere that assumes that there is an APZC for
> > the root scroll frame of an app, and my patch breaks that assumption? I will
> > continue to investigate.
> 
> The problem is in TabChild. When TabChild::Observe() receives a
> before-first-paint message, it "temporarily" sets the viewport to the
> default width (980), and sets the resolution to the value required to fit
> the viewport into the composition bounds (around 1/3 for my screen of width
> 320). It then calls HandlePossibleViewportChange(), expecting that that will
> set the viewport and resolution correctly according to the meta-viewport
> tag. However, HandlePossibleViewportChange() returns early if there is no
> root APZC, and doesn't do these things, leaving the viewport size and
> resolution and the undesirable "temporary" values.

Oh and the reason we don't have this problem when APZC is turned off altogether, is that in that case, the "before-first-paint" handling code in TabChild::Observe() returns early.

Basically, TabChild assumes that APZC being turned on implies that there is a root APZC.
Depends on: 943164
With the patch for bug 943164, the zoom and resolution seem fine.

Issue (2) from the previous comments remains, and needs further investigation.
(In reply to Botond Ballo [:botond] from comment #18)
>    2) An element that should be scrollable - the overflow:scroll <div> 
>       that contains all the contacts - isn't. This appears to be caused by 
>       the fact that layout lays some frames out differently (in terms of 
>       their size) with APZC enabled than without. I need to investigate this
>       further.

I investigated this, and here's what's happening:

  - TabChild sets the CSS viewport _width_ to the value requested in
    the meta-viewport tag [1]. Since we specify width=device-width, we 
    get a viewport of screen width (e.g. 320 pixels on hamachi).
  
  - Layout reflows the page in accordance with the specified viewport.
    Since this page does the thing where it puts the another screen
    of the app in an overflow:hidden area of the page to the right of
    the primary screen of the app, the actual page width ends up being 
    twice the screen width (640 pixels).

  - TabChild now computes the minimum zoom for the page as the ratio
    of the screen width to the page width [2]. 
    This ends up being 320 / 640 = 0.5.

  - Finally, TabChild sets the CSS viewport _height_ to be such that,
    at the minimum zoom, the page still fills the screen [3]. This
    causes the CSS viewport height to be twice the screen height
    (920 in this case).

  - Layout again reflows the page in accordance with the specified
    viewport. The scrollable <div> that contains the contacts fits
    just fine into 920 pixels without having to scroll, so no
    scrollable layer, and thus no APZC, is created for it. The page
    as a whole doesn't have an APZC either since it's marked
    overflow:hidden. Therefore, nothing scrolls.

I'm not exactly sure which component is at fault here. Chris, do you
have any ideas, or else know someone who might?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#567
[2] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#609
[3] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#613
Flags: needinfo?(chrislord.net)
Let's see if we can get to the right answer quickly; needinfo on David, Anthony, Matt...
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(dbaron)
Flags: needinfo?(ajones)
(In reply to Botond Ballo [:botond] from comment #23)
> (In reply to Botond Ballo [:botond] from comment #18)
> >    2) An element that should be scrollable - the overflow:scroll <div> 
> >       that contains all the contacts - isn't. This appears to be caused by 
> >       the fact that layout lays some frames out differently (in terms of 
> >       their size) with APZC enabled than without. I need to investigate this
> >       further.
> 
> I investigated this, and here's what's happening:
> 
>   - TabChild sets the CSS viewport _width_ to the value requested in
>     the meta-viewport tag [1]. Since we specify width=device-width, we 
>     get a viewport of screen width (e.g. 320 pixels on hamachi).
>   
>   - Layout reflows the page in accordance with the specified viewport.
>     Since this page does the thing where it puts the another screen
>     of the app in an overflow:hidden area of the page to the right of
>     the primary screen of the app, the actual page width ends up being 
>     twice the screen width (640 pixels).
> 
>   - TabChild now computes the minimum zoom for the page as the ratio
>     of the screen width to the page width [2]. 
>     This ends up being 320 / 640 = 0.5.
> 
>   - Finally, TabChild sets the CSS viewport _height_ to be such that,
>     at the minimum zoom, the page still fills the screen [3]. This
>     causes the CSS viewport height to be twice the screen height
>     (920 in this case).
> 
>   - Layout again reflows the page in accordance with the specified
>     viewport. The scrollable <div> that contains the contacts fits
>     just fine into 920 pixels without having to scroll, so no
>     scrollable layer, and thus no APZC, is created for it. The page
>     as a whole doesn't have an APZC either since it's marked
>     overflow:hidden. Therefore, nothing scrolls.
> 
> I'm not exactly sure which component is at fault here.

After thinking about this for a bit, I think the conflict is between APZ (in the sense of "code that runs only when APZ is enabled", including relevant parts of TabChild) taking the liberty of increasing the CSS viewport height (possibly to larger than the screen height), and there not being a root APZC to scroll to the portion of the page that is off-screen as a result.

Therefore, I'm thinking a simple fix for this is to condition the second update of the CSS viewport (the one that adjusts the height) on there being a root APZC. I'll do that as part of bug 943164.
Having the advantage of not knowing the code at all, I was going to blame the last step instead - the code makes an assumption that fitting into the CSS viewport height means no need for scrolling.
(In reply to Milan Sreckovic [:milan] from comment #26)
> Having the advantage of not knowing the code at all, I was going to blame
> the last step instead - the code makes an assumption that fitting into the
> CSS viewport height means no need for scrolling.

Which code are you referring to - the front end code that made the HTML element overflow:hidden? Or code in Gecko?
(In reply to Botond Ballo [:botond] from comment #25)
> After thinking about this for a bit, I think the conflict is between APZ (in
> the sense of "code that runs only when APZ is enabled", including relevant
> parts of TabChild) taking the liberty of increasing the CSS viewport height
> (possibly to larger than the screen height), and there not being a root APZC
> to scroll to the portion of the page that is off-screen as a result.
> 
> Therefore, I'm thinking a simple fix for this is to condition the second
> update of the CSS viewport (the one that adjusts the height) on there being
> a root APZC. I'll do that as part of bug 943164.

I tried out this approach (see my Part 2 patch for bug 943164), and that seems to solve the problem of the page being laid out incorrectly. The scroll port and scrollable rect of the <div> in question are now correct, and layout correctly creates a scroll info layer for it.

However, the scroll info layer does not make it into the final layer tree - it gets discarded somewhere in FrameLayerBuilder. I'm currently investigating this with the help of :tn.
(In reply to Botond Ballo [:botond] from comment #28)
> However, the scroll info layer does not make it into the final layer tree -
> it gets discarded somewhere in FrameLayerBuilder. I'm currently
> investigating this with the help of :tn.

This was a layout bug. I filed bug 943619 for it along with :tn's suggested fix.

With that fix, we now get the desired scrolling behaviour in the contacts app.

However, I've discovered a bigger problem: now that the app does not have an APZC for its root element, many user interactions with content that's not covered by an APZC are ignored. For example, tapping the '+' button to add a new contact does not work.

This is because with APZ enabled, input events are routed to Gecko via the APZC. The touches go to APZ, APZ does hit testing to find the correct APZC to send them to, the APZC maintains a state machine it uses for gesture detection, and it then notifies Gecko of detected gestures (e.g. a single tap) via GeckoContentController. If hit testing does not turn up an APZC (as is the case a touch on the '+' button in this example), the touch event is discarded by APZ.

This makes me question whether getting rid of the root APZC is the right approach. A lot of code seems to rely on it being there.

Perhaps a better approach would be to still have an APZC, but have it know that the element it corresponds to is overflow:hidden and that it therefore should not be pannable? (That's more or less Kats' suggestion in comment #3. My apologies if my alternate suggestion of not having an APZC has just been a distraction from that.)
Don't we need AZPC's to know about overflow:hidden regardless, for the case where we're hidden for one axis, but scrollable for the other?

Assuming that's true, then it seems simplest to just always have the root AZPC, and have both axis' marked as overflow:hidden.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #30)
> Don't we need AZPC's to know about overflow:hidden regardless, for the case
> where we're hidden for one axis, but scrollable for the other?

Yeah.

> Assuming that's true, then it seems simplest to just always have the root
> AZPC, and have both axis' marked as overflow:hidden.

I agree. I'll try this approach.
Note that the issue raised in comment #23 still arises, just a slightly different variation of it: without my bug 943164, part 2 patch, the CSS viewport height would still be larger than the screen height, and the off-screen portion will still be out of reach, just for a different reason (instead of not having an APZC, we'll have one that can't pan).
(In reply to Botond Ballo [:botond] from comment #32)
> Note that the issue raised in comment #23 still arises, just a slightly
> different variation of it: without my bug 943164, part 2 patch, the CSS
> viewport height would still be larger than the screen height, and the
> off-screen portion will still be out of reach, just for a different reason
> (instead of not having an APZC, we'll have one that can't pan).

Looks like we'll be removing this height adjustment altogether (https://bugzilla.mozilla.org/show_bug.cgi?id=942799), which will resolve this problem.
Flags: needinfo?(dbaron)
Flags: needinfo?(chrislord.net)
Flags: needinfo?(ajones)
So I think if user-scalable=no is set, we shouldn't expand the viewport at all - which I assume would fix the issue found in comment #23. We should probably not expand the viewport height if an explicit height is set, but instead adjust our minimum zoom to not expose empty page area.

wrt root APZC, I agree with comment #30.
(In reply to Botond Ballo [:botond] from comment #29)
> Perhaps a better approach would be to still have an APZC, but have it know
> that the element it corresponds to is overflow:hidden and that it therefore
> should not be pannable? (That's more or less Kats' suggestion in comment #3.
> My apologies if my alternate suggestion of not having an APZC has just been
> a distraction from that.)

In fact, Kats' patch from comment #3 seems to work well as-is.
(In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Created attachment 8335559 [details] [diff] [review]
> Squish the FrameMetrics::mScrollableRect if the thing has overflow:hidden
> 
> Even if this isn't the right approach to fixing it, it confirms my theory
> above because it prevents panning over to unrelated screens in the contacts
> app. It also prevents panning on a test page I have that has overflow:scroll
> on the body.

Which test page is that? If the test case has overflow:hidden on the <html> element and overflow:scroll on the <body> element, then I think you're not supposed to be able to pan it - that's how desktop behaves.
(In reply to Chris Lord [:cwiiis] from comment #34)
> So I think if user-scalable=no is set, we shouldn't expand the viewport at
> all - which I assume would fix the issue found in comment #23. We should
> probably not expand the viewport height if an explicit height is set, but
> instead adjust our minimum zoom to not expose empty page area.

If user-scalable=no is _not_ set, but the page's root scroll frame is overflow:hidden, should we allow zooming out?

I would think not, because then we'd essentially be disobeying the overflow:hidden. But then we can't expand the viewport height in that case, either.

Is it appropriate to condition whether or not we expand the viewport height on whether or not the root scroll frame is overflow:hidden? TabChild (which does the expanding) doesn't currently have access to that information.

Alternatively, we could just never expand the viewport height. What do you think about that?
(In reply to Botond Ballo [:botond] from comment #37)
> (In reply to Chris Lord [:cwiiis] from comment #34)
> > So I think if user-scalable=no is set, we shouldn't expand the viewport at
> > all - which I assume would fix the issue found in comment #23. We should
> > probably not expand the viewport height if an explicit height is set, but
> > instead adjust our minimum zoom to not expose empty page area.
> 
> If user-scalable=no is _not_ set, but the page's root scroll frame is
> overflow:hidden, should we allow zooming out?

I think so. If you wanted this situation *with* zooming, you wouldn't be able to do that if we didn't allow it.

> I would think not, because then we'd essentially be disobeying the
> overflow:hidden. But then we can't expand the viewport height in that case,
> either.

I don't think we're disobeying the overflow:hidden, as that would apply to the CSS viewport(?)

> Is it appropriate to condition whether or not we expand the viewport height
> on whether or not the root scroll frame is overflow:hidden? TabChild (which
> does the expanding) doesn't currently have access to that information.

That's a good question - I don't think we should depend on overflow:hidden so much as there being a viewport height specified and user-scalable=no. We should always expand, unless either the viewport height is specified or user-scalable=no is specified.

> Alternatively, we could just never expand the viewport height. What do you
> think about that?

I don't think we'd want this, for the reasons we do this in the first place; Though I question our current width-centric behaviour slightly. I'd say this is the kind of behaviour that would feel correct:

Portrait orientation:
- Page is wider than it is tall by a factor of 2, minimum zoom should expose entire height of page
- Otherwise, minimum zoom should expose entire width

Landscape orientation:
- Page is taller than it is wide by a factor of 2, minimum zoom should expose entire width of page
- Otherwise, minimum zoom should expose entire height

I think this would lead to natural-feeling behaviour on vertically and horizontally scrolling sites in both orientations, but I also think that this is over-thinking it and our current behaviour is probably fine.
(In reply to Botond Ballo [:botond] from comment #37)
> Alternatively, we could just never expand the viewport height. What do you
> think about that?

As I said in bug 942799, I think we should remove this viewport-height-expanding logic, and instead set the viewport height based only on the device-height/width and the viewport width (not the page width).  In addition to solving this particular bug, this would bring us into better alignment with the spec [1] and with reasonable expectations from developers.

[1] http://dev.w3.org/csswg/css-device-adapt/#constraining-procedure

...and it would naturally give the desired results that Cwiiis describes at the end of comment 38.
Depends on: 942799
Please see the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=942799. Perhaps we can take further discussion there.
(In reply to Timothy Nikkel (:tn) from comment #4)
> Comment on attachment 8335559 [details] [diff] [review]
> Squish the FrameMetrics::mScrollableRect if the thing has overflow:hidden
> 
> "overflow: hidden" basically means "clip the content and disable some of the
> most obvious ways to scroll the element (remove the scrolls bars, ignore
> some other user input that could scroll it), but they can still be scrolled
> in some ways, I think. I don't know the details.
> 
> This may disable panning on pages that use overflow: hidden where we do
> actually want the user to be able to pan. For example I think a page could
> set overflow: hidden and then listen for user input and manually update the
> scroll position via javascript. I'm not sure how big of a problem this is.

I think this scenario should still work with the patch I attached. The patch will prevent APZC from panning, but touch events will still get delivered to content, and content can update the scroll position via javascript. Gecko will then repaint at the new scroll position and inform the APZC. There might be an issue if the APZC decides the do a repaint and clamp the scroll position to the scrollable rect but that should be easy enough to fix.

(In reply to Timothy Nikkel (:tn) from comment #5)
> (In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:kats@mozilla.com) from
> comment #3)
> > It also prevents panning on a test page I have that has overflow:scroll
> > on the body.
> 
> That doesn't seem like something we want.

Oops, I meant overflow:hidden. In other words, I think the patch does the right thing on that test page [1].

(In reply to Botond Ballo [:botond] from comment #36)
> (In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:kats@mozilla.com) from
> comment #3)
> > Created attachment 8335559 [details] [diff] [review]
> > Squish the FrameMetrics::mScrollableRect if the thing has overflow:hidden
> > 
> > Even if this isn't the right approach to fixing it, it confirms my theory
> > above because it prevents panning over to unrelated screens in the contacts
> > app. It also prevents panning on a test page I have that has overflow:scroll
> > on the body.
> 
> Which test page is that? If the test case has overflow:hidden on the <html>
> element and overflow:scroll on the <body> element, then I think you're not
> supposed to be able to pan it - that's how desktop behaves.

Agreed. Test page is at [1].

[1] http://people.mozilla.org/~kgupta/noscroll.html
Does this bug still blocks the activation of the pref? I can't reproduce the original issue now so I'm not sure of what left to do here?
Botond landed my patch over in bug 942995 so that fixed the issue here.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Gaia      6415b8b44068596404c10365394544e94edd5ce5
Gecko     http://hg.mozilla.org/mozilla-central/rev/12ea03a70243
BuildID   20131211040203
Version   29.0a1
ro.build.version.incremental=291
ro.build.date=Wed Dec  4 09:48:04 CST 2013


Seems fixed.  Will need to look at the original bug.
Status: RESOLVED → VERIFIED
Flags: needinfo?(nhirata.bugzilla)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: