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

RESOLVED FIXED in mozilla2.0b8

Status

()

RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: phil.crosby, Assigned: Ehsan)

Tracking

Trunk
mozilla2.0b8
x86
All
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(4 attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 256991 [details]
Simple test case
QA Contact: editor

Comment 2

8 years ago
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.
(Assignee)

Comment 3

8 years ago
(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

Comment 4

8 years ago
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.
(Assignee)

Comment 5

8 years ago
Thanks for the information!  Nominating for blocking.
blocking2.0: --- → ?
(Assignee)

Comment 6

8 years ago
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+
(Assignee)

Updated

8 years ago
Attachment #256991 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 7

8 years ago
Created attachment 486492 [details] [diff] [review]
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!  :-)
Attachment #486492 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
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?
(Assignee)

Comment 9

8 years ago
(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?
(Assignee)

Comment 11

8 years ago
(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+
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review bz] → [needs landing]
(Assignee)

Comment 14

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

Updated

8 years ago
Whiteboard: [needs landing] → [has patch][needs review bz]
(Assignee)

Updated

8 years ago
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+
(Assignee)

Comment 16

8 years ago
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...
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review bz] → [crashes try browser-chrome]
(Assignee)

Comment 17

8 years ago
Created attachment 488273 [details] [diff] [review]
Part 3: cleanup

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+
(Assignee)

Comment 19

8 years ago
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]
(Assignee)

Comment 20

8 years ago
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
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8

Updated

8 years ago
Depends on: 632215
(Assignee)

Updated

8 years ago
No longer depends on: 632215
(Assignee)

Updated

8 years ago
Depends on: 632215
(Assignee)

Updated

8 years ago
Duplicate of this bug: 431235
You need to log in before you can comment on or make changes to this bug.