Closed Bug 1020801 Opened 10 years ago Closed 10 years ago

[Text Selection] Hide utility bubble when scrolling and show bubble when scroll end

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia::System::Window Mgmt
Platform:
All
Gonk (Firefox OS)
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mtseng, Assigned: pchang)

References

Details

Attachments

(2 files, 12 obsolete files)

WIP v9 part 1
9.73 KB, patch
pchang
: review+
Details | Diff | Splinter Review
WIP v9 part 2
11.96 KB, patch
pchang
: review+
pchang
: feedback+
Details | Diff | Splinter Review 
Per bug 987040 comment 41, we handle this special case in this bug. Detail use case is described at UX spec[1] page 17 figure 2.

[1] https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8412542
Depends on: 987040
Blocks: CopyPasteLegacy
Summary: [Text Selection] Hide utility bubble when scrolling → [Text Selection] Hide utility bubble when scrolling and show bubble when scroll end
Per latest spec [1], we should show bubble while scroll end.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8448605
Blocks: 921965
Assignee: nobody → pchang
Attached patch wip-bug1020801 (obsolete) — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP (obsolete)
        — 
          — Splinter Review
      

    


  
Pass scroll_start and scroll_end reasons to selection popup to show/hide popup

        Attachment #8465138 -
        Attachment is obsolete: true

        Attachment #8468184 -
        Flags: review?(ehsan)

    
    

    
  
Comment on attachment 8468184 [details] [diff] [review]
WIP

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

::: layout/base/SelectionCarets.cpp
@@ +223,5 @@
>    }
>    mVisible = aVisible;
>  
> +  //Notifiy the selection popup with the following reasons
> +  nsPIDOMWindow* domWindow = mPresShell->GetDocument()->GetWindow();

The document may be null here, which will crash this code.

@@ +228,5 @@
> +  if (domWindow) {
> +    switch (aReason) {
> +      case nsISelectionListener::SCROLLSTART_REASON:
> +      case nsISelectionListener::SCROLLEND_REASON:
> +        domWindow->UpdateCommands(NS_LITERAL_STRING("selectionchange"), GetSelection(), aReason);

Instead of passing these reason values like this, it would be much better if you did this properly.  See the PostReason() calls in nsSelection.cpp for how this is done for other reason codes, please.

        Attachment #8468184 -
        Flags: review?(ehsan) → review-

    
    

    
  
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4)
> Comment on attachment 8468184 [details] [diff] [review]
> WIP
> 
> Review of attachment 8468184 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/SelectionCarets.cpp
> @@ +223,5 @@
> >    }
> >    mVisible = aVisible;
> >  
> > +  //Notifiy the selection popup with the following reasons
> > +  nsPIDOMWindow* domWindow = mPresShell->GetDocument()->GetWindow();
> 
> The document may be null here, which will crash this code.
> 
> @@ +228,5 @@
> > +  if (domWindow) {
> > +    switch (aReason) {
> > +      case nsISelectionListener::SCROLLSTART_REASON:
> > +      case nsISelectionListener::SCROLLEND_REASON:
> > +        domWindow->UpdateCommands(NS_LITERAL_STRING("selectionchange"), GetSelection(), aReason);
> 
> Instead of passing these reason values like this, it would be much better if
> you did this properly.  See the PostReason() calls in nsSelection.cpp for
> how this is done for other reason codes, please.

ehasn, how do you think if I merge scroll_start/scroll_end events into nsSelection and notify scroll events to other listeners, like touchcaret or selectioncaret? And then Touchcaret or selectioncaret can receive scroll events via NotifySelectionListeners().
Flags: needinfo?(ehsan)

    
    

    
  
Wait, after thinking more about this, this is yet another quirk with the way that our UX spec has been written which doesn't integrate well with the way that selections work.  Starting a scroll doesn't modify the underlying selection object in any way, and as such, we should not be dispatching a selectionchange event at all.

The other point is that the UX spec is very vague on several things.  Firstly, what should happen on horizontal scrolls?  The other thing is, what happens during the scroll snapback (i.e. if you keep scrolling past the scroll bounds, don't release your finger, and let the content overscroll and then release your finger to let it snap back to the scroll bounds?  I assume the bubble should be kept hidden in all of these cases.  Also, what needs to happen as you pinch to zoom in/out?

When these details are determined, the right place for receiving these notifications is the async pan/zoom controller.  Chatting with Botond, he pointed me to APZStateChange <http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/public/GeckoContentController.h?from=GeckoContentController.h#119> which for example already has a StartPanning member.  These notifications are delivered to TabChild here: <http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?from=TabCHild.cpp#1874>.  Once we determine the exact behavior we need to support, we should:

1) Hook up SelectionCarets to those notifications.  Specifically, the LaunchScrollEndDetector/FireScrollEnd seems completely wrong to me.  The component which *knows* what scrolling is happening on the screen is APZC, so we should rely on notifications that it sends out, and add our own notification if we need ones that currently do not exist.

2) Expose the events that will enable us build the desired UX here through the mozbrowser API by adding a new event (or several new ones).  These events may need to expose state that is currently invisible to content (for example, take overscrolling etc into account.)  But until the UX spec is extended to cover the details above, it's hard to say what those events need to be exactly.

Sorry I didn't realize the scope of this sooner!  Please let me know if this makes sense.
Flags: needinfo?(ehsan)

    
    

    
  
I have a problem with APZC. If APZC is disabled, do we still receive notifications from APZC? If not, we should also find who fire those notifications when APZC is not enabled.

    
    

    
  
(In reply to Morris Tseng [:mtseng] from comment #7)
> I have a problem with APZC. If APZC is disabled, do we still receive
> notifications from APZC? If not, we should also find who fire those
> notifications when APZC is not enabled.

Perhaps Botond can help?  I know next to nothing about APZC.  :)
Flags: needinfo?(botond)

    
    

    
  
(In reply to Morris Tseng [:mtseng] from comment #7)
> I have a problem with APZC. If APZC is disabled, do we still receive
> notifications from APZC?

No. However, if this functionality is targeting Firefox OS, then there should be no reason for APZ to be disabled.
Flags: needinfo?(botond)

    
    

    
  
If I'm reading comment 6 correctly, there is mention of notifying content about overscroll. Kats has previously stated, in the context of another bug, that this isn't a great idea because of the lag between it actually happening on the compositor thread, and the notification being processed by the content thread. CC'ing him to loop him into the discussion.

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #10)
> If I'm reading comment 6 correctly, there is mention of notifying content
> about overscroll. Kats has previously stated, in the context of another bug,
> that this isn't a great idea because of the lag between it actually
> happening on the compositor thread, and the notification being processed by
> the content thread.

To be fair, that objection was to a proposal to notify web content about overscroll, and may not apply to notifying an internal Gecko consumer on the content thread.

    
    

    
  
In this case the APZStateChange notifications should be the right thing to use. Although we don't fire a special notification just for overscroll, the StartPanning/StopPanning stuff should include any sort of movement by the APZ, which includes overscroll. I think a good rule of thumb is that you want to keep the bubble hidden as long as the scrollbars are visible, and so you should use the same notifications as we use for that.

That being said, there is still a latency problem here because you can start panning in the compositor and the child process may not receive the notification until later, but that's unavoidable as long as the bubble is drawn by gecko and not the compositor. The same thing happens with the scrollbars.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> Although we don't fire a special notification just for overscroll, the
> StartPanning/StopPanning stuff should include any sort of movement by the
> APZ, which includes overscroll. I think a good rule of thumb is that you
> want to keep the bubble hidden as long as the scrollbars are visible, and so
> you should use the same notifications as we use for that.

Makes sense. In terms of the specific notifications, I'm pretty sure you meant TransformBegin/TransformEnd.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> In this case the APZStateChange notifications should be the right thing to
> use. Although we don't fire a special notification just for overscroll, the
> StartPanning/StopPanning stuff should include any sort of movement by the
> APZ, which includes overscroll. I think a good rule of thumb is that you
> want to keep the bubble hidden as long as the scrollbars are visible, and so
> you should use the same notifications as we use for that.
> 
> That being said, there is still a latency problem here because you can start
> panning in the compositor and the child process may not receive the
> notification until later, but that's unavoidable as long as the bubble is
> drawn by gecko and not the compositor. The same thing happens with the
> scrollbars.

Agree to use APZStateChange notification to show/hide the bubble. But I find there are some duplicate events(TransformBegin/TransformEnd) in TabChild::RecvNotifyAPZStateChange when scroll snapback on homescreen.

I assume we could hide bubble after receiving TransformEnd but it has two TransformEnd events for scoll snapback.

GeckoContentController::StartTouch      <-- start to scroll snapback on homescreen
GeckoContentController::TransformBegin
GeckoContentController::StartPanning    <-- receive panning
GeckoContentController::EndTouch        <-- last touch and start snapback
GeckoContentController::TransformEnd   
GeckoContentController::TransformBegin  
GeckoContentController::TransformEnd    <-- end of snapback

    
    

    
  
Instead of using APZStateChange notification to check scrolling, I find the scroll observers is a better solution to receive scrolling events because it can support with/without APZ. In b2g desktop, it only has one process, therefore, the apz is off by default.

The following is the callstack in child/parent side when scrollobservers receive scroll events.
And I find the TabChildBase::UpdateFrameHandler is triggered from APZController via RequestContentRepaint call. And these ContentRepaint calls are triggered from OnTouchEnd/UpdateAnimation.

botond, could I treat the scroll observer notification as the same as APZStateChange notification based on above callstack analysis?
And the good thing is scroll observer also supports scroll event notification when APZ is diabled(b2g_desktop case).

But scroll observer doesn't provide the scroll end event. Maybe we can add APZStateChange notification to nsGfxScrollFrame.


//callstack when apz is on
[TabChild]
nsDocShell::NotifyScrollObservers
mozilla::ScrollFrameHelper::ScrollToImpl
mozilla::ScrollFrameHelper::CompleteAsyncScroll
mozilla::ScrollFrameHelper::ScrollToWithOrigin
mozilla::ScrollFrameHelper::ScrollToCSSPixelsApproximate
mozilla::layers::ScrollFrameTo
mozilla::layers::APZCCallbackHelper::UpdateRootFrame
mozilla::dom::TabChildBase::ProcessUpdateFrame
mozilla::dom::TabChildBase::UpdateFrameHandler                 

[TabParent]
TabParent::SendUpdateFrame
TabParent::UpdateFrame
...
AsyncPanZoomController::RequestContentRepaint
AsyncPanZoomController::OnTouchEnd/UpdateAnimation

    
  
Flags: needinfo?(botond)

    
    

    
  
About the scroll end event, we can call NotifyScrollObserver[1] in ScrollbarActivityStopped[2] which is trigged by GeckoContentController::TransformEnd case.

[1]http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2307
[2]http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#96
[3]http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1890

But it receives Transform_end twice for scroll snapback case.

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #13)
> Makes sense. In terms of the specific notifications, I'm pretty sure you
> meant TransformBegin/TransformEnd.

Sorry, yes, that's what I meant.

(In reply to peter chang[:pchang][:peter] from comment #14)
> Agree to use APZStateChange notification to show/hide the bubble. But I find
> there are some duplicate events(TransformBegin/TransformEnd) in
> TabChild::RecvNotifyAPZStateChange when scroll snapback on homescreen.

This sounds like a bug. We shouldn't fire a TransformEnd/TransformBegin between the panning and overscroll phases. We can fix that.

(In reply to peter chang[:pchang][:peter] from comment #15)
> botond, could I treat the scroll observer notification as the same as
> APZStateChange notification based on above callstack analysis?
> And the good thing is scroll observer also supports scroll event
> notification when APZ is diabled(b2g_desktop case).

In theory, yes, you can use the scroll notification to get a *start* scrolling indication. But as you noticed, there's no way for you to get the end of the scroll, which is a problem.

> Maybe we can add
> APZStateChange notification to nsGfxScrollFrame.

I'm not sure what you mean by this. Even if you did wire the APZStateChange notification through to nsGfxScrollFrame, it's not going to be fired on B2G desktop because APZ is disabled. So that doesn't help you at all.

Tying the bubble visibility to the scrollbar activity makes sense to me - I think that's a good idea. We can fix the duplicate TransformEnd issue which should solve any issues you were seeing. Does that sound reasonable?

    
    

    
  
I filed bug 1053766 for the duplicate TransformBegin/TransformEnd notifications.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> (In reply to Botond Ballo [:botond] from comment #13)
> > Makes sense. In terms of the specific notifications, I'm pretty sure you
> > meant TransformBegin/TransformEnd.
> 
> Sorry, yes, that's what I meant.
> 
> (In reply to peter chang[:pchang][:peter] from comment #14)
> > Agree to use APZStateChange notification to show/hide the bubble. But I find
> > there are some duplicate events(TransformBegin/TransformEnd) in
> > TabChild::RecvNotifyAPZStateChange when scroll snapback on homescreen.
> 
> This sounds like a bug. We shouldn't fire a TransformEnd/TransformBegin
> between the panning and overscroll phases. We can fix that.
> 
> (In reply to peter chang[:pchang][:peter] from comment #15)
> > botond, could I treat the scroll observer notification as the same as
> > APZStateChange notification based on above callstack analysis?
> > And the good thing is scroll observer also supports scroll event
> > notification when APZ is diabled(b2g_desktop case).
> 
> In theory, yes, you can use the scroll notification to get a *start*
> scrolling indication. But as you noticed, there's no way for you to get the
> end of the scroll, which is a problem.
> 
> > Maybe we can add
> > APZStateChange notification to nsGfxScrollFrame.
> 
> I'm not sure what you mean by this. Even if you did wire the APZStateChange
> notification through to nsGfxScrollFrame, it's not going to be fired on B2G
> desktop because APZ is disabled. So that doesn't help you at all.

Right now SelectionCarets has a ScrollEndDetectorTimer(default 300ms) which will be reset when scroll events happen, otherwise, it triggers scroll end event after timeout. 

After adding the scroll end event when apz is on, we can cancel ScrollEndDetectorTimer and show the bubble. For apz is off, we just reuse ScrollEndDetectorTimer.

>http://dxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?from=SelectionCarets.cpp&case=true#892
> 
> Tying the bubble visibility to the scrollbar activity makes sense to me - I
> think that's a good idea. We can fix the duplicate TransformEnd issue which
> should solve any issues you were seeing. Does that sound reasonable?

Yes, that will be great for me.

    
    

    
  
Sounds like fixing bug 1053766 will enable you to use the APZStateChange notifications.
Flags: needinfo?(botond)

    
    

    
  
While discussing ways to fix bug 1053766, Kats and I came across an edge case involving scroll handoff that may be relevant to you.

The APZStateChange notification is per-{scrollable element}, as suggested by TabChild::RecvNotifyAPZStateChange having a ViewID parameter.

Consider a page with a parent scrollable element P, and a child scrollable element C.

If you pan C, and the pan exhausts C's scroll range, the scroll is handed off to P, and if P has room to scroll in the relevant direction, P will scroll. (The same thing happens for flings.) However, the APZStateChange notifications will be sent as if C was continuing to scroll.

To make this clearer, consider the following three points in time:

  T0: You start panning on C.
  T1: The pan is handed off from C to P.
  T2: You finish panning.

The APZStateChange notifications you get will be:

  T0: TransformBegin for C
  T1: nothing
  T2: TransformEnd for C

At no point do you get an APZStateChange notification for P, even though P is the thing scrolling from T1 to T2.

Does this present a problem for you? That is, would you want a bubble on content in P (but outside of C) to be hidden while P is undergoing a scroll that was handed off from C?
Flags: needinfo?(pchang)

    
    

    
  
Based on comment 19, create bug 1054901 to add scroll start/stop notification from apz.
Flags: needinfo?(pchang)

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #21)
> While discussing ways to fix bug 1053766, Kats and I came across an edge
> case involving scroll handoff that may be relevant to you.
> 
> The APZStateChange notification is per-{scrollable element}, as suggested by
> TabChild::RecvNotifyAPZStateChange having a ViewID parameter.
> 
> Consider a page with a parent scrollable element P, and a child scrollable
> element C.
> 
> If you pan C, and the pan exhausts C's scroll range, the scroll is handed
> off to P, and if P has room to scroll in the relevant direction, P will
> scroll. (The same thing happens for flings.) However, the APZStateChange
> notifications will be sent as if C was continuing to scroll.
> 
> To make this clearer, consider the following three points in time:
> 
>   T0: You start panning on C.
>   T1: The pan is handed off from C to P.
>   T2: You finish panning.
> 
> The APZStateChange notifications you get will be:
> 
>   T0: TransformBegin for C
>   T1: nothing
>   T2: TransformEnd for C
> 
> At no point do you get an APZStateChange notification for P, even though P
> is the thing scrolling from T1 to T2.
> 
> Does this present a problem for you? That is, would you want a bubble on
> content in P (but outside of C) to be hidden while P is undergoing a scroll
> that was handed off from C?

I will try to create test case to double check the question you mentioned.

    
  
Depends on: 1054901

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v2 (obsolete)
        — 
          — Splinter Review
      

    


  
This patch is depended on bug 1054901 and now it could listen the scroll start/stop from apz

        Attachment #8468184 -
        Attachment is obsolete: true

    
  

        Attachment #8474472 -
        Flags: review?(ehsan)

    
    

    
  
Comment on attachment 8474472 [details] [diff] [review]
WIP v2

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

I still believe we should do what I suggested in comment 6 here.  If you disagree, please explain why.  :-)

        Attachment #8474472 -
        Flags: review?(ehsan) → review-

    
    

    
  
Comment on attachment 8474472 [details] [diff] [review]
WIP v2

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

::: layout/base/SelectionCarets.h
@@ +63,5 @@
>  
>    // nsIScrollObserver
>    virtual void ScrollPositionChanged() MOZ_OVERRIDE;
> +  virtual void ScrollPositionChangeStarted() MOZ_OVERRIDE;
> +  virtual void ScrollPositionChangeStopped() MOZ_OVERRIDE;

ehsan, right now the notifications of above two APIs are generated from the  APZStateChange::TransformBegin/End which is the idea in comment 17 to align the scrollbar activity. And with this patch, it works for zoom/scroll snapback.

But the name of these two APIs are wrong based on bug 1054901 comment 3. I will work on the next WIP to fix the function name.

    
    

    
  
Before working further on the implementation, I recommend touching base with UX in order to clarify the unknown issues in comment 6.  Right now I'm not even sure what problem we're solving here exactly. :-)

    
    

    
  
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #27)
> Before working further on the implementation, I recommend touching base with
> UX in order to clarify the unknown issues in comment 6.  Right now I'm not
> even sure what problem we're solving here exactly. :-)

I will ask UX to update the specs about the following items and update the spec soon.
a. scroll snapback
b. zoom in/out
c. vertical/horizontal scroll

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #28)
> I will ask UX to update the specs about the following items and update the
> spec soon.
> a. scroll snapback
> b. zoom in/out
> c. vertical/horizontal scroll

When each of all 3 cases is happening (scrolling snapback/zooming in or out/scrolling vertically or horizontally), the bubble should hide. The bubble will show again after scrolling/zooming ends.
I'll add them into the spec asap. It should be early next Monday since I'm on PTO this Thursday and Friday.

    
    

    
  
Thanks, Omega, this makes sense!

    
    

    
  
A few more questions for UX based on the discussion in comment 21:

What should happen to the bubble in the case of nested scrollable frames? Say you have a page with an iframe that is scrollable. If you have a selection bubble in the top-level page, and then scroll the iframe, should the bubble disappear as you're doing it? What if you're scrolling the iframe, and you reach the end, and the top-level page starts scrolling because of the scroll handoff behaviour? Should the bubble disappear then?

Also what about the reverse case - say you have a bubble in the iframe and then you scroll the top-level page. Should the bubble disappear?
Flags: needinfo?(ofeng)

    
    

    
  
In order to support UX spec in comment 29, I create bug 1054901 to send out the notification about APZStateChange::TransformBegin/End.

In bug 1054901 attachment 8476420 [details] [diff] [review], there are three callbacks in nsIScrollObserver.
a. virtual void ScrollPositionChanged()
b. virtual void ScrollViewChangeStarted()
c. virtual void ScrollViewChangeStopped()

If APZ is enabled, we only need to listen b and c that covers scroll snapback, zoom in/out, and vertical/horizontal scrolling.

If APZ is disabled, we only receive callback a that covers zoom in/out and veritical/horizontal scrolling.(no scrolling snapback when APZ is off)

Now SelectionCarets[1] is a derived class from ScrollObserver to recevie above callbacks and show/hide itself/Selection bubble based on the notification.

Therefore, we could meet the UX spec in comment 29 with bug 1054901.

But now I have another idea is let nsSelection inherits from nsIScrollObserver and nsSlection can sendout to SelectionListener about APZ state changed events. Therefore, Both TouchCaret and SelectionCarets could receive these notification and take action.

Eshan, how do you think?

[1]http://dxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.h#46

    
  
Flags: needinfo?(ehsan)

    
    

    
  
Making nsSelection implement nsIScrollObserver is a good idea I think.
Flags: needinfo?(ehsan)

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> A few more questions for UX based on the discussion in comment 21:
> 
> What should happen to the bubble in the case of nested scrollable frames?
> Say you have a page with an iframe that is scrollable. If you have a
> selection bubble in the top-level page, and then scroll the iframe, should
> the bubble disappear as you're doing it? What if you're scrolling the
> iframe, and you reach the end, and the top-level page starts scrolling
> because of the scroll handoff behaviour? Should the bubble disappear then?
> 
> Also what about the reverse case - say you have a bubble in the iframe and
> then you scroll the top-level page. Should the bubble disappear?

Hi Kartikaya,
In all the above cases, the bubble disappears when user starts scrolling and appears after user ends scrolling, no matter the selected and the scrolled elements are in different levels.
Flags: needinfo?(ofeng)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Patch includes the following chanages.
a. make nsSelection to listen scroll notification from nsIScrollObserver.
b. notify selection listener with the scroll start/end reason

        Attachment #8474472 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v4 (obsolete)
        — 
          — Splinter Review
      

    


  
Remove some redundant changes.

        Attachment #8478165 -
        Attachment is obsolete: true

        Attachment #8478168 -
        Flags: review?(ehsan)

    
    

    
  
(In reply to Omega Feng [:Omega] [:馮於懋] (PTO until 8/22) from comment #34)
> Hi Kartikaya,
> In all the above cases, the bubble disappears when user starts scrolling and
> appears after user ends scrolling, no matter the selected and the scrolled
> elements are in different levels.

Sounds good, thanks.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37)
> (In reply to Omega Feng [:Omega] [:馮於懋] (PTO until 8/22) from comment #34)
> > Hi Kartikaya,
> > In all the above cases, the bubble disappears when user starts scrolling and
> > appears after user ends scrolling, no matter the selected and the scrolled
> > elements are in different levels.
> 
> Sounds good, thanks.

That also clears up my concern about the scenario in comment 21 - since it doesn't matter which APZC sends the APZStateChange message, there is no problem.

    
    

    
  
Comment on attachment 8478168 [details] [diff] [review]
WIP v4

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

I still maintain that scrolling activity does *not* change the selection, so exposing this on the selectionchange event is not a good idea.

I think it's better to expose two new mozbrowser events: onpanzoomstart and onpanzoomend.  I think for now we can keep them as simple events, unless there is any need to expose any extra information on them.  We can still use the nsGlobalWindow::UpdateCommands machinery for this, I'm just objecting to tying this down to the selectionchange mozbrowser event.

::: layout/generic/nsSelection.cpp
@@ +5925,5 @@
> +  mScrollEndDetectorTimer->Cancel();
> +  mScrollEndDetectorTimer->InitWithFuncCallback(FireScrollEnd,
> +                                                this,
> +                                                kScrollEndTimerDelay,
> +                                                nsITimer::TYPE_ONE_SHOT);

Why do we need this timer?  We should fire the onpanzoomend event as soon as the pan/zoom is ended.  If this is required for something in the UX spec, then that needs to be implemented on top of the API we're working on here.

@@ +5965,5 @@
> +void
> +Selection::ScrollPositionChanged()
> +{
> +  if (!mAPZenabled) {
> +    mFrameSelection->PostReason(nsISelectionListener::SCROLLSTART_REASON);

We should not use these reason codes...

        Attachment #8478168 -
        Flags: review?(ehsan) → review-

    
    

    
  
Comment on attachment 8478168 [details] [diff] [review]
WIP v4

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

Now not only selection bubble wants to listen scroll activities but only touchcaret and selectioncarets want too. That's why I expose scroll activities to selectionListeners. Does it make sense for you? Or I can add NotifyScrollActivities in nsISelectionListener[1] and selection(nsSelection.cpp)/tochcaret/selectioncaret will register as listener to receive scroll activities callbacks. And then selection will call UpdateCommands with the action 'scrollactivitychanges' to show/hide selection bubble.

[1]http://dxr.mozilla.org/mozilla-central/source/content/base/public/nsISelectionListener.idl#23

::: layout/generic/nsSelection.cpp
@@ +5925,5 @@
> +  mScrollEndDetectorTimer->Cancel();
> +  mScrollEndDetectorTimer->InitWithFuncCallback(FireScrollEnd,
> +                                                this,
> +                                                kScrollEndTimerDelay,
> +                                                nsITimer::TYPE_ONE_SHOT);

I explained the reason to keep this timer at line 5958 because the reflow process may still run after asyncpanzoomend is received. If we send out notification soon, you may see the selection bubble with wrong position.

@@ +5966,5 @@
> +Selection::ScrollPositionChanged()
> +{
> +  if (!mAPZenabled) {
> +    mFrameSelection->PostReason(nsISelectionListener::SCROLLSTART_REASON);
> +    NotifySelectionListeners();

Here I also consider the scenario that APZ is off and the ScrollPositionChanged will trigger the mScrollEndDetectorTimer to detect the scroll end.

    
    

    
  
ehsan, how do you think about comment 40?
Flags: needinfo?(ehsan)

    
    

    
  
I've just updated the UX spec with some above comments. See bug 921965 for the spec.

    
    

    
  
I and Peter discussed this extensively on IRC, and I believe he has all that he needs for this bug from me.
Flags: needinfo?(ehsan)

    
    

    
  
Comment on attachment 8478168 [details] [diff] [review]
WIP v4

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

::: layout/generic/nsSelection.cpp
@@ +5958,5 @@
> +  // Don't send out the scroll end notification here because
> +  // it is possible that the reflow is still running, invoke
> +  // the ScrollEndDetector to delay the scroll end notification
> +
> +  LaunchScrollEndDetector();

kats, if I show the selection bubble after apz stopped callback, it may have chance to display at incorrect position. And it is possible to see one ScrollPositionChanged (triggered by APZCCallbackHelper::UpdateRootFrame) after the AsyncPanZoomStopped, like the following log.
About the visibility changes for selection bubble, it is changed by calling UpdateCommands with selectionchange[1].
http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#3573
[logs]
AsyncPanZoomStarted called
ScrollPositionChanged called
ScrollPositionChanged called
ScrollPositionChanged called
ScrollPositionChanged called
ScrollPositionChanged called
AsyncPanZoomStopped called
ScrollPositionChanged called

    
    

    
  
ni kats for comment 44.
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #44)
> kats, if I show the selection bubble after apz stopped callback, it may have
> chance to display at incorrect position. And it is possible to see one
> ScrollPositionChanged (triggered by APZCCallbackHelper::UpdateRootFrame)
> after the AsyncPanZoomStopped, like the following log.

This is expected. The APZ might not trigger the last repaint until after the pan/zoom operation stops, so the scroll position may change at that point. If as ehsan said that the selection bubble is absolutely-positioned relative to the document, then the actual scroll offset of the page shouldn't matter, and the bubble should never be mispositioned regardless of what the scroll offset is.

> About the visibility changes for selection bubble, it is changed by calling
> UpdateCommands with selectionchange[1].
> http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.
> cpp#3573

Where is the UpdateCommands function implemented? Can you point me to the actual code that sets the position of the bubble? Like the thing that actually sets the x and y coordinates of it. I don't know how this code works so it'll take me forever to dig through all the layers of abstraction.
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #46)
> (In reply to peter chang[:pchang][:peter] from comment #44)
> > kats, if I show the selection bubble after apz stopped callback, it may have
> > chance to display at incorrect position. And it is possible to see one
> > ScrollPositionChanged (triggered by APZCCallbackHelper::UpdateRootFrame)
> > after the AsyncPanZoomStopped, like the following log.
> 
> This is expected. The APZ might not trigger the last repaint until after the
> pan/zoom operation stops, so the scroll position may change at that point.
> If as ehsan said that the selection bubble is absolutely-positioned relative
> to the document, then the actual scroll offset of the page shouldn't matter,
> and the bubble should never be mispositioned regardless of what the scroll
> offset is.
> 
> > About the visibility changes for selection bubble, it is changed by calling
> > UpdateCommands with selectionchange[1].
> > http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.
> > cpp#3573
> 
> Where is the UpdateCommands function implemented? Can you point me to the
> actual code that sets the position of the bubble? Like the thing that
> actually sets the x and y coordinates of it. I don't know how this code
> works so it'll take me forever to dig through all the layers of abstraction.
[1] is the place to configure the position of the bubble by calling GetBoundingClientRect with flushLayout is false.

If we call it with flushLayout is true, it causes lots of try server error.

[1]http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#9303

    
    

    
  
Thanks. For posterity here is a better link to the code, which won't change over time: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp?rev=fd51c8a61561#9365

Also I think I understand why this is happening now. GetClientBoundingRect returns a rect that is relative to the scroll offset, and the scroll offset at that point has not yet been updated to the final scroll offset (because the APZ repaint has not yet been processed). Code later in the pipeline must be adding the scroll offset back into this value before it sets the position:absolute properties of the actual bubble element. If we change this code to add the scroll offsets here and then dispatch the event with the absolute-position coordinates the problem should go away.

    
    

    
  
That makes a lot of sense, kats!  Thanks for lending a hand.  :-)

    
    

    
  
(In reply to (away|aug30-sep3) Kartikaya Gupta (email:kats@mozilla.com) from comment #48)
> Thanks. For posterity here is a better link to the code, which won't change
> over time:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp?rev=fd51c8a61561#9365
> 
> Also I think I understand why this is happening now. GetClientBoundingRect
> returns a rect that is relative to the scroll offset, and the scroll offset
> at that point has not yet been updated to the final scroll offset (because
> the APZ repaint has not yet been processed). Code later in the pipeline must
> be adding the scroll offset back into this value before it sets the
> position:absolute properties of the actual bubble element. If we change this
> code to add the scroll offsets here and then dispatch the event with the
> absolute-position coordinates the problem should go away.

This information is very useful to me. I will create a follow up bug to align the final scroll offset for the selection bubble.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v5 (obsolete)
        — 
          — Splinter Review
      

    


  
The version includes
a. Break the dependency between scroll activities and selection changes
b. Listen the APZ transfrom start/stop callback for selection carets.  
b. Add two new mozbrowser events, scrollviewchangestart and scrollviewchangestop which could be triggered no matter APZ is enabled or not.

        Attachment #8478168 -
        Attachment is obsolete: true

        Attachment #8481290 -
        Flags: review?(ehsan.akhgari)

    
    

    
  
Comment on attachment 8481290 [details] [diff] [review]
WIP v5

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +657,5 @@
> +    if (e.type == 'mozscrollviewchangestart') {
> +      detail.reasons.push('scrollstart');
> +    } else if (e.type == 'mozscrollviewchangestop') {
> +      detail.reasons.push('scrollstop');
> +    }

You are _still_ generating a selectionchange event as opposed to what comment 51 says.

If you are not sure what I would like us to achieve here, please ping me on IRC.  I think that would be more productive than you trying the same approach over and over and getting r-.  :-)

@@ +841,5 @@
>      if (win != content) {
>        return;
>      }
>  
> +    dump('scroll event x ' + win.scrollX + ' y ' + win.scrollY);

You don't want to dump this unconditionally.  I assume this is a debugging facility.

::: layout/base/SelectionCarets.cpp
@@ +848,5 @@
> +    nsCOMPtr<nsIDocument> doc = mPresShell->GetDocument();
> +    if (doc) {
> +      nsPIDOMWindow* domWindow = mPresShell->GetDocument()->GetWindow();
> +      if (domWindow) {
> +        domWindow->UpdateCommands(NS_LITERAL_STRING("scrollviewchangestart"), GetSelection(), 0);

What's the point of going through UpdateCommands here?  You should be able to dispatch the mozscrollviewchangestart/stop events from here directly.

::: layout/base/SelectionCarets.h
@@ +206,5 @@
>    // For filter multitouch event
>    int32_t mActiveTouchId;
>  
> +  // True if AsyncPanZoom is enabled
> +  bool mAPZenabled;

Nit: please move this down next to the rest of the bool members for better packing.

@@ +215,3 @@
>    bool mEndCaretVisible;
> +  bool mStartCaretVisible;
> +  bool mVisible;

Not sure why you're reordering things... (but it doesn't matter!)

        Attachment #8481290 -
        Flags: review?(ehsan.akhgari) → review-

    
    

    
  
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #52)
> Comment on attachment 8481290 [details] [diff] [review]
> WIP v5
> 
> Review of attachment 8481290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +657,5 @@
> > +    if (e.type == 'mozscrollviewchangestart') {
> > +      detail.reasons.push('scrollstart');
> > +    } else if (e.type == 'mozscrollviewchangestop') {
> > +      detail.reasons.push('scrollstop');
> > +    }
> 
> You are _still_ generating a selectionchange event as opposed to what
> comment 51 says.
> 
> If you are not sure what I would like us to achieve here, please ping me on
> IRC.  I think that would be more productive than you trying the same
> approach over and over and getting r-.  :-)
> 
Sure, let's discuss on IRC first. It is possible that gaia not only needs the scroll activities but also the scroll offset. That's why I reuse the selectionchange event here. But I think I could pass the scroll offset with the mozscrollviewchangestop to gaia to display the bubble correctly.

> @@ +841,5 @@
> >      if (win != content) {
> >        return;
> >      }
> >  
> > +    dump('scroll event x ' + win.scrollX + ' y ' + win.scrollY);
> 
> You don't want to dump this unconditionally.  I assume this is a debugging
> facility.
> 
Will remove it.
> ::: layout/base/SelectionCarets.cpp
> @@ +848,5 @@
> > +    nsCOMPtr<nsIDocument> doc = mPresShell->GetDocument();
> > +    if (doc) {
> > +      nsPIDOMWindow* domWindow = mPresShell->GetDocument()->GetWindow();
> > +      if (domWindow) {
> > +        domWindow->UpdateCommands(NS_LITERAL_STRING("scrollviewchangestart"), GetSelection(), 0);
> 
> What's the point of going through UpdateCommands here?  You should be able
> to dispatch the mozscrollviewchangestart/stop events from here directly.
> 
> ::: layout/base/SelectionCarets.h
> @@ +206,5 @@
> >    // For filter multitouch event
> >    int32_t mActiveTouchId;
> >  
> > +  // True if AsyncPanZoom is enabled
> > +  bool mAPZenabled;
> 
> Nit: please move this down next to the rest of the bool members for better
> packing.
> 
> @@ +215,3 @@
> >    bool mEndCaretVisible;
> > +  bool mStartCaretVisible;
> > +  bool mVisible;
> 
> Not sure why you're reordering things... (but it doesn't matter!)
I just consider the Alphabetical Order...do I need to change it?

    
    

    
  
Looks like we also need to pass the zoom factor/scroll offset to gaia to display the bubble.

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #53)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #52)
> > Comment on attachment 8481290 [details] [diff] [review]
> > WIP v5
> > 
> > Review of attachment 8481290 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/browser-element/BrowserElementChildPreload.js
> > @@ +657,5 @@
> > > +    if (e.type == 'mozscrollviewchangestart') {
> > > +      detail.reasons.push('scrollstart');
> > > +    } else if (e.type == 'mozscrollviewchangestop') {
> > > +      detail.reasons.push('scrollstop');
> > > +    }
> > 
> > You are _still_ generating a selectionchange event as opposed to what
> > comment 51 says.
> > 
> > If you are not sure what I would like us to achieve here, please ping me on
> > IRC.  I think that would be more productive than you trying the same
> > approach over and over and getting r-.  :-)
> > 
> Sure, let's discuss on IRC first. It is possible that gaia not only needs
> the scroll activities but also the scroll offset. That's why I reuse the
> selectionchange event here. But I think I could pass the scroll offset with
> the mozscrollviewchangestop to gaia to display the bubble correctly.

Why can't you get the scroll offsets from the mozbrowserasyncscroll event?

> > @@ +215,3 @@
> > >    bool mEndCaretVisible;
> > > +  bool mStartCaretVisible;
> > > +  bool mVisible;
> > 
> > Not sure why you're reordering things... (but it doesn't matter!)
> I just consider the Alphabetical Order...do I need to change it?

It's fine either way!

(In reply to peter chang[:pchang][:peter] from comment #54)
> Looks like we also need to pass the zoom factor/scroll offset to gaia to
> display the bubble.

Attaching the zoom factor to the scrollviewchangestart/end events makes sense.  I'd also be OK with adding the scroll offsets there, but I'd like to know why mozbrowserasyncscroll would not work.  But note that we don't want to dispatch a selectionchange event here at any rate, since the selection is not changing.  :-)

    
  
Priority: -- → P1

    
    

    
  
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #55)
> Why can't you get the scroll offsets from the mozbrowserasyncscroll event?

Please avoid using the mozbrowserasyncscroll event if at all possible. We're trying to get rid of it (bug 898075) because it is misleading (i.e. it is dispatched to the main thread and therefore by definition stale because the user can continue scrolling async). The regular scroll event is fired just as frequently and should have the same information so please use that instead. If there's something the mozbrowserasyncscroll lets you do that you can't do with regular scroll events please let me know.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #55)
> > Why can't you get the scroll offsets from the mozbrowserasyncscroll event?
> 
> Please avoid using the mozbrowserasyncscroll event if at all possible. We're
> trying to get rid of it (bug 898075) because it is misleading (i.e. it is
> dispatched to the main thread and therefore by definition stale because the
> user can continue scrolling async). The regular scroll event is fired just
> as frequently and should have the same information so please use that
> instead. If there's something the mozbrowserasyncscroll lets you do that you
> can't do with regular scroll events please let me know.

Which regular scroll event do you mean?
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
The one that triggers this:

window.addEventListener("scroll", function(e) { console.log(e) }, true);
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #58)
> The one that triggers this:
> 
> window.addEventListener("scroll", function(e) { console.log(e) }, true);

But that is not avaialable through the mozbrowser API, is it?  I'm not sure what I'm missing here...
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
No, it isn't available as part of the mozbrowser API. I didn't realize you were restricted to using the mozbrowser API for this - I was assuming the listener would be added somewhere in C++ code that had access to the scroll event. Never mind then.
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
kats/ehsan, How about get the scroll offset from FrameMetrics in TabChild[1] and then pass the scroll offset through AsyncPanZoomStarted/Stopped callback[2]?

[1]http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=db7212847c14#1861
[2]http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=db7212847c14#1987

And then I will dispatch a new DOM event scrollviewchange with scroll offset through mozbrowser API.
Flags: needinfo?(ehsan.akhgari)
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #61)
> kats/ehsan, How about get the scroll offset from FrameMetrics in TabChild[1]
> and then pass the scroll offset through AsyncPanZoomStarted/Stopped
> callback[2]?
> 
> [1]http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.
> cpp?rev=db7212847c14#1861
> [2]http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.
> cpp?rev=db7212847c14#1987

Kats can answer this question better.

> And then I will dispatch a new DOM event scrollviewchange with scroll offset
> through mozbrowser API.

As long as you don't expose that event to normal web content, this part sounds fine.
Flags: needinfo?(ehsan.akhgari)

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #61)
> kats/ehsan, How about get the scroll offset from FrameMetrics in TabChild[1]
> and then pass the scroll offset through AsyncPanZoomStarted/Stopped
> callback[2]?

Which scroll offset do you need? If you need the one for the subframe that is being scrolled you can just get that from sf->GetScrollPositionCSSPixels() at [1]. Or if you need the root window's scroll position you can call GetDOMWindowUtils()->GetScrollXY and get it from that.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=db7212847c14#1987
Flags: needinfo?(bugmail.mozilla)

    
    

    
  
Also just to state it explicitly I would prefer if you didn't pull it out of the FrameMetrics and keep yet another copy because it seems unnecessary to do that.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #63)
> (In reply to peter chang[:pchang][:peter] from comment #61)
> > kats/ehsan, How about get the scroll offset from FrameMetrics in TabChild[1]
> > and then pass the scroll offset through AsyncPanZoomStarted/Stopped
> > callback[2]?
> 
> Which scroll offset do you need? If you need the one for the subframe that
> is being scrolled you can just get that from
> sf->GetScrollPositionCSSPixels() at [1]. Or if you need the root window's
> scroll position you can call GetDOMWindowUtils()->GetScrollXY and get it
> from that.
> 
I think the scroll offset for the subframe is what I want.
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.
> cpp?rev=db7212847c14#1987

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #64)
> Also just to state it explicitly I would prefer if you didn't pull it out of
> the FrameMetrics and keep yet another copy because it seems unnecessary to
> do that.
Sure.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v6 (obsolete)
        — 
          — Splinter Review
      

    


  
Attach WIP v6 that includes
a. add new mozbrowserscrollveiwchange event and pass scroll offset to gaia
b. Fix the alphabetical order/compiler warning

        Attachment #8481290 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8487815 [details] [diff] [review]
WIP v6

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

::: dom/webidl/BrowserElementDictionaries.webidl
@@ +33,5 @@
> +dictionary ScrollViewChangeEventInit : EventInit {
> +  ScrollState state = "";
> +  float scrollWidth = 0;
> +  float scrollHeight = 0;
> +};

Since this event will be used by BrowerElementChildPreload.js, therefore, I put its event detail here.

        Attachment #8487815 -
        Flags: review?(ehsan.akhgari)

    
    

    
  
Comment on attachment 8487815 [details] [diff] [review]
WIP v6

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

::: b2g/chrome/content/shell.js
@@ +331,5 @@
>      window.addEventListener('sizemodechange', this);
>      window.addEventListener('unload', this);
>      this.contentBrowser.addEventListener('mozbrowserloadstart', this, true);
>      this.contentBrowser.addEventListener('mozbrowserselectionchange', this, true);
> +    this.contentBrowser.addEventListener('mozbrowserscrollviewchange', this, true);

Fabrice should review these bits when you're ready (and probably the dom/browser-element bits too!)

::: dom/ipc/TabChild.cpp
@@ +1989,3 @@
>          nsDocShell* nsdocshell = static_cast<nsDocShell*>(docshell.get());
> +        nsdocshell->NotifyAsyncPanZoomStarted(sf->GetScrollPositionCSSPixels().x,
> +                                              sf->GetScrollPositionCSSPixels().y);

Please check these bits with kats, to make sure these are the correct dimensions.

::: dom/webidl/BrowserElementDictionaries.webidl
@@ +27,5 @@
>    long width = 0;
>    long height = 0;
>  };
> +
> +enum ScrollState {"", "started", "stopped"};

Where is "" used?

@@ +33,5 @@
> +dictionary ScrollViewChangeEventInit : EventInit {
> +  ScrollState state = "";
> +  float scrollWidth = 0;
> +  float scrollHeight = 0;
> +};

Hmm, why not define the event interface itself in WebIDL too?

::: layout/base/SelectionCarets.cpp
@@ +885,5 @@
>  
> +static void
> +DispatchScrollViewChangeEvent(nsIPresShell *aPresShell, const dom::ScrollState aState, int aScrollWidth, int aScrollHeight)
> +{
> +  if (!aPresShell) {

How can this happen?

@@ +891,5 @@
> +  }
> +
> +  nsCOMPtr<nsIDocument> doc = aPresShell->GetDocument();
> +  if (doc) {
> +    nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(doc);

Please remove this, as you're not using domDoc.

@@ +894,5 @@
> +  if (doc) {
> +    nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(doc);
> +    ErrorResult res;
> +    nsRefPtr<Event> event =
> +      doc->CreateEvent(NS_LITERAL_STRING("CustomEvent"), res);

This should be a proper event object.

@@ +940,1 @@
>    SetVisibility(false);

We shouldn't change the visibility of the selection carets when scrolling begins/ends in Gecko.  This is Gaia's responsibility (and is the reason why we're exposing these events!)

@@ +943,5 @@
> +
> +void
> +SelectionCarets::AsyncPanZoomStopped(int aScrollWidth, int aScrollHeight)
> +{
> +  SetVisibility(true);

Ditto.

@@ +951,5 @@
> +void
> +SelectionCarets::ScrollPositionChanged()
> +{
> +  if (!mAPZenabled) {
> +    SetVisibility(false);

Ditto.

::: layout/base/SelectionCarets.h
@@ +206,5 @@
>    // For filter multitouch event
>    int32_t mActiveTouchId;
>  
> +  // True if AsyncPanZoom is enabled
> +  bool mAPZenabled;

Please move all of the bool members together.

        Attachment #8487815 -
        Flags: review?(ehsan.akhgari) → review-

    
    

    
  
Comment on attachment 8487815 [details] [diff] [review]
WIP v6

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

::: b2g/chrome/content/shell.js
@@ +331,5 @@
>      window.addEventListener('sizemodechange', this);
>      window.addEventListener('unload', this);
>      this.contentBrowser.addEventListener('mozbrowserloadstart', this, true);
>      this.contentBrowser.addEventListener('mozbrowserselectionchange', this, true);
> +    this.contentBrowser.addEventListener('mozbrowserscrollviewchange', this, true);

Sure, I will include him next time.

::: dom/ipc/TabChild.cpp
@@ +1989,3 @@
>          nsDocShell* nsdocshell = static_cast<nsDocShell*>(docshell.get());
> +        nsdocshell->NotifyAsyncPanZoomStarted(sf->GetScrollPositionCSSPixels().x,
> +                                              sf->GetScrollPositionCSSPixels().y);

Now I'm checking with gaia to confirm the offset is enough for them to handle selection bubble.

::: dom/webidl/BrowserElementDictionaries.webidl
@@ +27,5 @@
>    long width = 0;
>    long height = 0;
>  };
> +
> +enum ScrollState {"", "started", "stopped"};

It is used to initialize the ScrollState but I should declare ScrollState as the following way.
ScrollState? state = null;

@@ +33,5 @@
> +dictionary ScrollViewChangeEventInit : EventInit {
> +  ScrollState state = "";
> +  float scrollWidth = 0;
> +  float scrollHeight = 0;
> +};

Do we need to have a separate file called ScrollViewChangeEvent.webidl if I define a new event interface?

::: layout/base/SelectionCarets.cpp
@@ +885,5 @@
>  
> +static void
> +DispatchScrollViewChangeEvent(nsIPresShell *aPresShell, const dom::ScrollState aState, int aScrollWidth, int aScrollHeight)
> +{
> +  if (!aPresShell) {

Just error checking, I think it is safe to remove it.

@@ +891,5 @@
> +  }
> +
> +  nsCOMPtr<nsIDocument> doc = aPresShell->GetDocument();
> +  if (doc) {
> +    nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(doc);

Sure, will do

@@ +940,1 @@
>    SetVisibility(false);

As we discussed before, gaia couldn't change the visibility of selection carets because it is generated by gecko. Inside SelectionCarets[1], it also does the same thing.

[1]http://dxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?from=SelectionCarets.cpp#469

::: layout/base/SelectionCarets.h
@@ +206,5 @@
>    // For filter multitouch event
>    int32_t mActiveTouchId;
>  
> +  // True if AsyncPanZoom is enabled
> +  bool mAPZenabled;

Sure.

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #69)
> ::: dom/webidl/BrowserElementDictionaries.webidl
> @@ +27,5 @@
> >    long width = 0;
> >    long height = 0;
> >  };
> > +
> > +enum ScrollState {"", "started", "stopped"};
> 
> It is used to initialize the ScrollState but I should declare ScrollState as
> the following way.
> ScrollState? state = null;

OK.  But why not default initialize to "started"?

> @@ +33,5 @@
> > +dictionary ScrollViewChangeEventInit : EventInit {
> > +  ScrollState state = "";
> > +  float scrollWidth = 0;
> > +  float scrollHeight = 0;
> > +};
> 
> Do we need to have a separate file called ScrollViewChangeEvent.webidl if I
> define a new event interface?

Yes please.

> ::: layout/base/SelectionCarets.cpp
> @@ +885,5 @@
> >  
> > +static void
> > +DispatchScrollViewChangeEvent(nsIPresShell *aPresShell, const dom::ScrollState aState, int aScrollWidth, int aScrollHeight)
> > +{
> > +  if (!aPresShell) {
> 
> Just error checking, I think it is safe to remove it.

Yeah, I can't think of how this code path will be taken.

> @@ +940,1 @@
> >    SetVisibility(false);
> 
> As we discussed before, gaia couldn't change the visibility of selection
> carets because it is generated by gecko. Inside SelectionCarets[1], it also
> does the same thing.
> 
> [1]http://dxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.
> cpp?from=SelectionCarets.cpp#469

If this is to be done within Gecko, what's the point of exposing this event?
Flags: needinfo?(pchang)

    
    

    
  
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #70)
> (In reply to peter chang[:pchang][:peter] from comment #69)
> > ::: dom/webidl/BrowserElementDictionaries.webidl
> > @@ +27,5 @@
> > >    long width = 0;
> > >    long height = 0;
> > >  };
> > > +
> > > +enum ScrollState {"", "started", "stopped"};
> > 
> > It is used to initialize the ScrollState but I should declare ScrollState as
> > the following way.
> > ScrollState? state = null;
> 
> OK.  But why not default initialize to "started"?
> 
> > @@ +33,5 @@
> > > +dictionary ScrollViewChangeEventInit : EventInit {
> > > +  ScrollState state = "";
> > > +  float scrollWidth = 0;
> > > +  float scrollHeight = 0;
> > > +};
> > 
> > Do we need to have a separate file called ScrollViewChangeEvent.webidl if I
> > define a new event interface?
> 
> Yes please.
> 
> > ::: layout/base/SelectionCarets.cpp
> > @@ +885,5 @@
> > >  
> > > +static void
> > > +DispatchScrollViewChangeEvent(nsIPresShell *aPresShell, const dom::ScrollState aState, int aScrollWidth, int aScrollHeight)
> > > +{
> > > +  if (!aPresShell) {
> > 
> > Just error checking, I think it is safe to remove it.
> 
> Yeah, I can't think of how this code path will be taken.
> 
> > @@ +940,1 @@
> > >    SetVisibility(false);
> > 
> > As we discussed before, gaia couldn't change the visibility of selection
> > carets because it is generated by gecko. Inside SelectionCarets[1], it also
> > does the same thing.
> > 
> > [1]http://dxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.
> > cpp?from=SelectionCarets.cpp#469
> 
> If this is to be done within Gecko, what's the point of exposing this event?
Do you mean the "ScrollViewChange" event?
For selection carets, it is created and controlled by gecko.
For selection bubbles, it is controlled by system app(gaia).

During the scrolling, we need to hide the selection caret and also selection bubble. Therefore, we need to send "ScrollViewChange" event to gaia to show/hide the selection bubble. Does it make sense to you?
Flags: needinfo?(pchang)

    
  
Flags: needinfo?(ehsan.akhgari)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v6 (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v7 part 1 (obsolete)
        — 
          — Splinter Review
      

    


  
Add new DOM event 'ScrollViewChange' and pass this event from content process to system APP through mozbrowser interface

        Attachment #8487815 -
        Attachment is obsolete: true

        Attachment #8489887 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v7 part 2 (obsolete)
        — 
          — Splinter Review
      

    


  
- Register async pan zoom start/stop callback in SelectionCaret
- Send out new DOM event 'ScrolViewChange' events to system app during scrolling

    
    

    
  
Comment on attachment 8489890 [details] [diff] [review]
WIP v7 part 2

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

::: dom/ipc/TabChild.cpp
@@ +2002,3 @@
>          nsDocShell* nsdocshell = static_cast<nsDocShell*>(docshell.get());
> +        nsdocshell->NotifyAsyncPanZoomStarted(sf->GetScrollPositionCSSPixels().x,
> +                                              sf->GetScrollPositionCSSPixels().y);

kats, I think we need the scroll offset from subframe here. During the integration with gaia, they could calculate correct position of selection bubble based on this scroll offset. Let me know if you have any concern or suggestion.

        Attachment #8489890 -
        Flags: feedback?(bugmail.mozilla)

    
  

        Attachment #8489889 -
        Flags: review?(fabrice)

        Attachment #8489889 -
        Flags: review?(ehsan.akhgari)

    
  

        Attachment #8489890 -
        Flags: review?(ehsan.akhgari)

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #75)
> Comment on attachment 8489890 [details] [diff] [review]
> WIP v7 part 2
> 
> Review of attachment 8489890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabChild.cpp
> @@ +2002,3 @@
> >          nsDocShell* nsdocshell = static_cast<nsDocShell*>(docshell.get());
> > +        nsdocshell->NotifyAsyncPanZoomStarted(sf->GetScrollPositionCSSPixels().x,
> > +                                              sf->GetScrollPositionCSSPixels().y);
> 
> kats, I think we need the scroll offset from subframe here. During the
> integration with gaia, they could calculate correct position of selection
> bubble based on this scroll offset. Let me know if you have any concern or
> suggestion.

That code looks fine then. However what I would suggest is instead of passing the two int separately to nsDocShell, change that argument to a CSSIntPoint. Try to keep it as a CSSIntPoint for as long as possible; which I think should be all the way until it gets to DispatchScrollViewChangeEvent. When populating the detail object there you can use the x and y integers. Also, the names "scrollWidth" and "scrollHeight" are misleading, they should be scrollX and scrollY.

    
    

    
  
Comment on attachment 8489890 [details] [diff] [review]
WIP v7 part 2

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

f+ with previous comments.

        Attachment #8489890 -
        Flags: feedback?(bugmail.mozilla) → feedback+

    
    

    
  
Comment on attachment 8489889 [details] [diff] [review]
WIP v7 part 1

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

The WebIDL changes look good, but you need a DOM peer review for those, and the rest should be reviewed by Fabrice.

        Attachment #8489889 -
        Flags: review?(ehsan.akhgari)

    
    

    
  
Comment on attachment 8489890 [details] [diff] [review]
WIP v7 part 2

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

The problem that I have with this approach is that we're allowing the Firefox OS UX specs to dictate how Gecko behaves.  That is very unfortunate, and I _still_ believe that it's much better to move application specific logic like this to gaia, but it seems like we're going in circles over the same argument.  So I'm going to give up.  :(

But please file and fix a follow-up bug to add a pref to disable toggling the visibility of the selection and touch carets inside Gecko.  This behavior is undesirable for Fennec, for example, so this should be a Firefox OS specific hack for now, hidden behind a pref that we only turn on for b2g.

r=me with the follow-up bug filed, and with at least a promise to write a patch for it. :)

::: layout/base/SelectionCarets.cpp
@@ +21,5 @@
>  #include "nsIPresShell.h"
>  #include "nsPresContext.h"
>  #include "nsRect.h"
>  #include "nsView.h"
> +#include "mozilla/dom/CustomEvent.h"

You don't need this.

@@ +29,2 @@
>  #include "mozilla/dom/Selection.h"
> +#include "mozilla/dom/ToJSValue.h"

I don't think this is needed either.

        Attachment #8489890 -
        Flags: review?(ehsan.akhgari) → review+

    
  
Flags: needinfo?(ehsan.akhgari)

    
    

    
  
Comment on attachment 8489889 [details] [diff] [review]
WIP v7 part 1

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

r=me with nits fixed. You'll need a DOM peer to r+ the webidl to be able to land.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +631,5 @@
>    },
>  
> +  _ScrollViewChangeHandler: function(e) {
> +    e.stopPropagation();
> +    sendAsyncMsg("scrollviewchange", e.detail);

nit: single quotes in this file.

::: dom/browser-element/BrowserElementParent.jsm
@@ +500,5 @@
>    },
>  
> +  _handleScrollViewChange: function(data) {
> +    let evt;
> +    evt = this._createEvent('scrollviewchange', data.json,

let evt = this._createEvent(...)
nit: double quotes in this file ;)

        Attachment #8489889 -
        Flags: review?(fabrice) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v8 part 1 (obsolete)
        — 
          — Splinter Review
      

    


  
Address pervious comment and ask DOM peer review

        Attachment #8489889 -
        Attachment is obsolete: true

        Attachment #8492828 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v8 part 2 (obsolete)
        — 
          — Splinter Review
      

    


  
Address previous reviewer's comment

        Attachment #8489890 -
        Attachment is obsolete: true

        Attachment #8492829 -
        Flags: review+

        Attachment #8492829 -
        Flags: feedback+

    
    

    
  
Comment on attachment 8492828 [details] [diff] [review]
WIP v8 part 1

r=me, though you may want to fix the indentation issue in BrowserElementParent.jsm

        Attachment #8492828 -
        Flags: review?(bzbarsky) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v9 part 1
        — 
          — Splinter Review
      

    


  
Fix the indentation in BrowserElementParent.jsm

        Attachment #8492828 -
        Attachment is obsolete: true

        Attachment #8492970 -
        Flags: review+

    
    

    
  
Wait for try server result.
https://tbpl.mozilla.org/?tree=Try&rev=be4abe71a5bd

    
    

    
  
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #79)
> Comment on attachment 8489890 [details] [diff] [review]
> WIP v7 part 2
> 
> Review of attachment 8489890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The problem that I have with this approach is that we're allowing the
> Firefox OS UX specs to dictate how Gecko behaves.  That is very unfortunate,
Since the latest patch already exposes the scroll event to gaia, therefore, I assume you are talking about the logic to change the visibility of selection carets inside gecko.
  I think there are at least two problems if we move touch/selection carets to gaia.
a. Need to insert caret into the DOM tree of app, I think user won't expect this behavior
b. Need to expose many events to gaia, like the scrolling, touch events and orientation changes. And it will also have the synchronization and performance issue(bug 1066515) I guess. May I know your concern about involving UX specs inside gecko?

> and I _still_ believe that it's much better to move application specific
> logic like this to gaia, but it seems like we're going in circles over the
> same argument.  So I'm going to give up.  :(


> 
> But please file and fix a follow-up bug to add a pref to disable toggling
> the visibility of the selection and touch carets inside Gecko.  This
> behavior is undesirable for Fennec, for example, so this should be a Firefox
> OS specific hack for now, hidden behind a pref that we only turn on for b2g.
> 
Ehsan, current gecko already has the preferences to enable/disable touch caret[1] and selection carets[2]. Both are only enabled on B2G. I assume this is what you want, am I right?

[1]http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=ae2e4ca015fd#883
[2]http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=ae2e4ca015fd#888

> r=me with the follow-up bug filed, and with at least a promise to write a
> patch for it. :)

    
  
Flags: needinfo?(ehsan.akhgari)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP v9 part 2
        — 
          — Splinter Review
      

    


  
Fix the build error in linux x64 debug.

        Attachment #8492829 -
        Attachment is obsolete: true

        Attachment #8493731 -
        Flags: review+

        Attachment #8493731 -
        Flags: feedback+

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #85)
> Wait for try server result.
> https://tbpl.mozilla.org/?tree=Try&rev=be4abe71a5bd
All items are passed but got build errors for linux debug and linux x64 debug.

Fix build error in attachment 8493731 [details] [diff] [review] and the following is the try result for new patch.
https://tbpl.mozilla.org/?tree=Try&rev=7b3f76c04f05

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #86)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #79)
> > Comment on attachment 8489890 [details] [diff] [review]
> > WIP v7 part 2
> > 
> > Review of attachment 8489890 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The problem that I have with this approach is that we're allowing the
> > Firefox OS UX specs to dictate how Gecko behaves.  That is very unfortunate,
> Since the latest patch already exposes the scroll event to gaia, therefore,
> I assume you are talking about the logic to change the visibility of
> selection carets inside gecko.
>   I think there are at least two problems if we move touch/selection carets
> to gaia.
> a. Need to insert caret into the DOM tree of app, I think user won't expect
> this behavior

I don't understand what you mean.  We already inject these carets into the DOM tree of the app as a native anonymous node which is invisible to the script running in the page.

> b. Need to expose many events to gaia, like the scrolling, touch events and
> orientation changes. And it will also have the synchronization and
> performance issue(bug 1066515) I guess. May I know your concern about
> involving UX specs inside gecko?

Yeah, there may definitely be performance problems initially, but we may be able to address them.

The issue is that the Firefox OS UX specs are specific to Firefox OS (and some of them to Firefox OS releases, i.e., they may change across versions) and Gecko is not specific to Firefox OS.  Therefore, anywhere that we make Gecko only do what the UX specs require, we're introducing problems for sharing the code with other consumers such as Fennec, which may want a different behavior than what we want for Firefox OS.  Therefore, at least we should make sure that we add hooks that allow us to enable the Firefox OS specific behavior in Gecko only on Firefox OS and not elsewhere.

> > But please file and fix a follow-up bug to add a pref to disable toggling
> > the visibility of the selection and touch carets inside Gecko.  This
> > behavior is undesirable for Fennec, for example, so this should be a Firefox
> > OS specific hack for now, hidden behind a pref that we only turn on for b2g.
> > 
> Ehsan, current gecko already has the preferences to enable/disable touch
> caret[1] and selection carets[2]. Both are only enabled on B2G. I assume
> this is what you want, am I right?

No.  We want to share this code with Fennec and get rid of the current implementation of the selection carets that Fennec has.  The prefs you are talking about are useful to disable this functionality on products where it doesn't make sense (such as Firefox Desktop), but right now merely enabling them will give you the hiding behavior in the Firefox OS UX specs, which is not what Fennec wants to do.  I want to add two prefs, controlling whether this hiding logic happens, make their default value false, and turn them on only on b2g, so that when Fennec turns on the touch caret and selection caret prefs, they don't get the Firefox OS specific bits of the behavior.

I hope this helps clarify what I would like us to achieve here.  If not, please feel free to ask more questions.  :-)
Flags: needinfo?(ehsan.akhgari)

    
    

    
  
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #89)
> (In reply to peter chang[:pchang][:peter] from comment #86)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #79)
> > > Comment on attachment 8489890 [details] [diff] [review]
> > > WIP v7 part 2
> > > 
> > > Review of attachment 8489890 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > The problem that I have with this approach is that we're allowing the
> > > Firefox OS UX specs to dictate how Gecko behaves.  That is very unfortunate,
> > Since the latest patch already exposes the scroll event to gaia, therefore,
> > I assume you are talking about the logic to change the visibility of
> > selection carets inside gecko.
> >   I think there are at least two problems if we move touch/selection carets
> > to gaia.
> > a. Need to insert caret into the DOM tree of app, I think user won't expect
> > this behavior
> 
> I don't understand what you mean.  We already inject these carets into the
> DOM tree of the app as a native anonymous node which is invisible to the
> script running in the page.
> 
If you implement carets in gaia, they will be visible in the DOM tree of the app. Am I right? Not sure this behavior will cause problem or not.

> > b. Need to expose many events to gaia, like the scrolling, touch events and
> > orientation changes. And it will also have the synchronization and
> > performance issue(bug 1066515) I guess. May I know your concern about
> > involving UX specs inside gecko?
> 
> Yeah, there may definitely be performance problems initially, but we may be
> able to address them.
> 
> The issue is that the Firefox OS UX specs are specific to Firefox OS (and
> some of them to Firefox OS releases, i.e., they may change across versions)
> and Gecko is not specific to Firefox OS.  Therefore, anywhere that we make
> Gecko only do what the UX specs require, we're introducing problems for
> sharing the code with other consumers such as Fennec, which may want a
> different behavior than what we want for Firefox OS.  Therefore, at least we
> should make sure that we add hooks that allow us to enable the Firefox OS
> specific behavior in Gecko only on Firefox OS and not elsewhere.
> 
> > > But please file and fix a follow-up bug to add a pref to disable toggling
> > > the visibility of the selection and touch carets inside Gecko.  This
> > > behavior is undesirable for Fennec, for example, so this should be a Firefox
> > > OS specific hack for now, hidden behind a pref that we only turn on for b2g.
> > > 
> > Ehsan, current gecko already has the preferences to enable/disable touch
> > caret[1] and selection carets[2]. Both are only enabled on B2G. I assume
> > this is what you want, am I right?
> 
> No.  We want to share this code with Fennec and get rid of the current
> implementation of the selection carets that Fennec has.  The prefs you are
> talking about are useful to disable this functionality on products where it
> doesn't make sense (such as Firefox Desktop), but right now merely enabling
> them will give you the hiding behavior in the Firefox OS UX specs, which is
> not what Fennec wants to do.  I want to add two prefs, controlling whether
> this hiding logic happens, make their default value false, and turn them on
> only on b2g, so that when Fennec turns on the touch caret and selection
> caret prefs, they don't get the Firefox OS specific bits of the behavior.
> 
> I hope this helps clarify what I would like us to achieve here.  If not,
> please feel free to ask more questions.  :-)
Thanks for above detail information, I understood your concern now. As you mentioned, we implement touch/selection carets in gecko and also involves the UX logic inside carets. Instead of adding prefs, I would prefer to align the UX between Fennec and FxOS. If we have solid UX spec for touch/seleciton carets, then we could provide the consistent behavior for multiple produces(FxOS, Fennec). How do you think? Just for your information, I also feedback the different UX behavior to FxOS UX.

    
  
Keywords: checkin-needed

    
    

    
  
Can we please use more descriptive commit messages for these two patches? Having identical commit messages modulo "Part 1/2" doesn't meet the guidelines.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

You can just suggest new commit messages in a comment here. No need to attach new patches just for that.
Flags: needinfo?(pchang)

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #91)
> Can we please use more descriptive commit messages for these two patches?
> Having identical commit messages modulo "Part 1/2" doesn't meet the
> guidelines.
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Committing_Rules_and_Responsibilities#Checkin_comment
> 
> You can just suggest new commit messages in a comment here. No need to
> attach new patches just for that.

Thanks for reminder. Please help to update commit messages as the following.

Part 1 ==> Bug 1020801, add new DOM event 'ScrollViewChangeEvent', r=fabrice,bz
Part 2 ==> Bug 1020801, notify the ScrollViewChange DOM event when APZ starts/stops to change the transform, r=ehsan
Flags: needinfo?(pchang)

    
  
Keywords: checkin-needed

    
    

    
  
ni ehsan for comment 90.
Flags: needinfo?(ehsan.akhgari)

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #90)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #89)
> > (In reply to peter chang[:pchang][:peter] from comment #86)
> > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > comment #79)
> > > > Comment on attachment 8489890 [details] [diff] [review]
> > > > WIP v7 part 2
> > > > 
> > > > Review of attachment 8489890 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > The problem that I have with this approach is that we're allowing the
> > > > Firefox OS UX specs to dictate how Gecko behaves.  That is very unfortunate,
> > > Since the latest patch already exposes the scroll event to gaia, therefore,
> > > I assume you are talking about the logic to change the visibility of
> > > selection carets inside gecko.
> > >   I think there are at least two problems if we move touch/selection carets
> > > to gaia.
> > > a. Need to insert caret into the DOM tree of app, I think user won't expect
> > > this behavior
> > 
> > I don't understand what you mean.  We already inject these carets into the
> > DOM tree of the app as a native anonymous node which is invisible to the
> > script running in the page.
> > 
> If you implement carets in gaia, they will be visible in the DOM tree of the
> app. Am I right? Not sure this behavior will cause problem or not.

That depends on the implementation strategy.  If you simply use the normal DOM APIs to inject the caret, yes.

> > > b. Need to expose many events to gaia, like the scrolling, touch events and
> > > orientation changes. And it will also have the synchronization and
> > > performance issue(bug 1066515) I guess. May I know your concern about
> > > involving UX specs inside gecko?
> > 
> > Yeah, there may definitely be performance problems initially, but we may be
> > able to address them.
> > 
> > The issue is that the Firefox OS UX specs are specific to Firefox OS (and
> > some of them to Firefox OS releases, i.e., they may change across versions)
> > and Gecko is not specific to Firefox OS.  Therefore, anywhere that we make
> > Gecko only do what the UX specs require, we're introducing problems for
> > sharing the code with other consumers such as Fennec, which may want a
> > different behavior than what we want for Firefox OS.  Therefore, at least we
> > should make sure that we add hooks that allow us to enable the Firefox OS
> > specific behavior in Gecko only on Firefox OS and not elsewhere.
> > 
> > > > But please file and fix a follow-up bug to add a pref to disable toggling
> > > > the visibility of the selection and touch carets inside Gecko.  This
> > > > behavior is undesirable for Fennec, for example, so this should be a Firefox
> > > > OS specific hack for now, hidden behind a pref that we only turn on for b2g.
> > > > 
> > > Ehsan, current gecko already has the preferences to enable/disable touch
> > > caret[1] and selection carets[2]. Both are only enabled on B2G. I assume
> > > this is what you want, am I right?
> > 
> > No.  We want to share this code with Fennec and get rid of the current
> > implementation of the selection carets that Fennec has.  The prefs you are
> > talking about are useful to disable this functionality on products where it
> > doesn't make sense (such as Firefox Desktop), but right now merely enabling
> > them will give you the hiding behavior in the Firefox OS UX specs, which is
> > not what Fennec wants to do.  I want to add two prefs, controlling whether
> > this hiding logic happens, make their default value false, and turn them on
> > only on b2g, so that when Fennec turns on the touch caret and selection
> > caret prefs, they don't get the Firefox OS specific bits of the behavior.
> > 
> > I hope this helps clarify what I would like us to achieve here.  If not,
> > please feel free to ask more questions.  :-)
> Thanks for above detail information, I understood your concern now. As you
> mentioned, we implement touch/selection carets in gecko and also involves
> the UX logic inside carets. Instead of adding prefs, I would prefer to align
> the UX between Fennec and FxOS. If we have solid UX spec for touch/seleciton
> carets, then we could provide the consistent behavior for multiple
> produces(FxOS, Fennec). How do you think? Just for your information, I also
> feedback the different UX behavior to FxOS UX.

Well, it would be really nice if we adopted similar UX on Firefox OS and Fennec for selection, for sure.  But please note that Fennec probably needs to adhere to the Android conventions.  Also, I still think that parts of the UX spec for selection for Firefox OS are worse than the Android selection conventions (specifically things such as hiding the UI when scrolling, bug 1059165, etc.)  Because of those reasons, I'm not in a rush to suggest to the Fennec team to adopt the Firefox OS UX here. :(  So, as things currently stand, I don't see a way for us to avoid having these override prefs.

Based on the above, unless if you can think of a way of aligning the selection UX between Firefox OS and Android, please file a bug and submit a patch for those prefs.  Much appreciated! :-)
Flags: needinfo?(ehsan.akhgari)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: