Bug 6987

<rdar://problem/4463713> Implement maxlength for new text fields (6987)

Created attachment 6821 [details] patch for maxlength

+ if (evt->type() == khtmlInsertTextEvent && evt->isInsertTextEvent()) { What's the difference between these two checks? + // Make sure that the text to be inserted will not violate the maxLength + unsigned ml = maxLength(); + if (ml <= 0 || ml > 1024) + ml = 1024; Apply the restriction on maxlength once, when it's set, instead of every time it's used. + unsigned currentLength = value().length(); + String text = static_cast<InsertTextEventImpl *>(evt)->text(); + int selectionLength = getDocument()->frame()->selection().toString().length(); + + // Truncate the inserted text if necessary + if (currentLength + text.length() - selectionLength > ml) { + if (currentLength > ml) + text = String(""); When will currentLength ever be > ml? You could assert(currentLength <= ml). - DOMString value = input->value(); + DOMString value = input->value().copy(); Use String, DOMString is a synonym allowed for backward compatibility, but should not be used in anything new. The NodeStyles computed during the test rendering will become invalid if the InsertTextEvent handler changes the value of the string. You need to send the event earlier, where you do m_text = plainText(range);, and do another test rendering if the event handler forces you to create a new fragment. + if (startNode && startNode->rootEditableElement() && startNode->rootEditableElement()->isAnonymousDiv()) { + // Send InsertTextEvent and update text if necessary + RefPtr<EventImpl> evt = new InsertTextEventImpl(newText); + HTMLAnonymousDivElementImpl *div = static_cast<HTMLAnonymousDivElementImpl*>(startNode->rootEditableElement()); + div->shadowParentNode()->dispatchEvent(evt, exceptionCode, true); + } Why must you check for isAnonymousDiv()? Couldn't you dispatch the InsertTextEvents to all rootEditable elements, and let normal divs disregard the event, and make anonymousDivs send the event to their shadow parent? I don't think that the editing code should know about anonymous divs and shadow parents, it should only know about sending InsertTextEvents and the possibility that whoever handles that event will change the text to be inserted. r- because of the test rendering bit.

Comment on attachment 6821 [details] patch for maxlength see my comments above

Created attachment 6834 [details] new patch to address Justin's comments Justin and I discussed this, and I think this new patch addresses his comments

Darin and I talked about how the khtmlInsertTextEvent that I'm using after the text inserted, can just be a normal EventImpl. And I'd like to give it a better name.

Comment on attachment 6834 [details] new patch to address Justin's comments New top-level classes should be in separate files, even though we didn't split render_block.cpp yet. So HTMLAnonymousDivElementImpl should be in its own file as should InsertTextEventImpl. We might be able to come up with a better name for the HTMLAnonymousDivElementImpl class too. Since its implementation has a special case for the case where its parent has a RenderTextField for a renderer, it should probably have a name and location specific to its use instead of pretending to be more general purpose. For example, it could go in the rendering directory even though it's a subclass of a DOM element class, since it's used as part of form element rendering. HTMLAnonymousDivElementImpl::defaultEventHandler checks if the parent's renderer is a text field, but it doesn't check if its parent is an HTMLInputElementImpl -- instead it assumes that. That should be checked too. As we discussed, khtmlInsertTextEvent does not need to be an InsertTextEventImpl. There's no need to add an unused isAnonymousDiv() function. + HTMLAnonymousDivElementImpl(DocumentImpl *doc, NodeImpl* shadowParent = 0); Should just be DocumentImpl*. No need to give a name. + virtual void defaultEventHandler(EventImpl *evt); Similarly, should just be EventImpl*, no need for "evt". In HTMLInputElementImpl, I'd like to see the enforcement of range from 0 to 1024 be when m_maxLen is set, not in the maxLength() accessor. + return (m_div->textContent()); No need for parentheses on a return statement like this. macro(khtmlHorizontalmousewheel) \ + macro(khtmlBeforeInsertText) \ + macro(khtmlInsertText) \ macro(khtmlMove) \ The other events are in alphabetical order, so you should move BeforeInsertText up. + InsertTextEventImpl(const AtomicString &); + InsertTextEventImpl(const AtomicString &, String); No spaces before the & characters. + virtual void setText(String); Why is this function virtual? Also, it should take a const String&, not a String. We might want to consider a version of the BeforeInsertText event that sends a DOM fragment through instead of a string -- it seems a bit limiting to have this paste preflighting be specific to plain text pasting -- but perhaps the plain text one is easier to implement for now. You should undo the change to ReplaceSelectionCommand.h. The code in HTMLAnonymousDivElementImpl::defaultEventHandler calls subtreeHasChanged *before* calling the default handler. That seems like it's too early -- will the insertion be done at that point? If this event is fired only after text is inserted then perhaps it should be named InsertedText, TextInserted, or AfterInsertText?

I'm about to upload a patch that addresses most of Darin's comments. At one point, I had a version of the new event that stored a DocumentImpl. I switched over to storing just the text to make it easier to get a working version of this up and running. I think its a good enhancement to make for the future. subtreeHasChanged only gets called after the editing command has been applied, and the text has already been inserted. I've renamed the event to khtmlTextInsertedEvent to make more sense. Also, Justin and I discussed changing subtreeHasChanged so that instead of always updating the InputElement's value, we could just invalidate it, and lazily update the value. I didn't do that for this patch, but I'm planning on doing it soon.

Created attachment 6878 [details] patch

Also, Hyatt and I talked about whether the new element class (now HTMLTextFieldInnerElementImpl) should move to the rendering directory, and decided to leave it in the html dir for now. We also talked about ways to eliminate the need for this class all together- by adding the ability to have default event listeners. Actually, I meant to add a fixme in HTMLTextFieldInnerElementImpl.cpp about this. I will do that in my local tree before I commit.

Comment on attachment 6878 [details] patch One major difference between this maxLength implementation and the one I did in KWQTextField is that this one limits you to a certain number of UTF-16 characters. But the one in KWQTextField limits you to a certain number of "composed character sequences". That means that an e with an umlaut over it counts as 1 character even though it can be two Unicode characters in a row (the e followed by the non-spacing umlaut) and a single Japanese character that is outside the "BMP" that requires two UTF-16 codes (a "surrogate pair") to encode also counts as a single character. The code that deals with this in KWQTextField is _KWQ_numComposedCharacterSequences and _KWQ_truncateToNumComposedCharacterSequences:. We will need to replicate this, although I guess it's fine not to at first, but I'd like to see another bug about that. To tell if a character is half of a surrogate pair, you use macros in <unicode/utf16.h>, such as U16_LENGTH or U16_IS_LEAD. To tell if the character is going to combine with the one before it is more difficult. There's code in CoreFoundation that does this analysis and I presume there's some way to do it with ICU, but I don't know what that is. In addition to determining such things, code will have to be careful not to do math on the length of strings, since composing means that "length of A plus length of B" is not necessarily the same as "length of A plus B". HTMLTextFieldInnerElementImpl does not need an explicit destructor. The compiler-generated destructor will do the job. So don't declare or define a destructor. +void HTMLTextFieldInnerElementImpl::defaultEventHandler(EventImpl *evt) The above should have the * next to the EventImpl. The various files that say: \ No newline at end of file should be fixed so they have a newline at the end of the file. RenderTextField.h should not include HTMLTextFieldInnerElementImpl.h. To compile, all it needs is a forward declaration of the class. The .cpp file can include the complete header. if (m_div) { - m_div->removeEventListener(DOMCharacterDataModifiedEvent, m_mutationListener.get(), false); m_div->detach(); } Should remove the braces from the above. + m_div = new HTMLTextFieldInnerElementImpl(document(), element()); node() is more efficient than element() in cases like the above. There's no need to include "PlatformString.h" in BeforeTextInsertedEventImpl.h -- dom2_eventsimpl.h contains classes with String in them, and because of that it has the class defined already. + BeforeTextInsertedEventImpl(String&); That should be "const String&". In the ReplaceSelectionCommand code to send BeforeTextInsertedEvent, I notice that you compute the text before you check if there's a frame. That's unnecessary and a little bit unclear. I suggest computing the range and the text right when you are about to use it. I'd also like to see you declare "ec" as ExceptionCode instead of int for clarity and name it ec in both places you use it instead of calling it exceptionCode in some places. These issues are minor ones. r=me

filed http://bugzilla.opendarwin.org/show_bug.cgi?id=7622 to address the composed character sequence issue.