Bug 13624 – REGRESSION: can select text of an input button

• Component: Forms
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Um.  This is kinda cool.  But a regression none the less.

You can reproduce on this bugzilla page by clicking on the space to the right of a button and then drag-selecting left.

[Eric Seidel (no email)]

Actually the selection behavior seems "correct", the selection rect is just smaller than expected based on the behavior in the previous safari.

[Eric Seidel (no email)]

Created attachment 14410 [details]
picture showing change.  TOT in foreground

Created attachment 14516 [details]
Test case for buttons and inline-block

This is essentially a problem with replaced elements that have selectable content. The "regression" part is that buttons are implemented as such in TOT.

<rdar://problem/6977976>

[Konstantin Shcheglov]

Just for records, for myself or may be for someone else.
I've asked about this in IRC and here is my discussion with <rniwa>.


<rniwa> scheglov: text inside button shouldn't be treated differently
<rniwa> scheglov: it should act like any other text in the document
<rniwa> scheglov: or else, <div contenteditable><button>hello</button></div> won't work properly
<scheglov> rniwa: hm... So, do you have idea what https://bugs.webkit.org/show_bug.cgi?id=13624 means?
<rniwa> scheglov: it's just rendering bug
<rniwa> scheglov: selection isn't rendered properly inside/around button
<scheglov> rniwa: Should be text on button  selected?
<rniwa> scheglov: yes
<scheglov> OK, you told that it should be selected.
<rniwa> scheglov: but when the entire button is selected, text selection shouldn't be doubly-rendered
<rniwa> scheglov: I bet it's some quirk with theme/selection interaction
<scheglov> So, "too much" variant is bug, right?
<rniwa> scheglov: all of them are bug!
<scheglov> rniwa: Oops...
<scheglov> rniwa: I wonder what is correct  rendering.
<scheglov> rniwa: Ah...
<scheglov> rniwa: So, "body" of button should be selected too?
<rniwa> scheglov: yes
<scheglov> rniwa: OK, now I understand. Thank you.
<rniwa> scheglov: but if the entire button is selected, any selection inside the button should NOT be rendered
<rniwa> scheglov: because that'll result in nested-selection rendering
<rniwa> scheglov: on the other hand, it might be hard to render the case where just the button is selected
<scheglov> rniwa: Yes, I see, in other case text is selected twice.
<rniwa> scheglov: because we canonicalize position
<scheglov> rniwa: Well, I don't understand this yet (hard and canonicalize), I need to read source.

[Alexey Proskuryakov]

In other words, text in a button in non-editable content should never have a selection of its own painted, correct?

[Ryosuke Niwa]

(In reply to comment #6)
> In other words, text in a button in non-editable content should never have a selection of its own painted, correct?

Yes. In non-editable area, user shouldn't be able to select contents inside the button.

[Parag Radke]

Created attachment 131825 [details]
Proposed Patch

Patch to avoid selection background for the button text.

[WebKit Review Bot]

Comment on attachment 131825 [details]
Proposed Patch

Attachment 131825 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11957037

New failing tests:
editing/selection/3690703-2.html
editing/selection/3690703.html
editing/selection/3690719.html
editing/selection/extend-by-word-002.html

[Parag Radke]

Created attachment 132220 [details]
Updated Patch

Patch to avoid selection background for the button text if button is inside non content editable area.

[Parag Radke]

Created attachment 132239 [details]
Updated Patch to fix merge conflicts

Patch to avoid selection background for the button text if it is inside non editable area.

[WebKit Review Bot]

Comment on attachment 132239 [details]
Updated Patch to fix merge conflicts

Attachment 132239 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11960339

New failing tests:
editing/selection/3690703-2.html
editing/selection/select-all-004.html
editing/pasteboard/input-field-1.html
editing/selection/4895428-3.html
editing/selection/3690703.html
editing/selection/3690719.html

[Tony Chang]

(In reply to comment #12)
> (From update of attachment 132239 [details])
> Attachment 132239 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11960339
> 
> New failing tests:
> editing/selection/3690703-2.html
> editing/selection/select-all-004.html
> editing/pasteboard/input-field-1.html
> editing/selection/4895428-3.html
> editing/selection/3690703.html
> editing/selection/3690719.html

Looking at the checked in pngs, some of these look like progressions.  E.g., editing/selection/3690703-2.html, editing/selection/3690703.html and editing/selection/3690719.html have buttons with selected text which I assume will no longer be selected after this change.

The other 4 tests, I'm not so sure about.  Do you know what the new results/diff looks like for them?

[Parag Radke]

Created attachment 132978 [details]
Updated Patch for chromium pixel test failures

Patch to avoid selection for the button text.

[Ryosuke Niwa]

Comment on attachment 132978 [details]
Updated Patch for chromium pixel test failures

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

r- due to inefficient implementation.

> Source/WebCore/rendering/RenderButton.h:51
> +    virtual bool canBeSelectionLeaf() const { return node()->rootEditableElement(); }

Finding rootEditableElement is very expensive. You should either call isContentEditable() or rendererIsEditable(). Call later if the layout is up-to-date whenever canBeSelectionLeaf is called.

> Source/WebCore/rendering/RenderTextFragment.h:42
> +    virtual bool canBeSelectionLeaf() const { return node()->rootEditableElement(); }

Ditto.

[Parag Radke]

(In reply to comment #15)
> Finding rootEditableElement is very expensive. You should either call isContentEditable() or rendererIsEditable(). Call later if the layout is up-to-date whenever canBeSelectionLeaf is called.

To call isContentEditable(), i need to make sure this should not get call during layout is happening. I found needslayout() but I think it just tells weather present node's layout is complete or not but i have to check the complete layout is over or not. Can you please suggest the appropriate API i can use here.

[Ryosuke Niwa]

(In reply to comment #16)
> (In reply to comment #15)
> > Finding rootEditableElement is very expensive. You should either call isContentEditable() or rendererIsEditable(). Call later if the layout is up-to-date whenever canBeSelectionLeaf is called.
> 
> To call isContentEditable(), i need to make sure this should not get call during layout is happening. I found needslayout() but I think it just tells weather present node's layout is complete or not but i have to check the complete layout is over or not. Can you please suggest the appropriate API i can use here.

I'm not sure what you mean... isContentEditable only triggers style recalc. If you're in the middle of a layout, then the style must be up to date.

[Parag Radke]

Created attachment 133514 [details]
Updated patch

Updated patch with all review comments implemented

[Ryosuke Niwa]

Comment on attachment 133514 [details]
Updated patch

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

> Source/WebCore/rendering/RenderButton.cpp:66
> +    if (node())
> +        return node()->isContentEditable();
> +
> +    return false;

Ugh... this is a render object. You should be calling rendererIsEditable instead. r- because isContentEditable can trigger style recalc.
It's also better to write this as return node() && node()->rendererIsEditable().

> Source/WebCore/rendering/RenderButton.h:43
> +    virtual bool canBeSelectionLeaf() const;

Please use OVERRIDE macro :)

> Source/WebCore/rendering/RenderTextFragment.cpp:68
> +    if (node())
> +        return node()->isContentEditable();
> +        
> +    return false;

Ditto.

> Source/WebCore/rendering/RenderTextFragment.h:42
> +    virtual bool canBeSelectionLeaf() const;

OVERRIDE.

[Parag Radke]

Created attachment 133540 [details]
Updated patch

Updated patch with all review comments implemented

[WebKit Review Bot]

Comment on attachment 133540 [details]
Updated patch

Rejecting attachment 133540 [details] from commit-queue.

New failing tests:
editing/selection/3690703-2.html
editing/selection/3690703.html
editing/selection/3690719.html
Full output: http://queues.webkit.org/results/12120820

[WebKit Review Bot]

Comment on attachment 133540 [details]
Updated patch

Attachment 133540 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12117822

New failing tests:
editing/selection/3690703-2.html
editing/selection/3690703.html
editing/selection/3690719.html

[WebKit Review Bot]

Created attachment 133566 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ryosuke Niwa]

Comment on attachment 133540 [details]
Updated patch

Clearing flags on attachment: 133540

Committed r111946: <http://trac.webkit.org/changeset/111946>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.