Bug 47870 – Remove RenderTextControl::setSelectionRange

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Remove enderTextControl::setSelectionRange because it instantiates VisiblePosition.

[Ryosuke Niwa]

Created attachment 71115 [details]
fixes the bug

[Ryosuke Niwa]

Can someone review this patch?

[Darin Adler]

Comment on attachment 71115 [details]
fixes the bug

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

Looks good. I think we can do this without putting RenderTextControl code into Element.

> WebCore/accessibility/AccessibilityRenderObject.cpp:2013
>      if (isNativeTextControl()) {
> -        RenderTextControl* textControl = toRenderTextControl(m_renderer);
> -        textControl->setSelectionRange(range.start, range.start + range.length);
> +        static_cast<Element*>(m_renderer->node())->setSelectionRange(range.start, range.start + range.length);
>          return;
>      }

Presumably what makes the cast here safe is that we know this is an Element since all native text controls are Elements. It seems a little arbitrary to have this call be a member of the Element class. Since it already has to check the type of the renderer, perhaps it should just be a function that takes a Node*, and not a member function at all. Then we wouldn’t need the typecast.

> WebCore/dom/Element.cpp:1402
> +void Element::setSelectionRange(int start, int end)

I see why this function takes a Node* argument, but not why it’s a member of Element (as I said above).

I think this function could be in the RenderTextControl.cpp file, it just needs to be changed so it takes a Node* rather than a RenderTextControl*, and thus either should be free function or a static member function. By leaving it in RenderTextControl.cpp you can even make the diff muhc smaller.

> WebCore/dom/Element.cpp:1407
> +    end = max(end, 0);
> +    start = min(max(start, 0), end);

Why do this before checking renderer()? I would do it later after the early exits.

> WebCore/html/HTMLFormControlElement.cpp:561
> +    RenderTextControl* textControl = toRenderTextControl(renderer());

I’d just use the word “control” for this local variable. Needs to be a word, but it’s local so there’s no need to be specific.

> WebCore/rendering/RenderTextControl.cpp:206
> +bool RenderTextControl::isVisible() const
> +{
> +    return style()->visibility() != HIDDEN && m_innerText && m_innerText->renderer() && m_innerText->renderBox()->height();
> +}

I think that given what this function does, it should be called hasVisibleTextArea or something along those lines.

[Ryosuke Niwa]

Created attachment 72353 [details]
Fixed per Darin's comment

[Ryosuke Niwa]

Created attachment 72355 [details]
Fixed some details

[Ryosuke Niwa]

(In reply to comment #4)
> Created an attachment (id=72353) [details]
> Fixed per Darin's comment

I'm not sure if this approach is necessarily cleaner because we end up having 3-4 global functions like that by the time I finish removing all selection-related functions from RenderTextControl.

(In reply to comment #3)
> Presumably what makes the cast here safe is that we know this is an Element since all native text controls are Elements. It seems a little arbitrary to have this call be a member of the Element class. Since it already has to check the type of the renderer, perhaps it should just be a function that takes a Node*, and not a member function at all. Then we wouldn’t need the typecast.

In some sense, Element was the LCD of all classes that called setSelectionRange. But I agree that Element having this function seems quite arbitrary.

> I think this function could be in the RenderTextControl.cpp file, it just needs to be changed so it takes a Node* rather than a RenderTextControl*, and thus either should be free function or a static member function. By leaving it in RenderTextControl.cpp you can even make the diff muhc smaller.

I did this in my new patch but I'm not sure if this is a good approach.  I'm not a big fun of converting member functions to global functions...

> > WebCore/dom/Element.cpp:1407
> > +    end = max(end, 0);
> > +    start = min(max(start, 0), end);
> 
> Why do this before checking renderer()? I would do it later after the early exits.

Good point.  Fixed.

> > WebCore/html/HTMLFormControlElement.cpp:561
> > +    RenderTextControl* textControl = toRenderTextControl(renderer());
> 
> I’d just use the word “control” for this local variable. Needs to be a word, but it’s local so there’s no need to be specific.

Fixed.

> > WebCore/rendering/RenderTextControl.cpp:206
> > +bool RenderTextControl::isVisible() const
> > +{
> > +    return style()->visibility() != HIDDEN && m_innerText && m_innerText->renderer() && m_innerText->renderBox()->height();
> > +}
> 
> I think that given what this function does, it should be called hasVisibleTextArea or something along those lines.

I made setSelectionRange a friend of RenderTextControl so that I don't have to make cacheSelection public and add this member function.

[Darin Adler]

Comment on attachment 72353 [details]
Fixed per Darin's comment

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

review- because of some of the null check stuff

> WebCore/dom/InputElement.cpp:111
> +    if (toRenderTextControl(element->renderer()))
> +        setSelectionRange(element, start, end);

There is no need call toRenderTextControl just to check something for null. There’s no need for a null check here, since setSelectionRange already takes care of it.

> WebCore/html/HTMLFormControlElement.cpp:551
> +    WebCore::setSelectionRange(this, start, std::max(start, selectionEnd()));

I think you can use just "max", not "std::max", here. Also, seems fine to just call setSelectionRange, since it turns around and calls WebCore::setSelectionRange, both here and in the rest of the functions in this file.

> WebCore/html/HTMLFormControlElement.cpp:556
> +    WebCore::setSelectionRange(this, std::min(end, selectionStart()), end);

I think you can use just "min", not "std::min", here.

> WebCore/html/HTMLFormControlElement.cpp:562
> +    if (RenderTextControl* textControl = toRenderTextControl(renderer()))
> +        WebCore::setSelectionRange(this, 0, textControl->text().length());

I think instead you can write:

    setSelectionRange(this, 0, numeric_limits<int>::max())

Avoid the need for a typecast or a local variable.

> WebCore/rendering/RenderTextControl.cpp:219
> +void setSelectionRange(Node* node, int start, int end)

Do we need to ref/deref node? Is there any danger that this code results in JavaScript code running that could delete the node?

> WebCore/rendering/RenderTextControl.cpp:230
> +    RenderTextControl* textControl = toRenderTextControl(node->renderer());

I would name this local variable just “control” for brevity.

> WebCore/rendering/RenderTextControl.cpp:234
> +    if (textControl->style()->visibility() == HIDDEN || !textControl->m_innerText
> +        || !textControl->m_innerText->renderer() || !textControl->m_innerText->renderBox()->height()) {
> +        textControl->cacheSelection(start, end);

I find this formatting, where the || lines up with the code below it, so hard to read that I go out of my way to avoid this sort of thing. Maybe we can factor all or part of this check into an inline helper function to avoid the long body for the if.

[Early Warning System Bot]

Attachment 72353 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4775068

[Early Warning System Bot]

Attachment 72355 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4785072

[Ryosuke Niwa]

[e-mail]

(In reply to comment #8)
> Attachment 72353 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/4775068

This failure is happening in QWebPagePrivate::inputMethodEvent in WebKit/qt/qwebpage.cpp:

        case QInputMethodEvent::Selection: {
            hasSelection = true;
            // A selection in the inputMethodEvent is always reflected in the visible text
            if (renderer && renderer->isTextControl()) {
                renderTextControl->setSelectionStart(qMin(a.start, (a.start + a.length)));
                renderTextControl->setSelectionEnd(qMax(a.start, (a.start + a.length)));
            }

I don't understand why WebKit layer is accessing RenderTextControl.  These functions aren't even exported.

[Ryosuke Niwa]

Created attachment 72373 [details]
More fixes per Darin's comment

[Ryosuke Niwa]

(In reply to comment #7)
> > WebCore/dom/InputElement.cpp:111
> > +    if (toRenderTextControl(element->renderer()))
> > +        setSelectionRange(element, start, end);
> 
> There is no need call toRenderTextControl just to check something for null. There’s no need for a null check here, since setSelectionRange already takes care of it.

Oops, I missed that one.  Thanks for noticing that.

> > WebCore/html/HTMLFormControlElement.cpp:551
> > +    WebCore::setSelectionRange(this, start, std::max(start, selectionEnd()));
> 
> I think you can use just "max", not "std::max", here. Also, seems fine to just call setSelectionRange, since it turns around and calls WebCore::setSelectionRange, both here and in the rest of the functions in this file.

> I think you can use just "min", not "std::min", here.

Done.

> > WebCore/html/HTMLFormControlElement.cpp:562
> > +    if (RenderTextControl* textControl = toRenderTextControl(renderer()))
> > +        WebCore::setSelectionRange(this, 0, textControl->text().length());
> 
> I think instead you can write:
>     setSelectionRange(this, 0, numeric_limits<int>::max())

I don't think so. Neither setSelectionRange nor visiblePositionForIndex adjusts the offset.

> > WebCore/rendering/RenderTextControl.cpp:219
> > +void setSelectionRange(Node* node, int start, int end)
> 
> Do we need to ref/deref node? Is there any danger that this code results in JavaScript code running that could delete the node?

No.  Although selection change may trigger some events to fire, this is done at the very end of function at which point the existance of node is irrelevant.

> > WebCore/rendering/RenderTextControl.cpp:230
> > +    RenderTextControl* textControl = toRenderTextControl(node->renderer());
> 
> I would name this local variable just “control” for brevity.

Sorry, I forgot to fix that.

> > WebCore/rendering/RenderTextControl.cpp:234
> > +    if (textControl->style()->visibility() == HIDDEN || !textControl->m_innerText
> > +        || !textControl->m_innerText->renderer() || !textControl->m_innerText->renderBox()->height()) {
> > +        textControl->cacheSelection(start, end);
> 
> I find this formatting, where the || lines up with the code below it, so hard to read that I go out of my way to avoid this sort of thing. Maybe we can factor all or part of this check into an inline helper function to avoid the long body for the if.

Added hasVisibleTextArea back.

[Early Warning System Bot]

Attachment 72373 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4769075

[Ryosuke Niwa]

Created attachment 72381 [details]
qt build fix

[Darin Adler]

(In reply to comment #12)
> > > WebCore/html/HTMLFormControlElement.cpp:562
> > > +    if (RenderTextControl* textControl = toRenderTextControl(renderer()))
> > > +        WebCore::setSelectionRange(this, 0, textControl->text().length());
> > 
> > I think instead you can write:
> >     setSelectionRange(this, 0, numeric_limits<int>::max())
> 
> I don't think so. Neither setSelectionRange nor visiblePositionForIndex adjusts the offset.

I think you are wrong.

The visiblePositionForIndex function handles large offsets correctly, because the CharacterIterator is limited to the range of the node contents. The only thing done with the index is that it’s passed to CharacterIterator, which will handle a massive number just fine.

[Darin Adler]

Comment on attachment 72381 [details]
qt build fix

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

> WebCore/html/HTMLFormControlElement.cpp:563
>      if (RenderTextControl* renderer = textRendererAfterUpdateLayout())
> -        renderer->select();
> +        setSelectionRange(0, renderer->text().length());

I still stand by my earlier suggestion. You can call setSelectionRange(0, numeric_limits<int>::max()) and it will work fine; seems better than calling textRendererAfterUpdateLayout here. Too bad numeric_limits is so ugly.

> WebCore/rendering/RenderTextControl.h:113
> +    bool hasVisibleTextArea() const;
> +    friend void setSelectionRange(Node*, int start, int end);

I’d like to see these up with the private function members rather than at the bottom after the private data members.

> WebKit/qt/Api/qwebpage.cpp:1057
> +            if (renderer && renderer->isTextControl() && renderer->node())
> +                setSelectionRange(renderer->node(), qMin(a.start, (a.start + a.length)), qMax(a.start, (a.start + a.length)));

No need for the isTextControl() or node() checks in this if statement. The setSelectionRange function does those checks already.

[Ryosuke Niwa]

(In reply to comment #15)
> I think you are wrong.
> 
> The visiblePositionForIndex function handles large offsets correctly, because the CharacterIterator is limited to the range of the node contents. The only thing done with the index is that it’s passed to CharacterIterator, which will handle a massive number just fine.

Ah, I didn't know that.

(In reply to comment #16)
> (From update of attachment 72381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72381&action=review
> 
> > WebCore/html/HTMLFormControlElement.cpp:563
> >      if (RenderTextControl* renderer = textRendererAfterUpdateLayout())
> > -        renderer->select();
> > +        setSelectionRange(0, renderer->text().length());
> 
> I still stand by my earlier suggestion. You can call setSelectionRange(0, numeric_limits<int>::max()) and it will work fine; seems better than calling textRendererAfterUpdateLayout here. Too bad numeric_limits is so ugly.

Will try.

> > WebCore/rendering/RenderTextControl.h:113
> > +    bool hasVisibleTextArea() const;
> > +    friend void setSelectionRange(Node*, int start, int end);
> 
> I’d like to see these up with the private function members rather than at the bottom after the private data members.

Will do.

> > WebKit/qt/Api/qwebpage.cpp:1057
> > +            if (renderer && renderer->isTextControl() && renderer->node())
> > +                setSelectionRange(renderer->node(), qMin(a.start, (a.start + a.length)), qMax(a.start, (a.start + a.length)));
> 
> No need for the isTextControl() or node() checks in this if statement. The setSelectionRange function does those checks already.

I think we still do need to check renderer->node() here because setSelectionRange only assert that node is not null.  Or should we add early exit to setSelectionRange when node == 0?

[Ryosuke Niwa]

Committed r70945: <http://trac.webkit.org/changeset/70945>