372345 - [Midas] text cursor overrides CSS mouse cursor styles for some elements

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Phil Crosby]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060911 Camino/1.0.3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2

The editor ignores the cursor style for elements, always displaying a text cursor instead of the user-defined cursor.




Reproducible: Always

Steps to Reproduce:
Add a link element to the body of editor's iframe, and set its style.cursor property to "pointer". Mouse over the link element
Actual Results:  
The mouse cursor is a text caret.

Expected Results:  
The mouse cursor should be a pointer / hand cursor

The body of the document can have a custom cursor defined, but link elements (and I suspect all other text-based elements) cannot.

Comment 1

[Phil Crosby]

Attachment: Simple test case

Comment 2

[Marcos Almeida]

This also happens with images. Put the following into the midas demo page:

foo<span style="cursor: e-resize;"><img style="cursor: wait;" src="http://www.google.com/images/logo.gif">bar</span>

If you hover over "bar" you'll see the resize cursor, but if you hover over the image it stays as the pointer.

Checked on Mozilla/5.0 (Windows NT 5.1; rv:2.0b5pre) Gecko/20100826 Minefield/4.0b5pre (.NET CLR 3.5.30729)

This affects Google properties.

Comment 3

[(no longer active)]

(In reply to comment #2)
> This affects Google properties.

Marcos, could you please be more specific on which Google apps this affects?  If it's serious we can make sure to get it fixed for Firefox 4.

Comment 4

[Marcos Almeida]

This affects Blogger's new post editor. It allows the user to drag images to change their position or alignment. When the mouse is over an image, the cursor changes to "move" so that the user will realize that they are able to drag the image. Because of this bug, FF users will not discover this feature.

Comment 5

[(no longer active)]

Thanks for the information!  Nominating for blocking.

Comment 6

[(no longer active)]

Note to self: the invalid cursor is probably coming from here: <http://mxr.mozilla.org/mozilla-central/source/layout/style/contenteditable.css#68>

Comment 7

[(no longer active)]

Attachment: Patch (v1)

We should load the contenteditable and designmode stylesheets as agent stylesheets, which can be overridden by content, not as override stylesheets, which are basically not possible to override except by other override stylesheets.

As a bonus, with this patch, we pass one more browserscope test!  :-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ehsan, does this editing state code get entered if the presshell is recreated?  If not, it looks like we'll lose the stylesheets when that happens (though we may have had that issue before too, no)?

Is there a reason to not just save references to the stylesheet objects on the document and use object identity instead of URI compares to remove them?

Comment 9

[(no longer active)]

(In reply to comment #8)
> Ehsan, does this editing state code get entered if the presshell is recreated? 
> If not, it looks like we'll lose the stylesheets when that happens (though we
> may have had that issue before too, no)?

How can I get presshell to recreate in order to test this?  And yes, if this is a problem, it has existed before as well, and is not directly related to this bug.

We do call this from EndUpdate <http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#2965>, but I'm not sure if that's enough or not.

> Is there a reason to not just save references to the stylesheet objects on the
> document and use object identity instead of URI compares to remove them?

These objects are (should be) owned by the presshell, right?  I guess I could store them as members of nsHTMLDocument, but is the cost of looking them up in the agent stylesheet table so high to justify the memory footprint that we'll pay for every HTML document?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> How can I get presshell to recreate in order to test this? 

<iframe id="x"></iframe>
<script>
  // Nuke the presshell in the iframe and create a new one
  var x = document.getElementById("x");
  x.style.display = "none";
  document.body.offsetWidth;
  x.style.display = "";
  document.body.offsetWidth;
</script>

> These objects are (should be) owned by the presshell, right?

Yes, but see above.  Presshells don't have the same lifetime as documents...

> but is the cost of looking them up in the agent stylesheet table

I'm not sure what you mean.

The memory cost of caching them will be two words per HTML document, right?

Comment 11

[(no longer active)]

(In reply to comment #10)
> > How can I get presshell to recreate in order to test this? 
> 
> <iframe id="x"></iframe>
> <script>
>   // Nuke the presshell in the iframe and create a new one
>   var x = document.getElementById("x");
>   x.style.display = "none";
>   document.body.offsetWidth;
>   x.style.display = "";
>   document.body.offsetWidth;
> </script>

OK, for this test case, this code triggers, which ends up calling nsHTMLDocument::EditingStateChanged, so we should be fine for presshell recreation.

<http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#677>

> > These objects are (should be) owned by the presshell, right?
> 
> Yes, but see above.  Presshells don't have the same lifetime as documents...

But it shouldn't be a problem, because we reset the stylesheets on new presshells.

> > but is the cost of looking them up in the agent stylesheet table
> 
> I'm not sure what you mean.
> 
> The memory cost of caching them will be two words per HTML document, right?

It is, but it's two words which are almost never needed (a very small percentage of HTML documents loaded in a typical user's browser is editable, right?)

Comment 12

[Boris Zbarsky [:bzbarsky]]

> OK, for this test case, this code triggers

Oh, right, the "hack around editor being broken" thing saves the day.  OK, good.  In that case, no point in caching the sheets in the document.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486492 [details] [diff] [review]
Patch (v1)

r=me

Comment 14

[(no longer active)]

Attachment: Part 2: Ensure that the editor gets reconstructed when navigating back to an editable page

This patch started to regress <http://mxr.mozilla.org/mozilla-central/source/docshell/test/navigation/test_bug386782.html?force=1>.  This new patch basically ports the fix we took in bug 289384 for this issue.  Note that AddOverrideStyleSheet calls ReconstructStyleData, and this needs to happen before flushing the style changes.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486990 [details] [diff] [review]
Part 2: Ensure that the editor gets reconstructed when navigating back to an editable page

r=me

Comment 16

[(no longer active)]

This patch is causing weird crashes in the browser-chrome suite on the try server which I'm not able to reproduce locally.  I'm investigating it...

Comment 17

[(no longer active)]

Attachment: Part 3: cleanup

I also wrote a cleanup patch which I forgot to attach...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488273 [details] [diff] [review]
Part 3: cleanup

r=me

Comment 19

[(no longer active)]

It turns out that the crash that I was talking about was coming from the patch in bug 607482.  So I guess this one can land.

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/25e44442a6e8
http://hg.mozilla.org/mozilla-central/rev/8c90cf13be68
http://hg.mozilla.org/mozilla-central/rev/9912b2cfde1d