Closed Bug 1018639 Opened 5 years ago Closed 4 years ago

[e10s] Mouse cursor indicates a bidirectional resize

Categories

(Core :: General, defect)

32 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
e10s m6+ ---
firefox40 --- fixed

People

(Reporter: alice0775, Assigned: handyman)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 4 obsolete files)

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
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.
No longer blocks: fxe10s
Depends on: 973565
Blocks: fxe10s
Duplicate of this bug: 1095931
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
Attached image video of my problem.
Duplicate of this bug: 1103937
Aren't there any takers for this one?
I'd bet this and the parent bug will get fixed by bug 936092, which smaug is currently working on.
Duplicate of this bug: 1122892
Hey smaug, is my comment 7 correct? If not we need to re-triage this and get it into a milestone.
Flags: needinfo?(bugs)
Duplicate of this bug: 1123695
Nothing to do with bug 936092.
(and can't reproduce on linux)
Flags: needinfo?(bugs)
Cosmetic but also really annoying.
(In reply to Jim Mathies [:jimm] from comment #12)
> Cosmetic but also really annoying.

Agreed.
(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).
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.
C'mon guys this can't be that hard to fix?  A few more months and it will be a year old.
We're going to use this bug as a substitute for bug 1081691, which turned out to be a shim issue.
Blocks: 1120078
No longer depends on: 973565
Duplicate of this bug: 973565
Blocks: 1140199
Depends on: 1081691
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)
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)
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 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-
(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
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-
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-
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.
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d3a3a4a64303
https://hg.mozilla.org/mozilla-central/rev/9ab4b311726d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Haven't seen this at all in the 4-18 build. Thanks!
Status: RESOLVED → VERIFIED
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
Blocks: 1163044
Depends on: 1199892
Depends on: 1222662
Depends on: 1232636
Depends on: 1232640
No longer depends on: 1232636
You need to log in before you can comment on or make changes to this bug.