[e10s] Mouse cursor indicates a bidirectional resize

VERIFIED FIXED in Firefox 40

Status

()

VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: alice0775, Assigned: handyman)

Tracking

(Blocks: 2 bugs)

32 Branch
mozilla40
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm6+, firefox40 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

5 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
(Reporter)

Comment 1

5 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.
tracking-e10s: --- → ?

Updated

4 years ago
No longer blocks: 653064
tracking-e10s: ? → +
Depends on: 973565
(Reporter)

Updated

4 years ago
Blocks: 653064
Duplicate of this bug: 1095931

Comment 3

4 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

Comment 4

4 years ago
Created attachment 8532810 [details]
video of my problem.

Updated

4 years ago
Duplicate of this bug: 1103937

Comment 6

4 years ago
Aren't there any takers for this one?

Comment 7

4 years ago
I'd bet this and the parent bug will get fixed by bug 936092, which smaug is currently working on.
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1122892

Comment 9

4 years ago
Hey smaug, is my comment 7 correct? If not we need to re-triage this and get it into a milestone.
Flags: needinfo?(bugs)

Updated

4 years ago
Duplicate of this bug: 1123695
Nothing to do with bug 936092.
(and can't reproduce on linux)
Flags: needinfo?(bugs)

Comment 12

4 years ago
Cosmetic but also really annoying.
tracking-e10s: + → ?

Comment 13

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

Agreed.

Comment 14

4 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).
tracking-e10s: ? → m7+

Comment 15

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

Comment 16

4 years ago
C'mon guys this can't be that hard to fix?  A few more months and it will be a year old.
Assignee: nobody → davidp99
tracking-e10s: m7+ → m6+
We're going to use this bug as a substitute for bug 1081691, which turned out to be a shim issue.
Blocks: 1120078

Updated

4 years ago
No longer depends on: 973565
Duplicate of this bug: 973565

Updated

4 years ago
Blocks: 1140199

Updated

4 years ago
Depends on: 1081691
(Assignee)

Comment 19

4 years ago
Created attachment 8579088 [details] [diff] [review]
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.
Attachment #8579088 - Flags: review?(jmathies)
(Assignee)

Comment 20

4 years ago
Created attachment 8579103 [details] [diff] [review]
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).
Attachment #8579103 - Flags: review?(bugs)
(Assignee)

Comment 21

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

Comment 22

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

This should be ok.
Attachment #8579088 - Flags: review?(jmathies) → review+
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
Attachment #8579103 - Flags: review?(bugs) → review-
(Assignee)

Comment 24

4 years ago
Created attachment 8583272 [details] [diff] [review]
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
Attachment #8579103 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8583272 - Flags: review?(bugs)
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.
Attachment #8583272 - Flags: review?(bugs) → review-
(Assignee)

Comment 26

4 years ago
Created attachment 8587419 [details] [diff] [review]
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
Attachment #8583272 - Attachment is obsolete: true
Attachment #8587419 - Flags: review?(roc)
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.
Attachment #8587419 - Flags: review?(roc) → review-

Comment 28

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

Comment 29

4 years ago
Created attachment 8592364 [details] [diff] [review]
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
Attachment #8587419 - Attachment is obsolete: true
Attachment #8592364 - Flags: review?(roc)
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
Attachment #8592364 - Flags: review?(roc) → review+
(Assignee)

Comment 31

4 years ago
Created attachment 8592537 [details] [diff] [review]
2. Establish separate cursors for chrome and content procs (r+ed by roc)

Whitespace fix.  Tests in comment 29.
Attachment #8592364 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d3a3a4a64303
https://hg.mozilla.org/mozilla-central/rev/9ab4b311726d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Comment 34

4 years ago
Haven't seen this at all in the 4-18 build. Thanks!
Status: RESOLVED → VERIFIED

Comment 35

4 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.
Duplicate of this bug: 1150225

Updated

4 years ago
Blocks: 1163044

Updated

3 years ago
Depends on: 1199892

Updated

3 years ago
Depends on: 1222662
(Reporter)

Updated

3 years ago
Depends on: 1232636
(Reporter)

Updated

3 years ago
Depends on: 1232640

Updated

3 years ago
No longer depends on: 1232636
You need to log in before you can comment on or make changes to this bug.