contenteditable=true breaks mouse pointer hover style for unrelated image links

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Editor
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: zwol, Assigned: zwol)

Tracking

({regression, verified1.9.0.11, verified1.9.1})

1.9.0 Branch
mozilla1.9.2a1
regression, verified1.9.0.11, verified1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +
in-testsuite ?
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 364548 [details]
test case

In the attached test case, there is a large cyan box with a plus sign in the middle; this is an image link, but if you hover the mouse over the image, the pointer doesn't change to the pointing-hand icon that links in general normally display.  If you move the pointer onto the image very slowly (it's easiest to do this from below the image) you can see that when the hotspot is over the link outline you do get the pointing-hand, but as soon as it's over the image proper it reverts to the regular arrow.

This is caused simply by having <div contenteditable="true"> somewhere in the document -- it doesn't have to enclose the image.

Works fine in 2.x.  This is easily seen on social networking sites that have an embedded rich-text editor plus not-obviously-styled image links on the same page.
(Assignee)

Comment 1

9 years ago
Clarification: this bug is visible in 3.0, 3.1, and trunk, but not 2.0.
(Assignee)

Comment 2

9 years ago
Created attachment 364556 [details]
better test case

Here's an improved test case which is self-describing, but unfortunately still manual.  I'm not aware of any way to automate this test.
Attachment #364548 - Attachment is obsolete: true
(Assignee)

Comment 3

9 years ago
Created attachment 364560 [details] [diff] [review]
patch

This turns out to be really easy to fix.  layout/style/contenteditable.css applies to *everything* in a document that has a contenteditable element *anywhere* inside, so all of its selectors need to take care to select only things that are actually editable.  There are two rules relevant here: the "override the browser's pointer cursor over links" one and the "prevent clicking on links from going to link" one; both of them need all their selectors qualified with :-moz-read-write.  

(The "prevent clicking on links" rule doesn't seem to do anything -- as is, one would naively expect it to render every image link in a document with a contenteditable nonfunctional -- maybe it should just be taken out.)

There may be other rules in this file that have similar problems -- the ::-moz-canvas and ::-moz-display-combobox-control-frame rules look suspicious -- but I am not going to mess with 'em in the absence of a test case, this stuff is too black magic for me.

As a UI regression with a straightforward fix, I say this should go on all active branches.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #364560 - Flags: superreview?(peterv)
Attachment #364560 - Flags: review?(peterv)
Attachment #364560 - Flags: approval1.9.1?
Attachment #364560 - Flags: approval1.9.0.8?
Attachment #364560 - Flags: approval1.9.0.7?
Attachment #364560 - Flags: approval1.8.1.next?
(Assignee)

Comment 4

9 years ago
Comment on attachment 364560 [details] [diff] [review]
patch

... but it's known not to be a problem in ff2/moz1.8.x, duh.
Attachment #364560 - Flags: approval1.8.1.next?
Attachment #364560 - Flags: approval1.9.0.7?
Whiteboard: [needs r/sr]
(Assignee)

Comment 5

9 years ago
Is peterv still around and reviewing patches?  He wrote the file in question so he seems like the obvious reviewer, but we've had no action on the patch in more than a week.
Peterv often needs to be bugged by email to do reviews.

Also, would you mind clearing the approval requests for now and making them again after it's landed on mozilla-central?
(Assignee)

Updated

9 years ago
Attachment #364560 - Flags: approval1.9.1?
Attachment #364560 - Flags: approval1.9.0.8?
(Assignee)

Comment 7

9 years ago
Comment on attachment 364560 [details] [diff] [review]
patch

> would you mind clearing the approval requests for now?

No prob.
Comment on attachment 364560 [details] [diff] [review]
patch

>diff --git a/layout/style/contenteditable.css b/layout/style/contenteditable.css

> a[name]:-moz-only-whitespace {

I *think* we want to add -moz-read-write here too, no?
Attachment #364560 - Flags: superreview?(peterv)
Attachment #364560 - Flags: superreview+
Attachment #364560 - Flags: review?(peterv)
Attachment #364560 - Flags: review+
(Assignee)

Comment 9

9 years ago
I wasn't sure what that bit meant so I didn't mess with it, but it might indeed be safer.  You surely know more about this stuff than I do...
(Assignee)

Comment 10

9 years ago
Created attachment 367070 [details] [diff] [review]
patch v2: following peterv's suggestion
[Checkin: Comment 11 & 16 & 20]

Revised patch - only difference is addition of :-moz-read-write to the a[name]:-moz-only-whitespace rule.
Attachment #364560 - Attachment is obsolete: true
Attachment #367070 - Flags: superreview+
Attachment #367070 - Flags: review+
(Assignee)

Updated

9 years ago
Flags: in-litmus?
Keywords: checkin-needed
Whiteboard: [needs r/sr] → [needs trunk landing]
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
http://hg.mozilla.org/mozilla-central/rev/d4097abab19f
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Summary: [2.x regression] contenteditable=true breaks mouse pointer hover style for unrelated image links → contenteditable=true breaks mouse pointer hover style for unrelated image links
Whiteboard: [needs trunk landing]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → 1.9.0 Branch
(Assignee)

Comment 12

9 years ago
Comment on attachment 367070 [details] [diff] [review]
patch v2: following peterv's suggestion
[Checkin: Comment 11 & 16 & 20]

re-requesting approval for 1.9.1 and 1.9.0 now it's landed on the trunk.
Attachment #367070 - Flags: approval1.9.1?
Attachment #367070 - Flags: approval1.9.0.8?
Whiteboard: [needs 1.9.1 approval/landing]
Attachment #367070 - Flags: approval1.9.0.8?
(Assignee)

Comment 13

9 years ago
Hm, why not this patch for 1.9.0.x?
Zack: This needs to land on 1.9.1 and bake there before we're take it on 1.9.0.
Comment on attachment 367070 [details] [diff] [review]
patch v2: following peterv's suggestion
[Checkin: Comment 11 & 16 & 20]

a1.9.1=dbaron
Attachment #367070 - Flags: approval1.9.1? → approval1.9.1+
Flags: wanted1.9.1? → wanted1.9.1+

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 approval/landing]
(Assignee)

Updated

9 years ago
Attachment #367070 - Flags: approval1.9.0.11?
This is rather specific to be put into litmus, can this be pushed off to testsuite?


verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090430 Minefield/3.6a1pre ID:20090430031133

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090430 Shiretoko/3.5b5pre ID:20090430031222
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
Flags: in-litmus-
Keywords: fixed1.9.1 → verified1.9.1
Comment on attachment 367070 [details] [diff] [review]
patch v2: following peterv's suggestion
[Checkin: Comment 11 & 16 & 20]

Approved for 1.9.0.11, a=dveditz for release-drivers
Attachment #367070 - Flags: approval1.9.0.11? → approval1.9.0.11+
(Assignee)

Comment 19

9 years ago
c-n for 1.9.0 branch
Keywords: checkin-needed
Whiteboard: [c-n: 1.9.0 branch]
Fix checked into the 1.9.0 branch
Keywords: fixed1.9.0.11
Whiteboard: [c-n: 1.9.0 branch]
Attachment #367070 - Attachment description: patch v2: following peterv's suggestion → patch v2: following peterv's suggestion [Checkin: Comment 11 & 16 & 20]
Flags: wanted1.9.0.x?
Keywords: checkin-needed
Version: 1.9.0 Branch → Trunk

Updated

9 years ago
Version: Trunk → 1.9.0 Branch
Verified for 1.9.0.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051304 GranParadiso/3.0.11pre using the attached testcase.
Keywords: fixed1.9.0.11 → verified1.9.0.11
You need to log in before you can comment on or make changes to this bug.