Alice0775 White Reporter
	Description • 10 years ago

Steps To Reproduce:
1. Open e10s window
2. Open about:blank (to reproduce the problem easily)
3. Mouse over left border/right border/left-bottom corner/right/bottom corner
   --- mouse cursor indicates a bidirectional resize cursor as expected
4. Move mouse to content area.

Actual Results:
Mouse cursor would not change to default cursor(an arrow).

Expected Results:
Mouse cursor should change to default cursor

	Alice0775 White Reporter
	Comment 1 • 10 years ago

This maybe Windows specific problem.

It is easy to reproduce on Windows7 Classic.
It is slightly difficult to reproduce at bottom border on Windows7 Aero.

	Gary [:streetwolf52]
	Comment 3 • 10 years ago

I don't have the resize problem on the main windows edges but I do have it with sidebars.  Moving my pointer off the sidebar will either produce a horizontal resize icon, presumably from passing over the right border of the sidebar, or the hand icon presumably from moving my pointer from the sidebar after hovering over a link.

Doesn't happen all the time but I'd say >50%.

Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:37.0) Gecko/20100101 Firefox/37.0

	Gary [:streetwolf52]
	Comment 6 • 10 years ago

Aren't there any takers for this one?

	Jim Mathies [:jimm]
	Comment 7 • 10 years ago

I'd bet this and the parent bug will get fixed by bug 936092, which smaug is currently working on.

	Jim Mathies [:jimm]
	Comment 9 • 9 years ago

Hey smaug, is my comment 7 correct? If not we need to re-triage this and get it into a milestone.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 9 years ago

Nothing to do with bug 936092.
(and can't reproduce on linux)

	Jim Mathies [:jimm]
	Comment 12 • 9 years ago

Cosmetic but also really annoying.

	Gary [:streetwolf52]
	Comment 13 • 9 years ago

(In reply to Jim Mathies [:jimm] from comment #12)
> Cosmetic but also really annoying.

Agreed.

	Paul Dickler
	Comment 14 • 9 years ago

(In reply to Gary [:streetwolf] from comment #13)
> (In reply to Jim Mathies [:jimm] from comment #12)
> > Cosmetic but also really annoying.
> 
> Agreed.

Not just cosmetic: The change in cursor is required for the 'EdgeWise' extension, which is in production at this time (my Bug #1123695).

	Gary [:streetwolf52]
	Comment 15 • 9 years ago

Any idea when this will be fixed?  Contrary to Comment 14 I don't have a functional problem with the edgewise pointer.  My problem is mostly the pointer changing after moving it off of a sidebar.  It either stays as the hand pointer, the horizontal size pointer, or correctly displays.

	Gary [:streetwolf52]
	Comment 16 • 9 years ago

C'mon guys this can't be that hard to fix?  A few more months and it will be a year old.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 17 • 9 years ago

We're going to use this bug as a substitute for bug 1081691, which turned out to be a shim issue.

	David Parks [:handyman] Assignee
	Comment 19 • 9 years ago

This patch addresses the STR in comment 0.  This does not address the issue reported by streetwolf (I'm still looking at that).

To be clear, this patch addresses the issue where, if you park your cursor over the window edge (resize cursor), then quickly move the cursor into content, the resize cursor sticks around.  It does not fix the issue wrt the bookmarks sidebar, which is separate.

jimm, I'm not sure if you aren't the right person for this...  If not, feel free to retarget the review request.

	David Parks [:handyman] Assignee
	Comment 20 • 9 years ago

This patch *does* fix the STR with the bookmark sidebar (the one in the video).  It notified the content process whenever chrome changes the cursor so that content can properly filter redundant SetCursor calls (passing all SetCursor calls, as SetCursor is called frequently with no change to the cursor, would be expensive).

	David Parks [:handyman] Assignee
	Comment 21 • 9 years ago

OOPS!  In comment 20, I neglected to mention that the patch is a delta from the patches in bug 1075670.   The patches there must be applied before patch #2 in this bug.

	Jim Mathies [:jimm]
	Comment 22 • 9 years ago

Comment on attachment 8579088 [details] [diff] [review]
Set cursor on WM_SETCURSOR when over content

This should be ok.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 23 • 9 years ago

Comment on attachment 8579103 [details] [diff] [review]
2. Notify content when chrome changes cursor

>     return NS_OK;
>+  } else if (eventType.EqualsLiteral("MozCursorChanged")) {
>+    // If this tab is visible then notify the child process of the cursor change.
This comment sounds odd. whether or not the document has presshell has rather little to do with tab visibility.

>+    nsCOMPtr<nsIContent> content = do_QueryInterface(mFrameElement);
mFrameElement *is* an nsIContent. No need for content variable.

>+    nsIPresShell* shell = (content ? content->OwnerDoc()->GetShell() : nullptr);
>+    nsCOMPtr<nsIWidget> widget = GetWidget();
>+    if (widget && shell && shell->IsActive()) {
I don't really understand why we want this limitation. What happens when the shell become active? Do we update the cursor at that point then?


>+  // Force this widget to adopt the newCursor without notifying the parent
>+  // process (for example, because this setting comes from the parent process)
>+  void ForceCursor(nsCursor newCursor)
>+  { mCursor = newCursor; }
Either 
void ForceCursor(nsCursor newCursor) { mCursor = newCursor; }
or
void ForceCursor(nsCursor newCursor)
{
  mCursor = newCursor;
}

(latter would be correct per coding style)

>+++ b/widget/windows/nsWindow.cpp
Why only windows/ change? We want mWidgetListener to be called consistently on all the platforms

	David Parks [:handyman] Assignee
	Comment 24 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #23)
> Comment on attachment 8579103 [details] [diff] [review]
> 2. Notify content when chrome changes cursor
> 
> >     return NS_OK;
> >+  } else if (eventType.EqualsLiteral("MozCursorChanged")) {
> >+    // If this tab is visible then notify the child process of the cursor change.
> This comment sounds odd. whether or not the document has presshell has
> rather little to do with tab visibility.

Removed.

> 
> >+    nsCOMPtr<nsIContent> content = do_QueryInterface(mFrameElement);
> mFrameElement *is* an nsIContent. No need for content variable.

Removed.

> >+    nsIPresShell* shell = (content ? content->OwnerDoc()->GetShell() : nullptr);
> >+    nsCOMPtr<nsIWidget> widget = GetWidget();
> >+    if (widget && shell && shell->IsActive()) {
> I don't really understand why we want this limitation. What happens when the
> shell become active? Do we update the cursor at that point then?

Removed.

> >+++ b/widget/windows/nsWindow.cpp
> Why only windows/ change? We want mWidgetListener to be called consistently
> on all the platforms

Yes... I completely forgot to circle back on this.  Fixed.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b99ab01ff51

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 9 years ago

Comment on attachment 8583272 [details] [diff] [review]
2. Notify content when chrome changes cursor

>+TabChild::RecvCursorChanged(const uint32_t &aCursor)
>+{
>+  static_cast<PuppetWidget*>(mWidget.get())->ForceCursor((nsCursor)aCursor);
C++ casting, pretty please. So static_cast<nsCursor>

>   // If we held previous content then unregister for its events.
>   if (mFrameElement && mFrameElement->OwnerDoc()->GetWindow()) {
>     nsCOMPtr<nsPIDOMWindow> window = mFrameElement->OwnerDoc()->GetWindow();
>     nsCOMPtr<EventTarget> eventTarget = window->GetTopWindowRoot();
>     if (eventTarget) {
>       eventTarget->RemoveEventListener(NS_LITERAL_STRING("MozUpdateWindowPos"),
>                                        this, false);
>+      eventTarget->RemoveEventListener(NS_LITERAL_STRING("MozCursorChanged"),
>+                                       this, false);
(I think we need to figure out some better way for this stuff, like adding observer to widget or something, but perhaps not in this bug.)

>+  } else if (eventType.EqualsLiteral("MozCursorChanged")) {
>+    nsCOMPtr<nsIWidget> widget = GetWidget();
>+    if (widget) {
>+      unused << SendCursorChanged((uint32_t)widget->GetCursor());
C++ casting
>+  // Force this widget to adopt the newCursor without notifying the parent
>+  // process (for example, because this setting comes from the parent process)
>+  void ForceCursor(nsCursor newCursor) {
{ goes to the next line
>+  /**
>+   * Called when the cursor type (standard, resize, etc) has been changed.
>+   */
>+  virtual void CursorChanged(nsCursor oldCursor, nsCursor newCursor);
aOldCursor, aNewCursor


> nsWindow::SetCursor(nsCursor aCursor)
> {
>     if (mCursor == aCursor && !mUpdateCursor) {
>         return NS_OK;
>     }
>     mUpdateCursor = false;
>+    nsCursor oldCursor = mCursor;
>     mCursor = aCursor;
>     if (mWidget) {
>         mWidget->SetCursor(mCursor);
>     }
>+    if (mWidgetListener) {
>+      mWidgetListener->CursorChanged(oldCursor, mCursor);
>+    }
If mUpdateCursor is true, we end up calling CursorChanged even if it wasn't changed.
Same issue also elsewhere. In some cases there is effectively
if (oldCursor != newCursor) notify();
check, but not in all. Please be consistent. Either always call CursorChanged, or only when the cursor actually changed.

	David Parks [:handyman] Assignee
	Comment 26 • 9 years ago

Includes all of smaug's changes from comment 25 (his review queue is full).

Tests here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4115d803225

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 27 • 9 years ago

Comment on attachment 8587419 [details] [diff] [review]
2. Notify content when chrome changes cursor

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

I don't really like this approach, sorry! Maybe you should wait to get re-review from smaug :-).

It seems to me the right way to do this would be to give each TabChild a "current cursor". TabChild would notify the TabParent (asynchronously) each time its current cursor changed. The chrome process would be responsible for selecting the actual cursor to draw, using a TabParent's current cursor when the cursor is over that TabParent. Then there would be no need to tell content processes about cursor changes.

	Gary [:streetwolf52]
	Comment 28 • 9 years ago

Any progress on this?  While it's not an earth shattering bug it does get annoying at times.  Also want to say that e10s in general work a lot better now.

	David Parks [:handyman] Assignee
	Comment 29 • 9 years ago

Since everyone asked so nicely...

Roc, I think this version is closer to what you had in mind.  The content process takes over setting the cursor when the TabParent gets an NS_MOUSE_ENTER/_SYNTH message and stops on NS_MOUSE_EXIT/_SYNTH (NS_MOUSE_ENTER wasn't properly wired up as it wasn't used -- that's what the EventStateManager code does).

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e26e557d772

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 30 • 9 years ago

Comment on attachment 8592364 [details] [diff] [review]
2. Establish separate cursors for chrome and content procs

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

This looks much better!

::: dom/ipc/TabParent.cpp
@@ +1210,5 @@
> +               event.message == NS_MOUSE_EXIT_SYNTH) {
> +      mTabSetsCursor = false;
> +    }
> +  }
> +  

trailing whitespace

	David Parks [:handyman] Assignee
	Comment 31 • 9 years ago

Whitespace fix.  Tests in comment 29.

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a3a4a64303
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ab4b311726d

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/d3a3a4a64303
https://hg.mozilla.org/mozilla-central/rev/9ab4b311726d

	Jim Mathies [:jimm]
	Comment 34 • 9 years ago

Haven't seen this at all in the 4-18 build. Thanks!

	Jim Mathies [:jimm]
	Comment 35 • 9 years ago

You know I bet we could have written a test or two for this looking at the str in dupes. We might want to file a follow up on doing so.