Bug 39978 – spellcheck="false" not respected in designMode

• Component: HTML Editing
• Status: RESOLVED FIXED

[Tony Chang]

Created attachment 57512 [details]
test case

See attached test case.  If you click on the misspelled word and then click off of it, it gets a red underline even though spellcheck=false.

This is because when designMode = "on", the HTML element has focus and our check for the spellcheck tag happens by taking the focused node and climbing the DOM.

It would probably be straightforward to check based on the cursor position as well, but maybe we should change how focus works when designMode = "on" instead.

[Ojan Vafai]

Created attachment 57832 [details]
test case, logs activeELement

Adding a test case that logs activeElement to the console.

As per spec, designMode="on" should be equivalent to setting contentEditable on the body. So, I think the focus being on the HTML element is incorrect. To clarify, the body element is focused for the first mouseDown, but the HTML element is focused at the second mousedown. That definitely looks like a bug.

I'm not sure what should happen if you set contentEditable on the HTML element itself though.

[Ryosuke Niwa]

(In reply to comment #1)
> As per spec, designMode="on" should be equivalent to setting contentEditable on the body. So, I think the focus being on the HTML element is incorrect. To clarify, the body element is focused for the first mouseDown, but the HTML element is focused at the second mousedown. That definitely looks like a bug.

In SelectionController::setFocusedNodeIfNeeded, I see the target pointing at html element in your test case.  This is probably a bug in Node::isFocusable().  The following change seems to fix this bug:

Index: WebCore/dom/Node.cpp
===================================================================
--- WebCore/dom/Node.cpp	(revision 69327)
+++ WebCore/dom/Node.cpp	(working copy)
@@ -776,9 +776,10 @@
     
 bool Node::isFocusable() const
 {
-    if (!inDocument() || !supportsFocus())
+    bool isBodyInDesignMode = document() && document()->inDesignMode() && hasTagName(bodyTag);
+    if (!inDocument() || (!supportsFocus() && !isBodyInDesignMode))
         return false;
-    
+
     if (renderer())
         ASSERT(!renderer()->needsLayout());
     else

[Ryosuke Niwa]

+darin & othermaciej since I'm not too comfortable touching Node::isFocusable with all references to renderer and layout code.

[Darin Adler]

I don’t immediately know whether it’s OK to change Node::isFocusable like this.

It seems strange to have an inDesignMode special case, since we can also make the entire body of a document be editable with CSS.

It’s not pleasant to have logic this specific inside this basic a function. We have an HTMLBodyElement class, so we could override isFocusable there rather than putting body-specific code into Node.

I’m not sure isFocusable is the right level for this either.

[Ryosuke Niwa]

(In reply to comment #4)
> I don’t immediately know whether it’s OK to change Node::isFocusable like this.
> It seems strange to have an inDesignMode special case, since we can also make the entire body of a document be editable with CSS.

I don't like this change either.  Maybe the same problem occurs with other elements as well.  In fact, supportsFocus returning false for editable nodes seems wrong.

[Ryosuke Niwa]

Created attachment 70325 [details]
fixes the bug

[Darin Adler]

Comment on attachment 70325 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=70325&action=review

> WebCore/html/HTMLElement.cpp:635
> +    return this->hasTagName(bodyTag) || (parent() && !parent()->isContentEditable());

Can we put the body-specific code in an override of this function in the HTMLBodyElement class instead of calling hasTagName here?

[Ryosuke Niwa]

Created attachment 70375 [details]
Moved the change to HTMLBodyElement

[Ryosuke Niwa]

(In reply to comment #7)
> (From update of attachment 70325 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70325&action=review
> 
> > WebCore/html/HTMLElement.cpp:635
> > +    return this->hasTagName(bodyTag) || (parent() && !parent()->isContentEditable());
> 
> Can we put the body-specific code in an override of this function in the HTMLBodyElement class instead of calling hasTagName here?

Done.

[Darin Adler]

Comment on attachment 70375 [details]
Moved the change to HTMLBodyElement

View in context: https://bugs.webkit.org/attachment.cgi?id=70375&action=review

> WebCore/html/HTMLBodyElement.h:38
> +    virtual bool supportsFocus() const;

I would have made this a private member function. When overriding a function from a base class, the visibility does not need to match the base class. I often favor a design where we make the derived class override with a private function if we never expect to call that function directly on that derived class. A call to the virtual function when the pointer is an HTMLBodyElement* would usually indicate a programming mistake so it’s nice the compiler would catch it.

[Ryosuke Niwa]

Committed r69509: <http://trac.webkit.org/changeset/69509>

[WebKit Review Bot]

http://trac.webkit.org/changeset/69509 might have broken GTK Linux 32-bit Release

[Ryosuke Niwa]

(In reply to comment #10)
> (From update of attachment 70375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70375&action=review
> 
> > WebCore/html/HTMLBodyElement.h:38
> > +    virtual bool supportsFocus() const;
> 
> I would have made this a private member function. When overriding a function from a base class, the visibility does not need to match the base class. I often favor a design where we make the derived class override with a private function if we never expect to call that function directly on that derived class. A call to the virtual function when the pointer is an HTMLBodyElement* would usually indicate a programming mistake so it’s nice the compiler would catch it.

Done.  Thanks for the review!