1018639 - [e10s] Mouse cursor indicates a bidirectional resize

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Alice0775 White]

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

Comment 1

[Alice0775 White]

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.

Comment 3

[Gary [:streetwolf52]]

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

Comment 4

[Gary [:streetwolf52]]

Attachment: video of my problem.

Comment 6

[Gary [:streetwolf52]]

Aren't there any takers for this one?

Comment 7

[Jim Mathies [:jimm]]

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

Comment 9

[Jim Mathies [:jimm]]

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Jim Mathies [:jimm]]

Cosmetic but also really annoying.

Comment 13

[Gary [:streetwolf52]]

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

Agreed.

Comment 14

[Paul Dickler]

(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).

Comment 15

[Gary [:streetwolf52]]

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.

Comment 16

[Gary [:streetwolf52]]

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

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 19

[David Parks [:handyman]]

Attachment: Set cursor on WM_SETCURSOR when over content

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.

Comment 20

[David Parks [:handyman]]

Attachment: 2. Notify content when chrome changes cursor

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).

Comment 21

[David Parks [:handyman]]

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.

Comment 22

[Jim Mathies [:jimm]]

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

This should be ok.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 24

[David Parks [:handyman]]

Attachment: 2. Notify content when chrome changes cursor

(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

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 26

[David Parks [:handyman]]

Attachment: 2. Notify content when chrome changes cursor

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

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 28

[Gary [:streetwolf52]]

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.

Comment 29

[David Parks [:handyman]]

Attachment: 2. Establish separate cursors for chrome and content procs

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

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 31

[David Parks [:handyman]]

Attachment: 2. Establish separate cursors for chrome and content procs (r+ed by roc)

Whitespace fix.  Tests in comment 29.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Jim Mathies [:jimm]]

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

Comment 35

[Jim Mathies [:jimm]]

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.