Closed Bug 372345 Opened 15 years ago Closed 11 years ago

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

Categories

(Core :: DOM: Editor, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: phil.crosby, Assigned: ehsan.akhgari)

References

Details

Attachments

(4 files)

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.
Attached file Simple test case
QA Contact: editor
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.
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Thanks for the information!  Nominating for blocking.
blocking2.0: --- → ?
Note to self: the invalid cursor is probably coming from here: <http://mxr.mozilla.org/mozilla-central/source/layout/style/contenteditable.css#68>
Assignee: nobody → ehsan
blocking2.0: ? → final+
Attachment #256991 - Attachment mime type: text/plain → text/html
Attached patch Patch (v1)Splinter Review
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!  :-)
Attachment #486492 - Flags: review?(bzbarsky)
Whiteboard: [has patch][needs review bz]
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?
(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?
> 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?
(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?)
> 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 on attachment 486492 [details] [diff] [review]
Patch (v1)

r=me
Attachment #486492 - Flags: review?(bzbarsky) → review+
Whiteboard: [has patch][needs review bz] → [needs landing]
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.
Attachment #486990 - Flags: review?(bzbarsky)
Whiteboard: [needs landing] → [has patch][needs review bz]
Version: unspecified → Trunk
Comment on attachment 486990 [details] [diff] [review]
Part 2: Ensure that the editor gets reconstructed when navigating back to an editable page

r=me
Attachment #486990 - Flags: review?(bzbarsky) → review+
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...
Whiteboard: [has patch][needs review bz] → [crashes try browser-chrome]
Attached patch Part 3: cleanupSplinter Review
I also wrote a cleanup patch which I forgot to attach...
Attachment #488273 - Flags: review?(bzbarsky)
Comment on attachment 488273 [details] [diff] [review]
Part 3: cleanup

r=me
Attachment #488273 - Flags: review?(bzbarsky) → review+
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.
Whiteboard: [crashes try browser-chrome] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/25e44442a6e8
http://hg.mozilla.org/mozilla-central/rev/8c90cf13be68
http://hg.mozilla.org/mozilla-central/rev/9912b2cfde1d
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Depends on: 632215
No longer depends on: 632215
Depends on: 632215
Duplicate of this bug: 431235
You need to log in before you can comment on or make changes to this bug.