832120 - Shouldn't the default cursor of editable element be also I-beam?

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Twitter stopped using <textarea> for the input form of tweet (including for replay). Instead, they use <div contenteditable> now.

However, the editors look like <textarea> but the mouse cursor doesn't become I-beam on the editor where is no text. I think that over the editable element, the default cursor should be I-beam like <input type="text"> or <textarea>.

And also, if <div contenteditable> has tabindex 0 or more, even over its text, the default cursor doesn't become I-beam. The attached patch also fixes this.

Chrome, IE and Opera uses I-beam cursor on Windows.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 703709 [details] [diff] [review]
Patch

What do those other browser do for things like images, which are very different from blank space next to text?  What about when the mouse is over something like a border?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

On border, the cursor is I-beam (text) on all browsers (including the patched build).

<img> depends on the browser:
  IE10: nwse-resize?
  Chrome: text
  Opera: default
  patched build: default

On buttons, the cursor is default on all browsers (including the patched build).

Comment 3

[(no longer active)]

FWIW I agree with what this patch is doing.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 703709 [details] [diff] [review]
Patch

So there's no need to null-check mContent in nsTextFrameThebes; I think there may well be in nsFrame, though.

My bigger concern is really spec-related, though.  I think we had an extended working group discussion a while back about what cursor: auto means; some people thought the definition in the spec (which is very poorly worded) meant that everything that would normally go in the UA style sheet had to go in the implementation of 'auto'.  I argued that 'auto' was only there for an appropriate distinction between 'text' and 'default'.  I'm a little hesitant to add any more logic to the behavior of 'auto' until the spec's definition gets clarified, since I think adding any more logic might undercut our position that we don't want to put all the complexity inside 'auto'.  That said, I think this is probably ok, since it's still (a) about making a distinction between 'text' and 'default' rather than between any other cursor types and (b) could still match a spec clarification that defines 'auto' to be "The UA determines whether to display either the 'text' or 'default' cursor based on the current context."

So r=dbaron, I guess, though I'd note that I'm not ok with 'auto' making any further distinctions beyond those two cursors and the vertical-text cursor

Still, this might require that we move all of the 'cursor' declarations currently in forms.css into C++ code, which I'd be rather unhappy about.

Is there any way this could be fixed using contenteditable.css changes instead?

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 703709 [details] [diff] [review]
Patch

Actually, I take that back.  I'd like to know why this can't be fixed in contenteditable.css before taking this patch.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Also see http://lists.w3.org/Archives/Public/www-style/2013Jan/0275.html

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 703709 [details] [diff] [review]
Patch

(In reply to David Baron [:dbaron] from comment #5)
> Comment on attachment 703709 [details] [diff] [review]
> Patch
> 
> Actually, I take that back.  I'd like to know why this can't be fixed in
> contenteditable.css before taking this patch.

The reasons are, (1) I have no idea to support same behavior on designMode editor since the desigMode is not an attr of HTML element and there is no useful selector such as ":editable". (2) The computed "cursor" style of editable div on other browsers (I tested on Chrome and Opera) is "auto". So, I think that we should change the "auto" behavior.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

So, I think that if we use changing our default style approach, it causes reproducing this symptom with some websites which specify "cursor: default;" for the editable element. Therefore, I'm still thinking that my patch's approach is better for compatibility with other browsers.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I'm currently trying to get the other browsers to change behavior because our approach is more consistent with the CSS model and is sufficiently compatible with Web content.  So the only argument in comment 7 and comment 8 to which I give any weight is point (1) in comment 7.

(And, Re comment 8, sites that specify 'cursor: default' will get the default cursor no matter what behavior you give 'cursor: auto'.)

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7)
> The reasons are, (1) I have no idea to support same behavior on designMode
> editor since the desigMode is not an attr of HTML element and there is no
> useful selector such as ":editable".

:read-write in http://dev.w3.org/csswg/css3-ui/#pseudo-ro-rw seems like such a selector.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 703709 [details] [diff] [review]
Patch

r=dbaron, I suppose.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(with the first paragraph of comment 4)

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to David Baron [:dbaron] from comment #9)
> I'm currently trying to get the other browsers to change behavior because
> our approach is more consistent with the CSS model and is sufficiently
> compatible with Web content.  So the only argument in comment 7 and comment
> 8 to which I give any weight is point (1) in comment 7.
> 
> (And, Re comment 8, sites that specify 'cursor: default' will get the
> default cursor no matter what behavior you give 'cursor: auto'.)

Oops, I meant cursor: auto;. I'm concerned about the cursor difference of 'auto' from other browsers.

(In reply to David Baron [:dbaron] from comment #10)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7)
> > The reasons are, (1) I have no idea to support same behavior on designMode
> > editor since the desigMode is not an attr of HTML element and there is no
> > useful selector such as ":editable".
> 
> :read-write in http://dev.w3.org/csswg/css3-ui/#pseudo-ro-rw seems like such
> a selector.

Anyway, I'll check it. Thank you for the information.

I'll post the patch not checking mContent non-null to the tryserver.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

We need the null-check of mContent in nsFrame.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=c3afc0697de4

I'll land the patch which only cuts the check in nsTextFrame.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7055d0a5e9c6

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/594b9c2a8ccc

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/594b9c2a8ccc