309547 - chromeWindow.setCursor() doesn't affect <browser> element

Status: NEW

(Core :: XUL, defect)

Description

[Jens Bannmann]

If you set a cursor on a chrome window, it will affect the cursor only while it
hovers over chrome, not when it hovers a <browser> element.

Steps to reproduce:
1. Open Firefox and DOM Inspector
2. Inspect the Firefox window
3. Select the <window> node, select JavaScript object on the right-hand pane
4. Right-click target, choose "Evaluate JavaScript"
5. Enter: target.ownerDocument.defaultView.setCursor("move");
6. Hover over the Firefox toolbar and over the web page

Expected results:
A "move" cursor both over the chrome and the web page

Actual results:
The cursor is only affected when not hovering the <browser> element.

Comment 1

[Jens Bannmann]

Attachment: alternative testcase

Save this file to hard disk, then do firefox -chrome /path/to/file. It's a XUL
window that sets a 'move' cursor and contains a <browser> and some <spacer>s
around it. (This won't work when loaded in the browser)

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: diff -w (same code as attachment 197003)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Mmmm.... the first question, to the UI folks, is what the desired behavior here is.

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I want this in order to set a cursor when you're autoscrolling.

Comment 6

[[e-mail]]

Note also that there are several instances spread throughout the codebase of
people calling setCursor on all chrome windows (lxr for setBusyCursor).

Maybe the forced cursor should be stored somewhere else e.g. the nsIBaseWindow?

Comment 7

[Martijn Wargers (dead)]

Attachment: Testcase2

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #7)
> Created an attachment (id=197064) [edit]
> Testcase2
> 

The third button isn't expected to work, right?

Comment 9

[Martijn Wargers (dead)]

(In reply to comment #8)
> The third button isn't expected to work, right?
No, currently not.
Biesinger, from IRC: 
mw22, url cursors for window.setCursor aren't expected to work right now
mw22, "nobody implemented it"
mw22, it takes some extra work

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #6)
> Note also that there are several instances spread throughout the codebase of
> people calling setCursor on all chrome windows (lxr for setBusyCursor).
> 
> Maybe the forced cursor should be stored somewhere else e.g. the nsIBaseWindow?

Is that an sr- ?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 197003 [details] [diff] [review]
patch

The real question is whether the cursor the _chrome_ sets makes sense when you're hovering over the _content_.  Once we settle that issue, we can talk about how this should be implemented (assuming there is something to change at that point).

Comment 12

[[e-mail]]

(In reply to comment #11)
>The real question is whether the cursor the _chrome_ sets makes sense when
>you're hovering over the _content_.
No, the real question is "how do I lock the cursor for an entire XUL window?"
[or maybe base window, if this needs to play nicely with embedding]

Comment 13

[[e-mail]]

Sorry, that was still too ambiguous. I originally wrote "entire window", but realized that since we can already lock the cursor for an nsGlobalChromeWindow (but not its descendents) that I could be misinterpreted, so then I wrote "entire XUL window" meaning an nsXULWindow rather than a window containing XUL.

Comment 14

[Martijn Wargers (dead)]

So any update if this patch is something that is wanted?
I think the patch is very useful.
A colorpicker extension that I know of, now uses an ugly hack to get the same cursor in the entire window, but with this patch, they would not need it anymore.

Comment 15

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #14)
> So any update if this patch is something that is wanted?
> I think the patch is very useful.
> A colorpicker extension that I know of, now uses an ugly hack to get the same
> cursor in the entire window, but with this patch, they would not need it
> anymore.

It would improve the behavior for SeaMonkey's autoscroll implementation as well.

Neil, am I reading your comments correctly as "we need a way for chrome to set the cursor for an entire window, but I'm not sure what this patch does is the right way"?

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Who do I poke to get a decision?

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #16)
> Who do I poke to get a decision?
> 

Comment 18

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

I nominate Neil. (FWIW, I think locking the cursor for all subshells would be reasonable.)

Comment 19

[[e-mail]]

(In reply to comment #18)
>I nominate Neil.
Since Neil Deakin isn't CC'd I guess that means me ;-)

Unfortunately I'm not sure I want to make the decision.
As I recall, cursor setting is currently handled by the ESM associated with the chrome window in question. CTho's current patch simply recursively calls SetCursor on all available ESMs. This means that a) if you create a new frame it doesn't have a set cursor and b) if you call SetCursor on a chrome frame it overrides an existing SetCursor on its parent. For a) I guess we could make nsEventStateManager::SetPresContext scrape the cursor from its parent ESM, while for b) we could either accept the behaviour or check to see if the parent window (if any) already had a cursor.
The first alternative is to make the ESM walk the ESM tree to find the cursor. This would however appear to be really inefficient given the ESM-walking code.
The second alternative is to store the cursor elsewhere such as the xul window.

I notice the current SetCursor code checks that the window has a widget before trying to set the cursor on the ESM. Does anyone know why this might be? Won't roc's proposed widget changes break this, or will it simplify things because there will be fewer places to set the cursor?

Comment 20

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

Which check? Here?
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#7712

That will need to be changed eventually along with a lot of other stuff. Don't worry about it now.

Comment 21

[[e-mail]]

(In reply to comment #18)
>(FWIW, I think locking the cursor for all subshells would be reasonable.)
Hey, there's our decision :-) Sorry for not noticing it before.

Comment 22

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 197004 [details] [diff] [review]
diff -w (same code as attachment 197003 [details] [diff] [review])

jst, Neil was concerned about what to do in the case where one of the calls fails, and if it's safe to assume docshells have prescontexts.

Comment 23

[Johnny Stenback (:jst)]

(In reply to comment #22)
> (From update of attachment 197004 [details] [diff] [review])
> jst, Neil was concerned about what to do in the case where one of the calls
> fails, and if it's safe to assume docshells have prescontexts.

It is *not* safe to assume a docshell has a prescontext. In the case where an [i]frame is hidden (display:none) it won't have one. If any of the calls in this code fails then we'll end up in an inconsistent state, and I don't know that there's any reasonable guaranteed way to get around that, and I honestly don't think that's big issue.

Neil mentioned that a new frame inserted after the cursor has been set won't get the set cursor, and nor will a frame that was hidden and gets shown after the cursor was set. That I think is probably worth looking into for this issue.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 197004 [details] [diff] [review]
diff -w (same code as attachment 197003 [details] [diff] [review])

Clearing review flag here, a new patch is in order here.

Comment 25

[David Frahm]

Just noting that this is useful for Touchscreen/Kiosk and Mobile/Handheld applications where a cursor is not relative, and thus becomes annoying and even distracting to some users. And you might be surprised to know what hacks one must resort to in order to attempt to never see a cursor under X -- and even then you're left wondering if you missed something.

Comment 26

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

-> somebody else

Comment 27

[Erik]

I grabbed the patch labeled "diff -w (same code as attachment 197003 [details] [diff] [review])" above and applied it to the xulrunner-1.9 Debian source package from Ubuntu Intrepid (called xulrunner-1.9-1.9.0.4+nobinonly). The patch applies cleanly with just a line offset of 1700 or so lines.

I then compiled it, but

    browser_win.setCursor ('wait')

doesn't seem to work inside the <browser> element but it does outside that.

Further debugging required.

Comment 28

[Erik]

With the patch applied, the command:

    window.setCursor ('none');

hides the cursor until a new page is loaded. If I also call setCursor at the start of onLocationChange method, the cursor will flick into view for less than a second and then disappear again.

The above works for HTML but not for Flash components within the page (stupid bloody flash). Looks like I'll need another hammer for that problem.

Comment 29

[Erik]

With the patch applied, the command:

    window.setCursor ('none');

hides the cursor until a new page is loaded. If I also call setCursor at the start of onLocationChange method, the cursor will flick into view for less than a second and then disappear again.

The above works for HTML but not for Flash components within the page (stupid bloody flash). Looks like I'll need another hammer for that problem.