Bug 13624

Summary: REGRESSION: can select text of an input button
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dglazkov, inferno, mitz, parag, rniwa, scheglov, tony, webkit.review.bot
Priority: P1 Keywords: HasReduction, InRadar, Regression
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
picture showing change. TOT in foreground
none
Test case for buttons and inline-block
none
Proposed Patch
none
Updated Patch
none
Updated Patch to fix merge conflicts
none
Updated Patch for chromium pixel test failures
none
Updated patch
none
Updated patch
none
Archive of layout-test-results from ec2-cr-linux-04 none

Description Eric Seidel (no email) 2007-05-08 04:22:32 PDT
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.
Comment 1 Eric Seidel (no email) 2007-05-08 04:24:15 PDT
Actually the selection behavior seems "correct", the selection rect is just smaller than expected based on the behavior in the previous safari.
Comment 2 Eric Seidel (no email) 2007-05-08 04:24:52 PDT
Created attachment 14410 [details]
picture showing change.  TOT in foreground
Comment 3 mitz 2007-05-12 06:17:53 PDT
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.
Comment 4 mitz 2009-06-17 00:28:47 PDT
<rdar://problem/6977976>
Comment 5 Konstantin Shcheglov 2011-09-16 11:50:29 PDT
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.
Comment 6 Alexey Proskuryakov 2011-09-16 16:35:04 PDT
In other words, text in a button in non-editable content should never have a selection of its own painted, correct?
Comment 7 Ryosuke Niwa 2011-09-16 16:36:07 PDT
(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.
Comment 8 Parag Radke 2012-03-14 04:38:47 PDT
Created attachment 131825 [details]
Proposed Patch

Patch to avoid selection background for the button text.
Comment 9 WebKit Review Bot 2012-03-14 07:28:18 PDT
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
Comment 10 Parag Radke 2012-03-16 00:44:33 PDT
Created attachment 132220 [details]
Updated Patch

Patch to avoid selection background for the button text if button is inside non content editable area.
Comment 11 Parag Radke 2012-03-16 03:07:49 PDT
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.
Comment 12 WebKit Review Bot 2012-03-16 04:17:48 PDT
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
Comment 13 Tony Chang 2012-03-19 14:08:56 PDT
(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?
Comment 14 Parag Radke 2012-03-20 23:20:38 PDT
Created attachment 132978 [details]
Updated Patch for chromium pixel test failures

Patch to avoid selection for the button text.
Comment 15 Ryosuke Niwa 2012-03-20 23:28:53 PDT
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.
Comment 16 Parag Radke 2012-03-21 06:49:32 PDT
(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.
Comment 17 Ryosuke Niwa 2012-03-21 09:58:03 PDT
(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.
Comment 18 Parag Radke 2012-03-23 11:08:43 PDT
Created attachment 133514 [details]
Updated patch

Updated patch with all review comments implemented
Comment 19 Ryosuke Niwa 2012-03-23 11:42:52 PDT
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.
Comment 20 Parag Radke 2012-03-23 12:48:16 PDT
Created attachment 133540 [details]
Updated patch

Updated patch with all review comments implemented
Comment 21 WebKit Review Bot 2012-03-23 13:32:53 PDT
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
Comment 22 WebKit Review Bot 2012-03-23 14:46:54 PDT
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
Comment 23 WebKit Review Bot 2012-03-23 14:47:05 PDT
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
Comment 24 Ryosuke Niwa 2012-03-23 17:54:08 PDT
Comment on attachment 133540 [details]
Updated patch

Clearing flags on attachment: 133540

Committed r111946: <http://trac.webkit.org/changeset/111946>
Comment 25 Ryosuke Niwa 2012-03-23 17:54:25 PDT
All reviewed patches have been landed.  Closing bug.