Bug 39978

Summary: spellcheck="false" not respected in designMode
Product: WebKit Reporter: Tony Chang <tony>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Minor CC: abarth, darin, enrica, eric, hyatt, jacobly, mjs, ojan, rniwa, webkit.review.bot
Priority: P4 Keywords: EasyFix, HasReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
test case
none
test case, logs activeELement
none
fixes the bug
none
Moved the change to HTMLBodyElement darin: review+

Description Tony Chang 2010-05-31 20:06:56 PDT
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.
Comment 1 Ojan Vafai 2010-06-03 16:35:21 PDT
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.
Comment 2 Ryosuke Niwa 2010-10-07 14:26:08 PDT
(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
Comment 3 Ryosuke Niwa 2010-10-07 14:39:48 PDT
+darin & othermaciej since I'm not too comfortable touching Node::isFocusable with all references to renderer and layout code.
Comment 4 Darin Adler 2010-10-07 17:55:15 PDT
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.
Comment 5 Ryosuke Niwa 2010-10-07 19:47:33 PDT
(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.
Comment 6 Ryosuke Niwa 2010-10-08 18:08:25 PDT
Created attachment 70325 [details]
fixes the bug
Comment 7 Darin Adler 2010-10-08 18:09:32 PDT
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?
Comment 8 Ryosuke Niwa 2010-10-09 14:47:05 PDT
Created attachment 70375 [details]
Moved the change to HTMLBodyElement
Comment 9 Ryosuke Niwa 2010-10-09 14:50:26 PDT
(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.
Comment 10 Darin Adler 2010-10-10 15:47:03 PDT
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.
Comment 11 Ryosuke Niwa 2010-10-11 11:12:47 PDT
Committed r69509: <http://trac.webkit.org/changeset/69509>
Comment 12 WebKit Review Bot 2010-10-11 11:58:51 PDT
http://trac.webkit.org/changeset/69509 might have broken GTK Linux 32-bit Release
Comment 13 Ryosuke Niwa 2010-10-11 17:35:36 PDT
(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!